[Tarantool-patches] [PATCH 0/7] Stacked diagnostics area

Tarantool development patches archive
 help / color / mirror / Atom feed

* [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area
@ 2020-02-19 14:16 Nikita Pettik
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 1/7] box: rename diag_add_error to diag_set_error Nikita Pettik
                   ` (7 more replies)
  0 siblings, 8 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:16 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy

Branch: https://github.com/tarantool/tarantool/commits/np/gh-1148-stacked-diag
Issue:
https://github.com/tarantool/tarantool/issues/1148
https://github.com/tarantool/tarantool/issues/4778

Patch-set basically consists of two parts: first one fixes undocumented
behaviour of box.error.new() which sets created error to diagnostics
area. The reason why it is required is described in the corresponding
github ticked. Second part is about stacked diagnostics itself: patch
error structure extending it with double linked list, provide Lua and
C interfaces to interact with it, support stacked diagnostics in IProto
protocol and net.box module (all points according to the rfc: 
https://github.com/tarantool/tarantool/commit/1acd32d98f628431429b427df19caa9d269bb9c8).

Kirill Shcherbatov (2):
  box: rename diag_add_error to diag_set_error
  iproto: refactor error encoding with mpstream

Nikita Pettik (5):
  box/error: introduce box.error.set() method
  box/error: don't set error created via box.error.new to diag
  box: introduce stacked diagnostic area
  box/error: clarify purpose of reference counting in struct error
  iproto: support error stacked diagnostic area

 extra/exports                   |   2 +
 src/box/applier.cc              |   2 +-
 src/box/error.cc                |  33 +++++
 src/box/error.h                 |  24 ++++
 src/box/iproto_constants.h      |   6 +
 src/box/key_list.c              |  16 +--
 src/box/lua/call.c              |   6 +-
 src/box/lua/error.cc            |  71 ++++++----
 src/box/lua/net_box.lua         |  32 ++++-
 src/box/relay.cc                |   4 +-
 src/box/vy_scheduler.c          |   6 +-
 src/box/xrow.c                  | 150 ++++++++++++++++----
 src/lib/core/diag.c             |  51 +++++++
 src/lib/core/diag.h             | 110 ++++++++++++++-
 src/lib/core/exception.cc       |   2 +-
 src/lib/core/exception.h        |   2 +-
 src/lua/error.c                 |   2 +-
 src/lua/error.h                 |   3 +
 src/lua/error.lua               |  40 ++++++
 src/lua/utils.c                 |   2 +-
 test/box-py/iproto.result       |   6 +-
 test/box-py/iproto.test.py      |   6 +-
 test/box/iproto.result          | 166 ++++++++++++++++++++++
 test/box/iproto.test.lua        |  73 ++++++++++
 test/box/misc.result            | 295 ++++++++++++++++++++++++++++++++++++++++
 test/box/misc.test.lua          | 119 ++++++++++++++++
 test/box/net.box.result         |  65 +++++++++
 test/box/net.box.test.lua       |  25 ++++
 test/engine/func_index.result   |  50 +++++--
 test/engine/func_index.test.lua |   7 +
 30 files changed, 1282 insertions(+), 94 deletions(-)
 create mode 100644 test/box/iproto.result
 create mode 100644 test/box/iproto.test.lua

-- 
2.15.1

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Tarantool-patches] [PATCH 1/7] box: rename diag_add_error to diag_set_error
  2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
@ 2020-02-19 14:16 ` Nikita Pettik
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 2/7] box/error: introduce box.error.set() method Nikita Pettik
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:16 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy

From: Kirill Shcherbatov <kshcherbatov@tarantool.org>

Let's rename diag_add_error() to diag_set_error() because it actually
replaces an error object in diagnostic area with a new one and this name
is not representative. Moreover, we are going to introduce a new
diag_add_error() which will place error at the top of stack diagnostic
area.

Needed for #1148
---
 src/box/applier.cc        | 2 +-
 src/box/error.cc          | 2 +-
 src/box/relay.cc          | 4 ++--
 src/box/vy_scheduler.c    | 6 +++---
 src/lib/core/diag.h       | 6 +++---
 src/lib/core/exception.cc | 2 +-
 src/lib/core/exception.h  | 2 +-
 src/lua/utils.c           | 2 +-
 8 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/box/applier.cc b/src/box/applier.cc
index ae3d281a5..5871bea69 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -843,7 +843,7 @@ applier_on_rollback(struct trigger *trigger, void *event)
 	struct applier *applier = (struct applier *)trigger->data;
 	/* Setup a shared error. */
 	if (!diag_is_empty(&replicaset.applier.diag)) {
-		diag_add_error(&applier->diag,
+		diag_set_error(&applier->diag,
 			       diag_last_error(&replicaset.applier.diag));
 	}
 	/* Stop the applier fiber. */
diff --git a/src/box/error.cc b/src/box/error.cc
index 47dce3305..7dfe1b3ee 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -82,7 +82,7 @@ box_error_set(const char *file, unsigned line, uint32_t code,
 		error_vformat_msg(e, fmt, ap);
 		va_end(ap);
 	}
-	diag_add_error(&fiber()->diag, e);
+	diag_set_error(&fiber()->diag, e);
 	return -1;
 }
 
diff --git a/src/box/relay.cc b/src/box/relay.cc
index b89632273..1855e422c 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -472,7 +472,7 @@ relay_set_error(struct relay *relay, struct error *e)
 {
 	/* Don't override existing error. */
 	if (diag_is_empty(&relay->diag))
-		diag_add_error(&relay->diag, e);
+		diag_set_error(&relay->diag, e);
 }
 
 static void
@@ -651,7 +651,7 @@ relay_subscribe_f(va_list ap)
 	 * Don't clear the error for status reporting.
 	 */
 	assert(!diag_is_empty(&relay->diag));
-	diag_add_error(diag_get(), diag_last_error(&relay->diag));
+	diag_set_error(diag_get(), diag_last_error(&relay->diag));
 	diag_log();
 	say_crit("exiting the relay loop");
 
diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
index 86bed8013..cd1c19e66 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -619,7 +619,7 @@ vy_scheduler_dump(struct vy_scheduler *scheduler)
 		if (scheduler->is_throttled) {
 			/* Dump error occurred. */
 			struct error *e = diag_last_error(&scheduler->diag);
-			diag_add_error(diag_get(), e);
+			diag_set_error(diag_get(), e);
 			return -1;
 		}
 		fiber_cond_wait(&scheduler->dump_cond);
@@ -693,7 +693,7 @@ vy_scheduler_begin_checkpoint(struct vy_scheduler *scheduler)
 	 */
 	if (scheduler->is_throttled) {
 		struct error *e = diag_last_error(&scheduler->diag);
-		diag_add_error(diag_get(), e);
+		diag_set_error(diag_get(), e);
 		say_error("cannot checkpoint vinyl, "
 			  "scheduler is throttled with: %s", e->errmsg);
 		return -1;
@@ -729,7 +729,7 @@ vy_scheduler_wait_checkpoint(struct vy_scheduler *scheduler)
 		if (scheduler->is_throttled) {
 			/* A dump error occurred, abort checkpoint. */
 			struct error *e = diag_last_error(&scheduler->diag);
-			diag_add_error(diag_get(), e);
+			diag_set_error(diag_get(), e);
 			say_error("vinyl checkpoint failed: %s", e->errmsg);
 			return -1;
 		}
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index f763957c2..7e1e1a174 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -168,12 +168,12 @@ diag_clear(struct diag *diag)
 }
 
 /**
- * Add a new error to the diagnostics area
+ * Set a new error to the diagnostics area, replacing existent.
  * \param diag diagnostics area
  * \param e error to add
  */
 static inline void
-diag_add_error(struct diag *diag, struct error *e)
+diag_set_error(struct diag *diag, struct error *e)
 {
 	assert(e != NULL);
 	error_ref(e);
@@ -275,7 +275,7 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
 	say_debug("%s at %s:%i", #class, __FILE__, __LINE__);		\
 	struct error *e;						\
 	e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__);		\
-	diag_add_error(diag_get(), e);					\
+	diag_set_error(diag_get(), e);					\
 	/* Restore the errno which might have been reset.  */           \
 	errno = save_errno;                                             \
 } while (0)
diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
index 76dcea553..0ab10c4bd 100644
--- a/src/lib/core/exception.cc
+++ b/src/lib/core/exception.cc
@@ -99,7 +99,7 @@ Exception::operator new(size_t size)
 	void *buf = malloc(size);
 	if (buf != NULL)
 		return buf;
-	diag_add_error(diag_get(), &out_of_memory);
+	diag_set_error(diag_get(), &out_of_memory);
 	throw &out_of_memory;
 }
 
diff --git a/src/lib/core/exception.h b/src/lib/core/exception.h
index d6154eb32..1947b4f00 100644
--- a/src/lib/core/exception.h
+++ b/src/lib/core/exception.h
@@ -177,7 +177,7 @@ exception_init();
 #define tnt_error(class, ...) ({					\
 	say_debug("%s at %s:%i", #class, __FILE__, __LINE__);		\
 	class *e = new class(__FILE__, __LINE__, ##__VA_ARGS__);	\
-	diag_add_error(diag_get(), e);					\
+	diag_set_error(diag_get(), e);					\
 	e;								\
 })
 
diff --git a/src/lua/utils.c b/src/lua/utils.c
index de14c778c..54d18ac89 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -1022,7 +1022,7 @@ luaT_toerror(lua_State *L)
 	struct error *e = luaL_iserror(L, -1);
 	if (e != NULL) {
 		/* Re-throw original error */
-		diag_add_error(&fiber()->diag, e);
+		diag_set_error(&fiber()->diag, e);
 	} else {
 		/* Convert Lua error to a Tarantool exception. */
 		diag_set(LuajitError, luaT_tolstring(L, -1, NULL));
-- 
2.15.1

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Tarantool-patches] [PATCH 2/7] box/error: introduce box.error.set() method
  2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 1/7] box: rename diag_add_error to diag_set_error Nikita Pettik
@ 2020-02-19 14:16 ` Nikita Pettik
  2020-02-19 14:26   ` Cyrill Gorcunov
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag Nikita Pettik
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:16 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy

box.error.set(err) sets err to instance's diagnostics area. Argument err
is supposed to be instance of error object. This method is required
since we are going to avoid adding created via box.error.new() errors to
Tarantool's diagnostic area.

Needed for #1148
Part of #4778
---
 src/box/lua/error.cc   | 15 +++++++++++++++
 src/lua/error.c        |  2 +-
 src/lua/error.h        |  3 +++
 test/box/misc.result   | 34 ++++++++++++++++++++++++++++++++++
 test/box/misc.test.lua | 14 ++++++++++++++
 5 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index fc53a40f4..7311cb2ce 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -154,6 +154,17 @@ luaT_error_clear(lua_State *L)
 	return 0;
 }
 
+static int
+luaT_error_set(lua_State *L)
+{
+	if (lua_gettop(L) == 0)
+		return luaL_error(L, "Usage: box.error.set(error)");
+	struct error *e = luaL_checkerror(L, 1);
+	assert(e != NULL);
+	diag_set_error(&fiber()->diag, e);
+	return 0;
+}
+
 static int
 lbox_errinj_set(struct lua_State *L)
 {
@@ -268,6 +279,10 @@ box_lua_error_init(struct lua_State *L) {
 			lua_pushcfunction(L, luaT_error_new);
 			lua_setfield(L, -2, "new");
 		}
+		{
+			lua_pushcfunction(L, luaT_error_set);
+			lua_setfield(L, -2, "set");
+		}
 		lua_setfield(L, -2, "__index");
 	}
 	lua_setmetatable(L, -2);
diff --git a/src/lua/error.c b/src/lua/error.c
index d82e78dc4..18a990a88 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -53,7 +53,7 @@ luaL_iserror(struct lua_State *L, int narg)
 	return e;
 }
 
-static struct error *
+struct error *
 luaL_checkerror(struct lua_State *L, int narg)
 {
 	struct error *error = luaL_iserror(L, narg);
diff --git a/src/lua/error.h b/src/lua/error.h
index 64fa5eba3..16cdaf7fe 100644
--- a/src/lua/error.h
+++ b/src/lua/error.h
@@ -65,6 +65,9 @@ luaT_pusherror(struct lua_State *L, struct error *e);
 struct error *
 luaL_iserror(struct lua_State *L, int narg);
 
+struct error *
+luaL_checkerror(struct lua_State *L, int narg);
+
 void
 tarantool_lua_error_init(struct lua_State *L);
 
diff --git a/test/box/misc.result b/test/box/misc.result
index 5ac5e0f26..48c00aadc 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -290,6 +290,40 @@ type(err.errno)
 ---
 - nil
 ...
+-- gh-4778: don't add created via box.error.new() errors to
+-- Tarantool's diagnostic area.
+--
+err = box.error.new({code = 111, reason = "cause"})
+---
+...
+assert(box.error.last() ~= err)
+---
+- error: assertion failed!
+...
+box.error.set(err)
+---
+...
+assert(box.error.last() == err)
+---
+- true
+...
+-- Consider wrong or tricky inputs to box.error.set()
+--
+box.error.set(1)
+---
+- error: 'Invalid argument #1 (error expected, got number)'
+...
+box.error.set(nil)
+---
+- error: 'Invalid argument #1 (error expected, got nil)'
+...
+box.error.set(box.error.last())
+---
+...
+assert(box.error.last() == err)
+---
+- true
+...
 ----------------
 -- # box.stat
 ----------------
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index e1c2f990f..cb7b775fc 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -91,6 +91,20 @@ type(err.errno)
 err = box.error.new(box.error.PROC_LUA, "errno")
 type(err.errno)
 
+-- gh-4778: don't add created via box.error.new() errors to
+-- Tarantool's diagnostic area.
+--
+err = box.error.new({code = 111, reason = "cause"})
+assert(box.error.last() ~= err)
+box.error.set(err)
+assert(box.error.last() == err)
+-- Consider wrong or tricky inputs to box.error.set()
+--
+box.error.set(1)
+box.error.set(nil)
+box.error.set(box.error.last())
+assert(box.error.last() == err)
+
 ----------------
 -- # box.stat
 ----------------
-- 
2.15.1

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag
  2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 1/7] box: rename diag_add_error to diag_set_error Nikita Pettik
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 2/7] box/error: introduce box.error.set() method Nikita Pettik
@ 2020-02-19 14:16 ` Nikita Pettik
  2020-02-22 17:18   ` Vladislav Shpilevoy
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area Nikita Pettik
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:16 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy

To achieve this let's refactor luaT_error_create() to return error
object instead of setting it via box_error_set().
luaT_error_created() is used both to handle box.error() and
box.error.new() invocations, and box.error() is still expected to set
error to diagnostic area. So, luaT_error_call() which implements
box.error() processing at the end calls diag_set_error().
It is worth mentioning that net.box module relied on the fact that
box.error.new() set error to diagnostic area: otherwise request errors
don't get to diagnostic area on client side.

@TarantoolBot document
Title: Don't promote error created via box.error.new to diagnostic area

Now box.error.new() only creates error object, but doesn't set it to
Tarantool's diagnostic area:
box.error.clear()
e = box.error.new({code = 111, reason = "cause"})
assert(box.error.last() == nil)
---
- true
...

To set error in diagnostic area explicitly box.error.set() has been
introduced. It accepts error object which is set as last system error
(i.e. becomes available via box.error.last()).
Finally, box.error.new() does not longer accept error object as an
argument (this was undocumented feature).
Note that patch does not affect box.error(), which still pushed error to
diagnostic area. This fact is reflected in docs:
'''
Emulate a request error, with text based on one of the pre-defined
Tarantool errors...
'''

Needed for #1148
Closes #4778
---
 src/box/error.cc       | 16 +++++++++++++++
 src/box/error.h        |  8 ++++++++
 src/box/lua/error.cc   | 56 ++++++++++++++++++++++++++++----------------------
 test/box/misc.result   | 30 ++++++++++++++++++++++++++-
 test/box/misc.test.lua | 16 +++++++++++++++
 5 files changed, 101 insertions(+), 25 deletions(-)

diff --git a/src/box/error.cc b/src/box/error.cc
index 7dfe1b3ee..824a4617c 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -86,6 +86,22 @@ box_error_set(const char *file, unsigned line, uint32_t code,
 	return -1;
 }
 
+struct error *
+box_error_construct(const char *file, unsigned line, uint32_t code,
+		    const char *fmt, ...)
+{
+	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
+	ClientError *client_error = type_cast(ClientError, e);
+	if (client_error != NULL) {
+		client_error->m_errcode = code;
+		va_list ap;
+		va_start(ap, fmt);
+		error_vformat_msg(e, fmt, ap);
+		va_end(ap);
+	}
+	return e;
+}
+
 /* }}} */
 
 struct rmean *rmean_error = NULL;
diff --git a/src/box/error.h b/src/box/error.h
index b8c7cf73d..42043ef80 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -129,6 +129,14 @@ int
 box_error_set(const char *file, unsigned line, uint32_t code,
 	      const char *format, ...);
 
+/**
+ * Construct error object without setting it in the diagnostics
+ * area. On the memory allocation fail returns NULL.
+ */
+struct error *
+box_error_construct(const char *file, unsigned line, uint32_t code,
+		    const char *fmt, ...);
+
 /**
  * A backward-compatible API define.
  */
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index 7311cb2ce..17d4de653 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -42,7 +42,14 @@ extern "C" {
 #include "lua/utils.h"
 #include "box/error.h"
 
-static void
+/**
+ * Parse Lua arguments (they can come as single table
+ * f({code : number, reason : string}) or as separate members
+ * f(code, reason)) and construct struct error with given values.
+ * In case one of arguments is missing it corresponding field
+ * in struct error is filled with default value.
+ */
+static struct error *
 luaT_error_create(lua_State *L, int top_base)
 {
 	uint32_t code = 0;
@@ -69,25 +76,19 @@ luaT_error_create(lua_State *L, int top_base)
 			reason = lua_tostring(L, -1);
 		} else if (strchr(reason, '%') != NULL) {
 			/* Missing arguments to format string */
-			luaL_error(L, "box.error(): bad arguments");
-		}
-	} else if (top == top_base) {
-		if (lua_istable(L, top_base)) {
-			/* A special case that rethrows raw error (used by net.box) */
-			lua_getfield(L, top_base, "code");
-			code = lua_tonumber(L, -1);
-			lua_pop(L, 1);
-			lua_getfield(L, top_base, "reason");
-			reason = lua_tostring(L, -1);
-			if (reason == NULL)
-				reason = "";
-			lua_pop(L, 1);
-		} else if (luaL_iserror(L, top_base)) {
-			lua_error(L);
-			return;
+			return NULL;
 		}
+	} else if (top == top_base && lua_istable(L, top_base)) {
+		lua_getfield(L, top_base, "code");
+		code = lua_tonumber(L, -1);
+		lua_pop(L, 1);
+		lua_getfield(L, top_base, "reason");
+		reason = lua_tostring(L, -1);
+		if (reason == NULL)
+			reason = "";
+		lua_pop(L, 1);
 	} else {
-		luaL_error(L, "box.error(): bad arguments");
+		return NULL;
 	}
 
 raise:
@@ -101,8 +102,7 @@ raise:
 		}
 		line = info.currentline;
 	}
-	say_debug("box.error() at %s:%i", file, line);
-	box_error_set(file, line, code, "%s", reason);
+	return box_error_construct(file, line, code, "%s", reason);
 }
 
 static int
@@ -111,10 +111,15 @@ luaT_error_call(lua_State *L)
 	if (lua_gettop(L) <= 1) {
 		/* Re-throw saved exceptions if any. */
 		if (box_error_last())
-			luaT_error(L);
+			return luaT_error(L);
 		return 0;
 	}
-	luaT_error_create(L, 2);
+	if (lua_gettop(L) == 2 && luaL_iserror(L, 2))
+		return lua_error(L);
+	struct error *e = luaT_error_create(L, 2);
+	if (e == NULL)
+		return luaL_error(L, "box.error(): bad arguments");
+	diag_set_error(&fiber()->diag, e);
 	return luaT_error(L);
 }
 
@@ -139,9 +144,12 @@ luaT_error_new(lua_State *L)
 {
 	if (lua_gettop(L) == 0)
 		return luaL_error(L, "Usage: box.error.new(code, args)");
-	luaT_error_create(L, 1);
+	struct error *e = luaT_error_create(L, 1);
+	if (e == NULL)
+		return luaL_error(L, "box.error.new(): bad arguments");
 	lua_settop(L, 0);
-	return luaT_error_last(L);
+	luaT_pusherror(L, e);
+	return 1;
 }
 
 static int
diff --git a/test/box/misc.result b/test/box/misc.result
index 48c00aadc..b0a81a055 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -298,7 +298,7 @@ err = box.error.new({code = 111, reason = "cause"})
 ...
 assert(box.error.last() ~= err)
 ---
-- error: assertion failed!
+- true
 ...
 box.error.set(err)
 ---
@@ -324,6 +324,34 @@ assert(box.error.last() == err)
 ---
 - true
 ...
+-- Check that box.error.new() does not set error to diag.
+--
+box.error.clear()
+---
+...
+err = box.error.new(1, "cause")
+---
+...
+assert(box.error.last() == nil)
+---
+- true
+...
+-- box.error.new() does not accept error objects.
+--
+box.error.new(err)
+---
+- error: 'box.error.new(): bad arguments'
+...
+-- box.error() is supposed to re-throw last diagnostic error.
+-- Make sure it does not fail if there's no errors at all
+-- (in diagnostics area).
+--
+box.error.clear()
+---
+...
+box.error()
+---
+...
 ----------------
 -- # box.stat
 ----------------
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index cb7b775fc..0351317dd 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -104,6 +104,22 @@ box.error.set(1)
 box.error.set(nil)
 box.error.set(box.error.last())
 assert(box.error.last() == err)
+-- Check that box.error.new() does not set error to diag.
+--
+box.error.clear()
+err = box.error.new(1, "cause")
+assert(box.error.last() == nil)
+
+-- box.error.new() does not accept error objects.
+--
+box.error.new(err)
+
+-- box.error() is supposed to re-throw last diagnostic error.
+-- Make sure it does not fail if there's no errors at all
+-- (in diagnostics area).
+--
+box.error.clear()
+box.error()
 
 ----------------
 -- # box.stat
-- 
2.15.1

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area
  2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
                   ` (2 preceding siblings ...)
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag Nikita Pettik
@ 2020-02-19 14:16 ` Nikita Pettik
  2020-02-19 21:10   ` Vladislav Shpilevoy
  2020-02-23 17:43   ` Vladislav Shpilevoy
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 5/7] box/error: clarify purpose of reference counting in struct error Nikita Pettik
                   ` (3 subsequent siblings)
  7 siblings, 2 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:16 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy

In terms of implementation, now struct error objects can be organized
into double-linked lists. To achieve this pointers to the next and
previous elements have been added to struct error. It is worth
mentioning that already existing rlist and stailq list implementations
are not suitable: rlist is cycled list, as a result it is impossible to
start iteration over the list from random list entry and finish it at
the logical end of the list; stailq is single-linked list leaving no
possibility to remove elements from the middle of the list.

As a part of C interface, box_error_add() has been introduced. In
contrast to box_error_set() it does not replace last raised error, but
instead it adds error to the list of diagnostic errors having already
been set. If error is to be deleted (its reference counter hits 0 value)
it is unlinked from the list it belongs to and destroyed.

To organize errors into lists in Lua, table representing error object in
Lua now has .prev field (corresponding to 'previous' error) and method
:set_prev(e). The latter accepts error object (i.e. created via
box.error.new() or box.error.last()) and nil value. Both field .prev and
:set_prev() method are implemented as ffi functions. Also note that
cycles are now allowed while organizing errors into lists:
e1 -> e2 -> e3; e3:set_prev(e1) -- would lead to error.

Part of #1148
---
 extra/exports                   |   2 +
 src/box/key_list.c              |  16 +--
 src/box/lua/call.c              |   6 +-
 src/lib/core/diag.c             |  51 +++++++++
 src/lib/core/diag.h             |  95 +++++++++++++++-
 src/lua/error.lua               |  40 +++++++
 test/box/misc.result            | 233 ++++++++++++++++++++++++++++++++++++++++
 test/box/misc.test.lua          |  89 +++++++++++++++
 test/engine/func_index.result   |  50 +++++++--
 test/engine/func_index.test.lua |   7 ++
 10 files changed, 568 insertions(+), 21 deletions(-)

diff --git a/extra/exports b/extra/exports
index 7b84a1452..94cbdd210 100644
--- a/extra/exports
+++ b/extra/exports
@@ -246,6 +246,8 @@ clock_monotonic64
 clock_process64
 clock_thread64
 string_strip_helper
+error_prev
+error_set_prev
 
 # Lua / LuaJIT
 
diff --git a/src/box/key_list.c b/src/box/key_list.c
index 3d736b55f..a766ce0ec 100644
--- a/src/box/key_list.c
+++ b/src/box/key_list.c
@@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
 	if (rc != 0) {
 		/* Can't evaluate function. */
 		struct space *space = space_by_id(index_def->space_id);
-		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
-			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
+			 space != NULL ? space_name(space) : "",
+			 "can't evaluate function");
 		return -1;
 	}
 	uint32_t key_data_sz;
@@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
 	if (key_data == NULL) {
 		struct space *space = space_by_id(index_def->space_id);
 		/* Can't get a result returned by function . */
-		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
-			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
+			 space != NULL ? space_name(space) : "",
+			 "can't get a value returned by function");
 		return -1;
 	}
 
@@ -170,9 +170,9 @@ key_list_iterator_next(struct key_list_iterator *it, const char **value)
 		 * The key doesn't follow functional index key
 		 * definition.
 		 */
-		diag_set(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
+		diag_add(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
 			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+			 "key does not follow functional index definition");
 		return -1;
 	}
 
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index f1bbde7f0..5d3579eff 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -687,9 +687,9 @@ func_persistent_lua_load(struct func_lua *func)
 	if (func->base.def->is_sandboxed) {
 		if (prepare_lua_sandbox(tarantool_L, default_sandbox_exports,
 					nelem(default_sandbox_exports)) != 0) {
-			diag_set(ClientError, ER_LOAD_FUNCTION,
-				func->base.def->name,
-				diag_last_error(diag_get())->errmsg);
+			diag_add(ClientError, ER_LOAD_FUNCTION,
+				 func->base.def->name,
+				 "can't prepare a Lua sandbox");
 			goto end;
 		}
 	} else {
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index c350abb4a..1776dc8cf 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -34,6 +34,55 @@
 /* Must be set by the library user */
 struct error_factory *error_factory = NULL;
 
+struct error *
+error_prev(struct error *e)
+{
+	assert(e != NULL);
+	return e->next;
+}
+
+int
+error_set_prev(struct error *e, struct error *prev)
+{
+	/*
+	 * Make sure that adding error won't result in cycles.
+	 * Don't bother with sophisticated cycle-detection
+	 * algorithms, simple iteration is OK since as a rule
+	 * list contains a dozen errors at maximum.
+	 */
+	struct error *tmp = prev;
+	while (tmp != NULL) {
+		if (tmp == e)
+			return -1;
+		tmp = tmp->next;
+	}
+	/*
+	 * At once error can be reason for only one error.
+	 * So unlink previous 'prev' node.
+	 *
+	 * +--------+ NEXT +--------+
+	 * |    e   | ---> |old prev|
+	 * +--------+      +--------+
+	 *     ^               |
+	 *     |      PREV     |
+	 *     +-------X-------+
+	 *
+	 */
+	if (e->next != NULL)
+		e->next->prev = NULL;
+	/* Set new 'prev' node. */
+	e->next = prev;
+	/*
+	 * Unlink new 'prev' node from its old stack.
+	 * nil can be also passed as an argument.
+	 */
+	if (prev != NULL) {
+		error_unlink_tail(prev);
+		prev->prev = e;
+	}
+	return 0;
+}
+
 void
 error_create(struct error *e,
 	     error_f destroy, error_f raise, error_f log,
@@ -53,6 +102,8 @@ error_create(struct error *e,
 		e->line = 0;
 	}
 	e->errmsg[0] = '\0';
+	e->prev = NULL;
+	e->next = NULL;
 }
 
 struct diag *
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 7e1e1a174..5271733cb 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -37,6 +37,7 @@
 #include <stdbool.h>
 #include <assert.h>
 #include "say.h"
+#include "small/rlist.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -84,6 +85,17 @@ struct error {
 	char file[DIAG_FILENAME_MAX];
 	/* Error description. */
 	char errmsg[DIAG_ERRMSG_MAX];
+	/**
+	 * Link to the next and previous errors.
+	 * RLIST implementation is not really suitable here
+	 * since it is organized as circular list. In such
+	 * a case it is impossible to start an iteration
+	 * from any node and finish at the logical end of the
+	 * list. Double-linked list is required to allow deletion
+	 * from the middle of the list.
+	 */
+	struct error *next;
+	struct error *prev;
 };
 
 static inline void
@@ -92,15 +104,61 @@ error_ref(struct error *e)
 	e->refs++;
 }
 
+/**
+ * Unlink error from any error which point to it. For instance:
+ * e1 -> e2 -> e3 -> e4  (e1:set_prev(e2); e2:set_prev(33) ...)
+ * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
+ */
+static inline void
+error_unlink_tail(struct error *e)
+{
+	if (e->prev != NULL)
+		e->prev->next = NULL;
+	e->prev = NULL;
+}
+
+
 static inline void
 error_unref(struct error *e)
 {
 	assert(e->refs > 0);
 	--e->refs;
-	if (e->refs == 0)
+	if (e->refs == 0) {
+		/* Unlink error from the list completely. */
+		if (e->prev != NULL)
+			e->prev->next = e->next;
+		if (e->next != NULL)
+			e->next->prev = e->prev;
+		e->next = NULL;
+		e->prev = NULL;
 		e->destroy(e);
+	}
 }
 
+/**
+ * Return previous (for given error) error. Result can be NULL
+ * which means that there's no previous error. Simple getter
+ * to be used as ffi method in lua/error.lua.
+ */
+struct error *
+error_prev(struct error *e);
+
+/**
+ * Set previous error: remove @a prev from its current stack and
+ * link to the one @a e belongs to. Note that all previous errors
+ * starting from @a prev->next are transferred with it as well
+ * (i.e. reasons for given error are not erased). For instance:
+ * e1 -> e2 -> NULL; e3 -> e4 -> NULL;
+ * e2:set_prev(e3): e1 -> e2 -> e3 -> e4 -> NULL
+ *
+ * @a prev can be  NULL. To be used as ffi method in lua/error.lua.
+ *
+ * @retval -1 in case adding @a prev results in list cycles;
+ *          0 otherwise.
+ */
+int
+error_set_prev(struct error *e, struct error *prev);
+
 NORETURN static inline void
 error_raise(struct error *e)
 {
@@ -163,7 +221,12 @@ diag_clear(struct diag *diag)
 {
 	if (diag->last == NULL)
 		return;
-	error_unref(diag->last);
+	struct error *last = diag->last;
+	while (last != NULL) {
+		struct error *tmp = last->next;
+		error_unref(last);
+		last = tmp;
+	}
 	diag->last = NULL;
 }
 
@@ -178,6 +241,25 @@ diag_set_error(struct diag *diag, struct error *e)
 	assert(e != NULL);
 	error_ref(e);
 	diag_clear(diag);
+	error_unlink_tail(e);
+	diag->last = e;
+}
+
+/**
+ * Add a new error to the diagnostics area. It is added to the
+ * tail, so that list forms stack.
+ * @param diag Diagnostics area.
+ * @param e Error to be added.
+ */
+static inline void
+diag_add_error(struct diag *diag, struct error *e)
+{
+	assert(e != NULL);
+	error_ref(e);
+	error_unlink_tail(e);
+	e->next = diag->last;
+	if (diag->last != NULL)
+		diag->last->prev = e;
 	diag->last = e;
 }
 
@@ -280,6 +362,15 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
 	errno = save_errno;                                             \
 } while (0)
 
+#define diag_add(class, ...) do {					\
+	int save_errno = errno;						\
+	say_debug("%s at %s:%i", #class, __FILE__, __LINE__);		\
+	struct error *e;						\
+	e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__);		\
+	diag_add_error(diag_get(), e);					\
+	errno = save_errno;						\
+} while (0)
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 7f249864a..222b5e273 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -11,6 +11,11 @@ enum {
 
 typedef void (*error_f)(struct error *e);
 
+struct rlist {
+   struct rlist *prev;
+   struct rlist *next;
+};
+
 struct error {
     error_f _destroy;
     error_f _raise;
@@ -24,12 +29,20 @@ struct error {
     char _file[DIAG_FILENAME_MAX];
     /* Error description. */
     char _errmsg[DIAG_ERRMSG_MAX];
+    struct error *_next;
+    struct error *_prev;
 };
 
 char *
 exception_get_string(struct error *e, const struct method_info *method);
 int
 exception_get_int(struct error *e, const struct method_info *method);
+
+struct error *
+error_prev(struct error *e);
+
+int
+error_set_prev(struct error *e, struct error *prev);
 ]]
 
 local REFLECTION_CACHE = {}
@@ -95,11 +108,37 @@ local function error_errno(err)
     return e
 end
 
+local function error_prev(err)
+    local e = ffi.C.error_prev(err);
+    if e ~= nil then
+        return e
+    else
+        return nil
+    end
+end
+
+local function error_set_prev(err, prev)
+    -- First argument must be error.
+    if not ffi.istype('struct error', err) then
+        error("Usage: error1:set_prev(error2)")
+    end
+    -- Second argument must be error or nil.
+    if not ffi.istype('struct error', prev) and prev ~= nil then
+        error("Usage: error1:set_prev(error2)")
+    end
+    local ok = tonumber(ffi.C.error_set_prev(err, prev));
+    if ok ~= 0 then
+        error("Cycles are not allowed")
+    end
+
+end
+
 local error_fields = {
     ["type"]        = error_type;
     ["message"]     = error_message;
     ["trace"]       = error_trace;
     ["errno"]       = error_errno;
+    ["prev"]        = error_prev;
 }
 
 local function error_unpack(err)
@@ -143,6 +182,7 @@ local error_methods = {
     ["raise"] = error_raise;
     ["match"] = error_match; -- Tarantool 1.6 backward compatibility
     ["__serialize"] = error_serialize;
+    ["set_prev"] = error_set_prev;
 }
 
 local function error_index(err, key)
diff --git a/test/box/misc.result b/test/box/misc.result
index b0a81a055..4e4825a76 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -352,6 +352,239 @@ box.error.clear()
 box.error()
 ---
 ...
+-- gh-1148: erros can be arranged into list (so called
+-- stacked diagnostics).
+--
+e1 = box.error.new({code = 111, reason = "cause"})
+---
+...
+assert(e1.prev == nil)
+---
+- true
+...
+e1:set_prev(e1)
+---
+- error: 'builtin/error.lua: Cycles are not allowed'
+...
+assert(e1.prev == nil)
+---
+- true
+...
+e2 = box.error.new({code = 111, reason = "cause of cause"})
+---
+...
+e1:set_prev(e2)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+e2:set_prev(e1)
+---
+- error: 'builtin/error.lua: Cycles are not allowed'
+...
+assert(e2.prev == nil)
+---
+- true
+...
+-- At this point stack is following: e1 -> e2
+-- Let's test following cases:
+-- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
+-- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
+-- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
+-- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
+--
+e3 = box.error.new({code = 111, reason = "another cause"})
+---
+...
+e3:set_prev(e2)
+---
+...
+assert(e3.prev == e2)
+---
+- true
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e1.prev == nil)
+---
+- true
+...
+-- Reset stack to e1 -> e2 and test case 2.
+--
+e1:set_prev(e2)
+---
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e3.prev == nil)
+---
+- true
+...
+e1:set_prev(e3)
+---
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e1.prev == e3)
+---
+- true
+...
+assert(e3.prev == nil)
+---
+- true
+...
+-- Reset stack to e1 -> e2 and test case 3.
+--
+e1:set_prev(e2)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e3.prev == nil)
+---
+- true
+...
+e3:set_prev(e1)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e3.prev == e1)
+---
+- true
+...
+-- Unlink errors and test case 4.
+--
+e1:set_prev(nil)
+---
+...
+e2:set_prev(nil)
+---
+...
+e3:set_prev(nil)
+---
+...
+e1:set_prev(e2)
+---
+...
+e2:set_prev(e3)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == e3)
+---
+- true
+...
+assert(e3.prev == nil)
+---
+- true
+...
+-- Test circle detecting. At the moment stack is
+-- following: e1 -> e2 -> e3
+--
+e3:set_prev(e1)
+---
+- error: 'builtin/error.lua: Cycles are not allowed'
+...
+assert(e3.prev == nil)
+---
+- true
+...
+e3:set_prev(e2)
+---
+- error: 'builtin/error.lua: Cycles are not allowed'
+...
+assert(e3.prev == nil)
+---
+- true
+...
+-- Test splitting list into two ones.
+-- After that we will get two lists: e1->e2->e5 and e3->e4
+--
+e4 = box.error.new({code = 111, reason = "yet another cause"})
+---
+...
+e5 = box.error.new({code = 111, reason = "and another one"})
+---
+...
+e3:set_prev(e4)
+---
+...
+e2:set_prev(e5)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == e5)
+---
+- true
+...
+assert(e3.prev == e4)
+---
+- true
+...
+assert(e5.prev == nil)
+---
+- true
+...
+assert(e4.prev == nil)
+---
+- true
+...
+-- Another splitting option: e1->e2 and e5->e3->e4
+-- But firstly restore to one single list e1->e2->e3->e4
+--
+e2:set_prev(e3)
+---
+...
+e5:set_prev(e3)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e5.prev == e3)
+---
+- true
+...
+assert(e3.prev == e4)
+---
+- true
+...
+assert(e4.prev == nil)
+---
+- true
+...
 ----------------
 -- # box.stat
 ----------------
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index 0351317dd..bf429d18f 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -121,6 +121,95 @@ box.error.new(err)
 box.error.clear()
 box.error()
 
+-- gh-1148: erros can be arranged into list (so called
+-- stacked diagnostics).
+--
+e1 = box.error.new({code = 111, reason = "cause"})
+assert(e1.prev == nil)
+e1:set_prev(e1)
+assert(e1.prev == nil)
+e2 = box.error.new({code = 111, reason = "cause of cause"})
+e1:set_prev(e2)
+assert(e1.prev == e2)
+e2:set_prev(e1)
+assert(e2.prev == nil)
+-- At this point stack is following: e1 -> e2
+-- Let's test following cases:
+-- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
+-- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
+-- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
+-- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
+--
+e3 = box.error.new({code = 111, reason = "another cause"})
+e3:set_prev(e2)
+assert(e3.prev == e2)
+assert(e2.prev == nil)
+assert(e1.prev == nil)
+
+-- Reset stack to e1 -> e2 and test case 2.
+--
+e1:set_prev(e2)
+assert(e2.prev == nil)
+assert(e3.prev == nil)
+e1:set_prev(e3)
+assert(e2.prev == nil)
+assert(e1.prev == e3)
+assert(e3.prev == nil)
+
+-- Reset stack to e1 -> e2 and test case 3.
+--
+e1:set_prev(e2)
+assert(e1.prev == e2)
+assert(e2.prev == nil)
+assert(e3.prev == nil)
+e3:set_prev(e1)
+assert(e1.prev == e2)
+assert(e2.prev == nil)
+assert(e3.prev == e1)
+
+-- Unlink errors and test case 4.
+--
+e1:set_prev(nil)
+e2:set_prev(nil)
+e3:set_prev(nil)
+e1:set_prev(e2)
+e2:set_prev(e3)
+assert(e1.prev == e2)
+assert(e2.prev == e3)
+assert(e3.prev == nil)
+
+-- Test circle detecting. At the moment stack is
+-- following: e1 -> e2 -> e3
+--
+e3:set_prev(e1)
+assert(e3.prev == nil)
+e3:set_prev(e2)
+assert(e3.prev == nil)
+
+-- Test splitting list into two ones.
+-- After that we will get two lists: e1->e2->e5 and e3->e4
+--
+e4 = box.error.new({code = 111, reason = "yet another cause"})
+e5 = box.error.new({code = 111, reason = "and another one"})
+e3:set_prev(e4)
+e2:set_prev(e5)
+assert(e1.prev == e2)
+assert(e2.prev == e5)
+assert(e3.prev == e4)
+assert(e5.prev == nil)
+assert(e4.prev == nil)
+
+-- Another splitting option: e1->e2 and e5->e3->e4
+-- But firstly restore to one single list e1->e2->e3->e4
+--
+e2:set_prev(e3)
+e5:set_prev(e3)
+assert(e1.prev == e2)
+assert(e2.prev == nil)
+assert(e5.prev == e3)
+assert(e3.prev == e4)
+assert(e4.prev == nil)
+
 ----------------
 -- # box.stat
 ----------------
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index 84cb83022..8f92fcf11 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -5,6 +5,10 @@ test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
  | ---
  | ...
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+ | ---
+ | - true
+ | ...
 
 --
 -- gh-1260: Func index.
@@ -158,8 +162,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn1.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -197,8 +200,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -217,8 +219,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn4.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -264,8 +265,41 @@ idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'stri
 s:insert({1})
  | ---
  | - error: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
- |     [string "return function(tuple)                 local ..."]:1: attempt to call
- |     global ''require'' (a nil value)'
+ |     can''t evaluate function'
+ | ...
+e = box.error.last()
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 198
+ |   trace:
+ |   - file: <filename>
+ |     line: 68
+ |   type: ClientError
+ |   message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ |     can''t evaluate function'
+ |   prev: '[string "return function(tuple)                 local ..."]:1: attempt to
+ |     call global ''require'' (a nil value)'
+ | ...
+e = e.prev
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - type: LuajitError
+ |   message: '[string "return function(tuple)                 local ..."]:1: attempt
+ |     to call global ''require'' (a nil value)'
+ |   trace:
+ |   - file: <filename>
+ |     line: 1028
+ | ...
+e = e.prev
+ | ---
+ | ...
+e == nil
+ | ---
+ | - true
  | ...
 idx:drop()
  | ---
diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua
index f31162c97..0e4043260 100644
--- a/test/engine/func_index.test.lua
+++ b/test/engine/func_index.test.lua
@@ -1,5 +1,6 @@
 test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
 
 --
 -- gh-1260: Func index.
@@ -99,6 +100,12 @@ test_run:cmd("setopt delimiter ''");
 box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true})
 idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}})
 s:insert({1})
+e = box.error.last()
+e:unpack()
+e = e.prev
+e:unpack()
+e = e.prev
+e == nil
 idx:drop()
 
 -- Remove old persistent functions
-- 
2.15.1

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Tarantool-patches] [PATCH 5/7] box/error: clarify purpose of reference counting in struct error
  2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
                   ` (3 preceding siblings ...)
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area Nikita Pettik
@ 2020-02-19 14:16 ` Nikita Pettik
  2020-02-23 17:43   ` Vladislav Shpilevoy
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream Nikita Pettik
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:16 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy

---
 src/lib/core/diag.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 5271733cb..a9181c522 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -72,6 +72,15 @@ struct error {
 	error_f raise;
 	error_f log;
 	const struct type_info *type;
+	/**
+	 * Reference counting is basically required since
+	 * instances of this structure are available in Lua
+	 * as well (as cdata with overloaded fields and methods
+	 * by the means of introspection). Thus, it may turn
+	 * out that Lua's GC attempts at releasing object
+	 * meanwhile it is still used in C internals or vice
+	 * versa. For details see luaT_pusherror().
+	 */
 	int refs;
 	/**
 	 * Errno at the moment of the error
-- 
2.15.1

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream
  2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
                   ` (4 preceding siblings ...)
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 5/7] box/error: clarify purpose of reference counting in struct error Nikita Pettik
@ 2020-02-19 14:16 ` Nikita Pettik
  2020-02-23 17:44   ` Vladislav Shpilevoy
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area Nikita Pettik
  2020-02-22 17:18 ` [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Vladislav Shpilevoy
  7 siblings, 1 reply; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:16 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy

From: Kirill Shcherbatov <kshcherbatov@tarantool.org>

Refactor iproto_reply_error and iproto_write_error with a new
mpstream-based helper mpstream_iproto_encode_error that encodes
error object for iproto protocol on a given stream object.
Previously each routine implemented an own error encoding, but
with the increasing complexity of encode operation with following
patches we need a uniform way to do it.

The iproto_write_error routine starts using region location
to use region-based mpstream. It is not a problem itself, because
errors reporting is not really performance-critical path.

Needed for #1148
---
 src/box/xrow.c | 72 +++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 21 deletions(-)

diff --git a/src/box/xrow.c b/src/box/xrow.c
index 968c3a202..3f1c90c87 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -36,12 +36,14 @@
 #include "third_party/base64.h"
 
 #include "fiber.h"
+#include "reflection.h"
 #include "version.h"
 #include "tt_static.h"
 #include "error.h"
 #include "vclock.h"
 #include "scramble.h"
 #include "iproto_constants.h"
+#include "mpstream.h"
 
 static_assert(IPROTO_DATA < 0x7f && IPROTO_METADATA < 0x7f &&
 	      IPROTO_SQL_INFO < 0x7f, "encoded IPROTO_BODY keys must fit into "\
@@ -381,10 +383,6 @@ static const struct iproto_body_bin iproto_body_bin = {
 	0x81, IPROTO_DATA, 0xdd, 0
 };
 
-static const struct iproto_body_bin iproto_error_bin = {
-	0x81, IPROTO_ERROR, 0xdb, 0
-};
-
 /** Return a 4-byte numeric error code, with status flags. */
 static inline uint32_t
 iproto_encode_error(uint32_t error)
@@ -478,46 +476,78 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot,
 	return 0;
 }
 
+static void
+mpstream_error_handler(void *error_ctx)
+{
+	*(bool *)error_ctx = true;
+}
+
+static void
+mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
+{
+	mpstream_encode_map(stream, 2);
+	mpstream_encode_uint(stream, IPROTO_ERROR);
+	mpstream_encode_str(stream, error->errmsg);
+}
+
 int
 iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
 		   uint32_t schema_version)
 {
-	uint32_t msg_len = strlen(e->errmsg);
-	uint32_t errcode = box_error_code(e);
-
-	struct iproto_body_bin body = iproto_error_bin;
 	char *header = (char *)obuf_alloc(out, IPROTO_HEADER_LEN);
 	if (header == NULL)
 		return -1;
 
+	/* The obuf-based stream has reserved area for header. */
+	bool is_error = false;
+	struct mpstream stream;
+	mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
+		      mpstream_error_handler, &is_error);
+
+	uint32_t used = obuf_size(out);
+	mpstream_iproto_encode_error(&stream, e);
+	mpstream_flush(&stream);
+
+	uint32_t errcode = box_error_code(e);
 	iproto_header_encode(header, iproto_encode_error(errcode), sync,
-			     schema_version, sizeof(body) + msg_len);
-	body.v_data_len = mp_bswap_u32(msg_len);
+			     schema_version, obuf_size(out) - used);
+
 	/* Malformed packet appears to be a lesser evil than abort. */
-	return obuf_dup(out, &body, sizeof(body)) != sizeof(body) ||
-	       obuf_dup(out, e->errmsg, msg_len) != msg_len ? -1 : 0;
+	return is_error;
 }
 
 void
 iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
 		   uint64_t sync)
 {
-	uint32_t msg_len = strlen(e->errmsg);
-	uint32_t errcode = box_error_code(e);
+	bool is_error = false;
+	struct mpstream stream;
+	struct region *region = &fiber()->gc;
+	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
+		      mpstream_error_handler, &is_error);
+
+	size_t region_svp = region_used(region);
+	mpstream_iproto_encode_error(&stream, e);
+	mpstream_flush(&stream);
+	if (is_error)
+		goto cleanup;
+
+	size_t payload_size = region_used(region) - region_svp;
+	char *payload = region_join(region, payload_size);
+	if (payload == NULL)
+		goto cleanup;
 
+	uint32_t errcode = box_error_code(e);
 	char header[IPROTO_HEADER_LEN];
-	struct iproto_body_bin body = iproto_error_bin;
-
 	iproto_header_encode(header, iproto_encode_error(errcode), sync,
-			     schema_version, sizeof(body) + msg_len);
-
-	body.v_data_len = mp_bswap_u32(msg_len);
+			     schema_version, payload_size);
 
 	ssize_t unused;
 	unused = write(fd, header, sizeof(header));
-	unused = write(fd, &body, sizeof(body));
-	unused = write(fd, e->errmsg, msg_len);
+	unused = write(fd, payload, payload_size);
 	(void) unused;
+cleanup:
+	region_truncate(region, region_svp);
 }
 
 int
-- 
2.15.1

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area
  2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
                   ` (5 preceding siblings ...)
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream Nikita Pettik
@ 2020-02-19 14:16 ` Nikita Pettik
  2020-02-23 17:43   ` Vladislav Shpilevoy
  2020-02-22 17:18 ` [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Vladislav Shpilevoy
  7 siblings, 1 reply; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:16 UTC (permalink / raw)
  To: tarantool-patches; +Cc: v.shpilevoy

This patch introduces support of stacked errors in IProto protocol and
in net.box module.

@TarantoolBot document
Title: Stacked error diagnostic area

Starting from now errors can be organized into lists. To achieve this
Lua table representing error object is extended with .prev field and
e:set_prev(err) method. .prev field returns previous error if any exist.
e:set_prev(err) method expects err to be error object or nil and sets
err as previous error of e. For instance:

e1 = box.error.new({code = 111, reason = "cause"})
e2 = box.error.new({code = 111, reason = "cause of cause"})

e1:set_prev(e2)
assert(e1.prev == e2) -- true

Cycles are not allowed for error lists:

e2:set_prev(e1)
- error: 'builtin/error.lua: Cycles are not allowed'

Nil is valid input to :set_prev() method:

e1:set_prev(nil)
assert(e1.prev == nil) -- true

Note that error can be 'previous' only to the one error at once:

e1:set_prev(e2)
e3:set_prev(e2)
assert(e1.prev == nil) -- true
assert(e3.prev == e2) -- true

Setting previous error does not erase its own previous members:

-- e1 -> e2 -> e3 -> e4
e1:set_prev(e2)
e2:set_prev(e3)
e3:set_prev(e4)
e2:set_prev(e5)
-- Now there are two lists: e1->e2->e5 and e3->e4
assert(e1.prev == e2) -- true
assert(e2.prev == e5) -- true
assert(e3.prev == e4) -- true

Alternatively:

e1:set_prev(e2)
e2:set_prev(e3)
e3:set_prev(e4)
e5:set_prev(e3)
-- Now there are two lists: e1->e2 and e5->e3->e4
assert(e1.prev == e2) -- true
assert(e2.prev == nil) -- true
assert(e5.prev == e3) -- true
assert(e3.prev == e4) -- true

Stacked diagnostics is also supported by IProto protocol. Now responses
containing errors always (even there's only one error to be returned)
include new IProto key: IPROTO_ERROR_STACK (0x51). So, body corresponding to
error response now looks like:
MAP{IPROTO_ERROR : string, IPROTO_ERROR_STACK : ARRAY[MAP{ERROR_CODE : uint, ERROR_MESSAGE : string}, MAP{...}, ...]}

where IPROTO_ERROR is 0x31 key, IPROTO_ERROR_STACK is 0x51, ERROR_CODE
is 0x01 and ERROR_MESSAGE is 0x02.
Instances of older versions (without support of stacked errors in
protocol) simply ignore unknown keys and still rely only on IPROTO_ERROR
key.

Closes #1148
---
 src/box/error.cc           |  17 +++++
 src/box/error.h            |  16 +++++
 src/box/iproto_constants.h |   6 ++
 src/box/lua/net_box.lua    |  32 ++++++++-
 src/box/xrow.c             |  78 +++++++++++++++++++--
 test/box-py/iproto.result  |   6 +-
 test/box-py/iproto.test.py |   6 +-
 test/box/iproto.result     | 166 +++++++++++++++++++++++++++++++++++++++++++++
 test/box/iproto.test.lua   |  73 ++++++++++++++++++++
 test/box/net.box.result    |  65 ++++++++++++++++++
 test/box/net.box.test.lua  |  25 +++++++
 11 files changed, 475 insertions(+), 15 deletions(-)
 create mode 100644 test/box/iproto.result
 create mode 100644 test/box/iproto.test.lua

diff --git a/src/box/error.cc b/src/box/error.cc
index 824a4617c..e3197c8e6 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -102,6 +102,23 @@ box_error_construct(const char *file, unsigned line, uint32_t code,
 	return e;
 }
 
+int
+box_error_add(const char *file, unsigned line, uint32_t code,
+	      const char *fmt, ...)
+{
+	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
+	ClientError *client_error = type_cast(ClientError, e);
+	if (client_error) {
+		client_error->m_errcode = code;
+		va_list ap;
+		va_start(ap, fmt);
+		error_vformat_msg(e, fmt, ap);
+		va_end(ap);
+	}
+	diag_add_error(&fiber()->diag, e);
+	return -1;
+}
+
 /* }}} */
 
 struct rmean *rmean_error = NULL;
diff --git a/src/box/error.h b/src/box/error.h
index 42043ef80..626701f27 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -137,6 +137,22 @@ struct error *
 box_error_construct(const char *file, unsigned line, uint32_t code,
 		    const char *fmt, ...);
 
+/**
+ * Add error to the diagnostic area. In contrast to box_error_set()
+ * it does not replace previous error being set, but rather link
+ * them into list.
+ *
+ * \param code IPROTO error code (enum \link box_error_code \endlink)
+ * \param format (const char * ) - printf()-like format string
+ * \param ... - format arguments
+ * \returns -1 for convention use
+ *
+ * \sa enum box_error_code
+ */
+int
+box_error_add(const char *file, unsigned line, uint32_t code,
+	      const char *fmt, ...);
+
 /**
  * A backward-compatible API define.
  */
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index b66c05c06..a77660018 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -125,6 +125,7 @@ enum iproto_key {
 	IPROTO_STMT_ID = 0x43,
 	/* Leave a gap between SQL keys and additional request keys */
 	IPROTO_REPLICA_ANON = 0x50,
+	IPROTO_ERROR_STACK = 0x51,
 	IPROTO_KEY_MAX
 };
 
@@ -149,6 +150,11 @@ enum iproto_ballot_key {
 	IPROTO_BALLOT_IS_LOADING = 0x04,
 };
 
+enum iproto_error_key {
+	IPROTO_ERROR_CODE = 0x01,
+	IPROTO_ERROR_MESSAGE = 0x02,
+};
+
 #define bit(c) (1ULL<<IPROTO_##c)
 
 #define IPROTO_HEAD_BMAP (bit(REQUEST_TYPE) | bit(SYNC) | bit(REPLICA_ID) |\
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index b4811edfa..9a619e3d4 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -44,6 +44,9 @@ local SQL_INFO_ROW_COUNT_KEY = 0
 local IPROTO_FIELD_NAME_KEY = 0
 local IPROTO_DATA_KEY      = 0x30
 local IPROTO_ERROR_KEY     = 0x31
+local IPROTO_ERROR_STACK   = 0x51
+local IPROTO_ERROR_CODE    = 0x01
+local IPROTO_ERROR_MESSAGE = 0x02
 local IPROTO_GREETING_SIZE = 128
 local IPROTO_CHUNK_KEY     = 128
 local IPROTO_OK_KEY        = 0
@@ -277,8 +280,24 @@ local function create_transport(host, port, user, password, callback,
     --
     function request_index:result()
         if self.errno then
-            return nil, box.error.new({code = self.errno,
-                                       reason = self.response})
+            if type(self.response) == 'table' then
+                -- Decode and fill in error stack.
+                local prev = nil
+                for i = #self.response, 1, -1 do
+                    local error = self.response[i]
+                    local code = error[IPROTO_ERROR_CODE]
+                    local msg = error[IPROTO_ERROR_MESSAGE]
+                    assert(type(code) == 'number')
+                    assert(type(msg) == 'string')
+                    local new_err = box.error.new({code = code, reason = msg})
+                    new_err:set_prev(prev)
+                    prev = new_err
+                end
+                return nil, prev
+            else
+                return nil, box.error.new({code = self.errno,
+                                           reason = self.response})
+            end
         elseif not self.id then
             return self.response
         elseif not worker_fiber then
@@ -559,7 +578,14 @@ local function create_transport(host, port, user, password, callback,
             body, body_end_check = decode(body_rpos)
             assert(body_end == body_end_check, "invalid xrow length")
             request.errno = band(status, IPROTO_ERRNO_MASK)
-            request.response = body[IPROTO_ERROR_KEY]
+            -- IPROTO_ERROR_STACK comprises error encoded with
+            -- IPROTO_ERROR_KEY, so we may ignore content of that key.
+            if body[IPROTO_ERROR_STACK] ~= nil then
+                request.response = body[IPROTO_ERROR_STACK]
+                assert(type(request.response) == 'table')
+            else
+                request.response = body[IPROTO_ERROR_KEY]
+            end
             request.cond:broadcast()
             return
         end
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 3f1c90c87..b8c78cbc5 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -488,6 +488,19 @@ mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
 	mpstream_encode_map(stream, 2);
 	mpstream_encode_uint(stream, IPROTO_ERROR);
 	mpstream_encode_str(stream, error->errmsg);
+
+	uint32_t err_cnt = 0;
+	for (const struct error *it = error; it != NULL; it = it->next)
+		err_cnt++;
+	mpstream_encode_uint(stream, IPROTO_ERROR_STACK);
+	mpstream_encode_array(stream, err_cnt);
+	for (const struct error *it = error; it != NULL; it = it->next) {
+		mpstream_encode_map(stream, 2);
+		mpstream_encode_uint(stream, IPROTO_ERROR_CODE);
+		mpstream_encode_uint(stream, box_error_code(it));
+		mpstream_encode_uint(stream, IPROTO_ERROR_MESSAGE);
+		mpstream_encode_str(stream, it->errmsg);
+	}
 }
 
 int
@@ -1072,6 +1085,48 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len,
 	return 0;
 }
 
+static int
+iproto_decode_error_stack(const char **pos)
+{
+	char *reason = tt_static_buf();
+	static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\
+		      "expected to be larger than error message max length");
+	/*
+	 * Erase previously set diag errors. It is required since
+	 * box_error_add() does not replace previous errors.
+	 */
+	box_error_clear();
+	uint32_t stack_sz = mp_decode_array(pos);
+	for (uint32_t i = 0; i < stack_sz; i++) {
+		uint32_t code = 0;
+		if (mp_typeof(**pos) != MP_MAP)
+			return -1;
+		uint32_t map_sz = mp_decode_map(pos);
+		for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
+			if (mp_typeof(**pos) != MP_UINT)
+				return -1;
+			uint8_t key = mp_decode_uint(pos);
+			if (key == IPROTO_ERROR_CODE) {
+				if (mp_typeof(**pos) != MP_UINT)
+					return -1;
+				code = mp_decode_uint(pos);
+			} else if (key == IPROTO_ERROR_MESSAGE) {
+				if (mp_typeof(**pos) != MP_STR)
+					return -1;
+				uint32_t len;
+				const char *str = mp_decode_str(pos, &len);
+				snprintf(reason, TT_STATIC_BUF_LEN, "%.*s",
+					 len, str);
+			} else {
+				mp_next(pos);
+				continue;
+			}
+			box_error_add(__FILE__, __LINE__, code, reason);
+		}
+	}
+	return 0;
+}
+
 void
 xrow_decode_error(struct xrow_header *row)
 {
@@ -1098,15 +1153,26 @@ xrow_decode_error(struct xrow_header *row)
 			continue;
 		}
 		uint8_t key = mp_decode_uint(&pos);
-		if (key != IPROTO_ERROR || mp_typeof(*pos) != MP_STR) {
-			mp_next(&pos); /* value */
+		if (key == IPROTO_ERROR && mp_typeof(*pos) == MP_STR) {
+			/*
+			 * Obsolete way of sending error responses.
+			 * To be deprecated but still should be supported
+			 * to not break backward compatibility.
+			 */
+			uint32_t len;
+			const char *str = mp_decode_str(&pos, &len);
+			snprintf(error, sizeof(error), "%.*s", len, str);
+			box_error_set(__FILE__, __LINE__, code, error);
+		} else if (key == IPROTO_ERROR_STACK &&
+			   mp_typeof(*pos) == MP_ARRAY) {
+			if (iproto_decode_error_stack(&pos) != 0)
+				continue;
+		} else {
+			mp_next(&pos);
 			continue;
 		}
-
-		uint32_t len;
-		const char *str = mp_decode_str(&pos, &len);
-		snprintf(error, sizeof(error), "%.*s", len, str);
 	}
+	return;
 
 error:
 	box_error_set(__FILE__, __LINE__, code, error);
diff --git a/test/box-py/iproto.result b/test/box-py/iproto.result
index 900e6e24f..04e2e220c 100644
--- a/test/box-py/iproto.result
+++ b/test/box-py/iproto.result
@@ -169,9 +169,9 @@ box.schema.user.revoke('guest', 'read,write,execute', 'universe')
 # Test bugs gh-272, gh-1654 if the packet was incorrect, respond with
 # an error code and do not close connection
 
-sync=0, {49: 'Invalid MsgPack - packet header'}
-sync=1234, {49: "Missing mandatory field 'space id' in request"}
-sync=5678, {49: "Read access to space '_user' is denied for user 'guest'"}
+sync=0, Invalid MsgPack - packet header
+sync=1234, Missing mandatory field 'space id' in request
+sync=5678, Read access to space '_user' is denied for user 'guest'
 space = box.schema.space.create('test_index_base', { id = 568 })
 ---
 ...
diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py
index 77637d8ed..cdd1a71c5 100644
--- a/test/box-py/iproto.test.py
+++ b/test/box-py/iproto.test.py
@@ -355,15 +355,15 @@ s = c._socket
 header = { "hello": "world"}
 body = { "bug": 272 }
 resp = test_request(header, body)
-print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'])
+print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR))
 header = { IPROTO_CODE : REQUEST_TYPE_SELECT }
 header[IPROTO_SYNC] = 1234
 resp = test_request(header, body)
-print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'])
+print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR))
 header[IPROTO_SYNC] = 5678
 body = { IPROTO_SPACE_ID: 304, IPROTO_KEY: [], IPROTO_LIMIT: 1 }
 resp = test_request(header, body)
-print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'])
+print 'sync=%d, %s' % (resp['header'][IPROTO_SYNC], resp['body'].get(IPROTO_ERROR))
 c.close()
 
 
diff --git a/test/box/iproto.result b/test/box/iproto.result
new file mode 100644
index 000000000..28b8bf140
--- /dev/null
+++ b/test/box/iproto.result
@@ -0,0 +1,166 @@
+test_run = require('test_run').new()
+---
+...
+net_box = require('net.box')
+---
+...
+urilib = require('uri')
+---
+...
+msgpack = require('msgpack')
+---
+...
+IPROTO_REQUEST_TYPE   = 0x00
+---
+...
+IPROTO_INSERT         = 0x02
+---
+...
+IPROTO_SYNC           = 0x01
+---
+...
+IPROTO_SPACE_ID       = 0x10
+---
+...
+IPROTO_TUPLE          = 0x21
+---
+...
+IPROTO_ERROR          = 0x31
+---
+...
+IPROTO_ERROR_STACK    = 0x51
+---
+...
+IPROTO_ERROR_CODE     = 0x01
+---
+...
+IPROTO_ERROR_MESSAGE  = 0x02
+---
+...
+IPROTO_OK             = 0x00
+---
+...
+IPROTO_SCHEMA_VERSION = 0x05
+---
+...
+IPROTO_STATUS_KEY     = 0x00
+---
+...
+-- gh-1148: test capabilities of stacked diagnostics bypassing net.box.
+--
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
+test_run:cmd("setopt delimiter ''");
+---
+...
+box.schema.func.create('f1', {body = lua_func, is_deterministic = true, is_sandboxed = true})
+---
+...
+s = box.schema.space.create('s')
+---
+...
+pk = s:create_index('pk')
+---
+...
+idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
+---
+...
+box.schema.user.grant('guest', 'read, write, execute', 'universe')
+---
+...
+next_request_id = 16
+---
+...
+header = msgpack.encode({ \
+    [IPROTO_REQUEST_TYPE] = IPROTO_INSERT, \
+    [IPROTO_SYNC]         = next_request_id, \
+})
+---
+...
+body = msgpack.encode({ \
+    [IPROTO_SPACE_ID] = s.id, \
+    [IPROTO_TUPLE]    = box.tuple.new({1}) \
+})
+---
+...
+uri = urilib.parse(box.cfg.listen)
+---
+...
+sock = net_box.establish_connection(uri.host, uri.service)
+---
+...
+-- Send request.
+--
+size = msgpack.encode(header:len() + body:len())
+---
+...
+sock:write(size .. header .. body)
+---
+- 14
+...
+-- Read responce.
+--
+size = msgpack.decode(sock:read(5))
+---
+...
+header_body = sock:read(size)
+---
+...
+header, header_len = msgpack.decode(header_body)
+---
+...
+body = msgpack.decode(header_body:sub(header_len))
+---
+...
+sock:close()
+---
+- true
+...
+-- Both keys (obsolete and stack ones) are present in response.
+--
+assert(body[IPROTO_ERROR_STACK] ~= nil)
+---
+- true
+...
+assert(body[IPROTO_ERROR] ~= nil)
+---
+- true
+...
+err = body[IPROTO_ERROR_STACK][1]
+---
+...
+assert(err[IPROTO_ERROR_MESSAGE] == body[IPROTO_ERROR])
+---
+- true
+...
+err = body[IPROTO_ERROR_STACK][2]
+---
+...
+assert(err[IPROTO_ERROR_CODE] ~= nil)
+---
+- true
+...
+assert(type(err[IPROTO_ERROR_CODE]) == 'number')
+---
+- true
+...
+assert(err[IPROTO_ERROR_MESSAGE] ~= nil)
+---
+- true
+...
+assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string')
+---
+- true
+...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+---
+...
+s:drop()
+---
+...
+box.func.f1:drop()
+---
+...
diff --git a/test/box/iproto.test.lua b/test/box/iproto.test.lua
new file mode 100644
index 000000000..0425d0a21
--- /dev/null
+++ b/test/box/iproto.test.lua
@@ -0,0 +1,73 @@
+test_run = require('test_run').new()
+net_box = require('net.box')
+urilib = require('uri')
+msgpack = require('msgpack')
+
+IPROTO_REQUEST_TYPE   = 0x00
+IPROTO_INSERT         = 0x02
+IPROTO_SYNC           = 0x01
+IPROTO_SPACE_ID       = 0x10
+IPROTO_TUPLE          = 0x21
+IPROTO_ERROR          = 0x31
+IPROTO_ERROR_STACK    = 0x51
+IPROTO_ERROR_CODE     = 0x01
+IPROTO_ERROR_MESSAGE  = 0x02
+IPROTO_OK             = 0x00
+IPROTO_SCHEMA_VERSION = 0x05
+IPROTO_STATUS_KEY     = 0x00
+
+-- gh-1148: test capabilities of stacked diagnostics bypassing net.box.
+--
+test_run:cmd("setopt delimiter ';'")
+lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
+test_run:cmd("setopt delimiter ''");
+
+box.schema.func.create('f1', {body = lua_func, is_deterministic = true, is_sandboxed = true})
+s = box.schema.space.create('s')
+pk = s:create_index('pk')
+idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
+
+box.schema.user.grant('guest', 'read, write, execute', 'universe')
+
+next_request_id = 16
+header = msgpack.encode({ \
+    [IPROTO_REQUEST_TYPE] = IPROTO_INSERT, \
+    [IPROTO_SYNC]         = next_request_id, \
+})
+
+body = msgpack.encode({ \
+    [IPROTO_SPACE_ID] = s.id, \
+    [IPROTO_TUPLE]    = box.tuple.new({1}) \
+})
+
+uri = urilib.parse(box.cfg.listen)
+sock = net_box.establish_connection(uri.host, uri.service)
+
+-- Send request.
+--
+size = msgpack.encode(header:len() + body:len())
+sock:write(size .. header .. body)
+-- Read responce.
+--
+size = msgpack.decode(sock:read(5))
+header_body = sock:read(size)
+header, header_len = msgpack.decode(header_body)
+body = msgpack.decode(header_body:sub(header_len))
+sock:close()
+
+-- Both keys (obsolete and stack ones) are present in response.
+--
+assert(body[IPROTO_ERROR_STACK] ~= nil)
+assert(body[IPROTO_ERROR] ~= nil)
+
+err = body[IPROTO_ERROR_STACK][1]
+assert(err[IPROTO_ERROR_MESSAGE] == body[IPROTO_ERROR])
+err = body[IPROTO_ERROR_STACK][2]
+assert(err[IPROTO_ERROR_CODE] ~= nil)
+assert(type(err[IPROTO_ERROR_CODE]) == 'number')
+assert(err[IPROTO_ERROR_MESSAGE] ~= nil)
+assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string')
+
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+s:drop()
+box.func.f1:drop()
diff --git a/test/box/net.box.result b/test/box/net.box.result
index e3dabf7d9..c5d5d3743 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3902,6 +3902,71 @@ sock:close()
 ---
 - true
 ...
+-- gh-1148: test stacked diagnostics.
+--
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+---
+- true
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
+test_run:cmd("setopt delimiter ''");
+---
+...
+box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+---
+...
+s = box.schema.space.create('s')
+---
+...
+pk = s:create_index('pk')
+---
+...
+idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
+---
+...
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+---
+...
+c = net.connect(box.cfg.listen)
+---
+...
+f = function(...) return c.space.s:insert(...) end
+---
+...
+_, e = pcall(f, {1})
+---
+...
+assert(e ~= nil)
+---
+- true
+...
+e:unpack().message
+---
+- 'Failed to build a key for functional index ''idx'' of space ''s'': can''t evaluate
+  function'
+...
+e.prev:unpack().message
+---
+- '[string "return function(tuple) local json = require(''..."]:1: attempt to call
+  global ''require'' (a nil value)'
+...
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+---
+...
+s:drop()
+---
+...
+box.func.f1:drop()
+---
+...
+test_run:cmd("clear filter")
+---
+- true
+...
 test_run:wait_log('default', 'Got a corrupted row.*', nil, 10)
 ---
 - 'Got a corrupted row:'
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 8e65ff470..d459293df 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1574,6 +1574,31 @@ data = string.fromhex('3C'..string.rep(require('digest').sha1_hex('bcde'), 3))
 sock:write(data)
 sock:close()
 
+-- gh-1148: test stacked diagnostics.
+--
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+test_run:cmd("setopt delimiter ';'")
+lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
+test_run:cmd("setopt delimiter ''");
+box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
+s = box.schema.space.create('s')
+pk = s:create_index('pk')
+idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
+
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+c = net.connect(box.cfg.listen)
+f = function(...) return c.space.s:insert(...) end
+_, e = pcall(f, {1})
+assert(e ~= nil)
+
+e:unpack().message
+e.prev:unpack().message
+
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+s:drop()
+box.func.f1:drop()
+test_run:cmd("clear filter")
+
 test_run:wait_log('default', 'Got a corrupted row.*', nil, 10)
 test_run:wait_log('default', '00000000:.*', nil, 10)
 test_run:wait_log('default', '00000010:.*', nil, 10)
-- 
2.15.1

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/7] box/error: introduce box.error.set() method
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 2/7] box/error: introduce box.error.set() method Nikita Pettik
@ 2020-02-19 14:26   ` Cyrill Gorcunov
  2020-02-19 14:30     ` Nikita Pettik
  0 siblings, 1 reply; 27+ messages in thread
From: Cyrill Gorcunov @ 2020-02-19 14:26 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches, v.shpilevoy

On Wed, Feb 19, 2020 at 05:16:51PM +0300, Nikita Pettik wrote:
> +static int
> +luaT_error_set(lua_State *L)
> +{
> +	if (lua_gettop(L) == 0)
> +		return luaL_error(L, "Usage: box.error.set(error)");
> +	struct error *e = luaL_checkerror(L, 1);
> +	assert(e != NULL);
> +	diag_set_error(&fiber()->diag, e);
> +	return 0;
> +}

diag_set_error already has assert(e != NULL), maybe we could omit this one?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/7] box/error: introduce box.error.set() method
  2020-02-19 14:26   ` Cyrill Gorcunov
@ 2020-02-19 14:30     ` Nikita Pettik
  2020-02-19 14:53       ` Cyrill Gorcunov
  0 siblings, 1 reply; 27+ messages in thread
From: Nikita Pettik @ 2020-02-19 14:30 UTC (permalink / raw)
  To: Cyrill Gorcunov; +Cc: tarantool-patches, v.shpilevoy

On 19 Feb 17:26, Cyrill Gorcunov wrote:
> On Wed, Feb 19, 2020 at 05:16:51PM +0300, Nikita Pettik wrote:
> > +static int
> > +luaT_error_set(lua_State *L)
> > +{
> > +	if (lua_gettop(L) == 0)
> > +		return luaL_error(L, "Usage: box.error.set(error)");
> > +	struct error *e = luaL_checkerror(L, 1);
> > +	assert(e != NULL);
> > +	diag_set_error(&fiber()->diag, e);
> > +	return 0;
> > +}
> 
> diag_set_error already has assert(e != NULL), maybe we could omit this one?

Yep, you are right, this assertion is likely to be redundant. Thanks,
will drop it while preparing next patch-set version.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/7] box/error: introduce box.error.set() method
  2020-02-19 14:30     ` Nikita Pettik
@ 2020-02-19 14:53       ` Cyrill Gorcunov
  0 siblings, 0 replies; 27+ messages in thread
From: Cyrill Gorcunov @ 2020-02-19 14:53 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches, v.shpilevoy

On Wed, Feb 19, 2020 at 05:30:51PM +0300, Nikita Pettik wrote:
> > 
> > diag_set_error already has assert(e != NULL), maybe we could omit this one?
> 
> Yep, you are right, this assertion is likely to be redundant. Thanks,
> will drop it while preparing next patch-set version.

I think we could clean it on top later :)

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area Nikita Pettik
@ 2020-02-19 21:10   ` Vladislav Shpilevoy
  2020-02-20 11:53     ` Nikita Pettik
  2020-02-23 17:43   ` Vladislav Shpilevoy
  1 sibling, 1 reply; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-19 21:10 UTC (permalink / raw)
  To: Nikita Pettik, tarantool-patches



On 19/02/2020 15:16, Nikita Pettik wrote:
> In terms of implementation, now struct error objects can be organized
> into double-linked lists. To achieve this pointers to the next and
> previous elements have been added to struct error. It is worth
> mentioning that already existing rlist and stailq list implementations
> are not suitable: rlist is cycled list, as a result it is impossible to
> start iteration over the list from random list entry and finish it at
> the logical end of the list; stailq is single-linked list leaving no
> possibility to remove elements from the middle of the list.
> 
> As a part of C interface, box_error_add() has been introduced. In
> contrast to box_error_set() it does not replace last raised error, but
> instead it adds error to the list of diagnostic errors having already
> been set. If error is to be deleted (its reference counter hits 0 value)
> it is unlinked from the list it belongs to and destroyed.
> 
> To organize errors into lists in Lua, table representing error object in
> Lua now has .prev field (corresponding to 'previous' error) and method
> :set_prev(e). The latter accepts error object (i.e. created via
> box.error.new() or box.error.last()) and nil value. Both field .prev and
> :set_prev() method are implemented as ffi functions. Also note that
> cycles are now allowed while organizing errors into lists:
> e1 -> e2 -> e3; e3:set_prev(e1) -- would lead to error.
> 
> Part of #1148
> ---
>  extra/exports                   |   2 +
>  src/box/key_list.c              |  16 +--
>  src/box/lua/call.c              |   6 +-
>  src/lib/core/diag.c             |  51 +++++++++
>  src/lib/core/diag.h             |  95 +++++++++++++++-
>  src/lua/error.lua               |  40 +++++++
>  test/box/misc.result            | 233 ++++++++++++++++++++++++++++++++++++++++
>  test/box/misc.test.lua          |  89 +++++++++++++++
>  test/engine/func_index.result   |  50 +++++++--
>  test/engine/func_index.test.lua |   7 ++
>  10 files changed, 568 insertions(+), 21 deletions(-)
> 
> diff --git a/extra/exports b/extra/exports
> index 7b84a1452..94cbdd210 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -246,6 +246,8 @@ clock_monotonic64
>  clock_process64
>  clock_thread64
>  string_strip_helper
> +error_prev
> +error_set_prev
>  
>  # Lua / LuaJIT
>  
> diff --git a/src/box/key_list.c b/src/box/key_list.c
> index 3d736b55f..a766ce0ec 100644
> --- a/src/box/key_list.c
> +++ b/src/box/key_list.c
> @@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  	if (rc != 0) {
>  		/* Can't evaluate function. */
>  		struct space *space = space_by_id(index_def->space_id);
> -		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> -			 space ? space_name(space) : "",
> -			 diag_last_error(diag_get())->errmsg);
> +		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> +			 space != NULL ? space_name(space) : "",
> +			 "can't evaluate function");
>  		return -1;
>  	}
>  	uint32_t key_data_sz;
> @@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  	if (key_data == NULL) {
>  		struct space *space = space_by_id(index_def->space_id);
>  		/* Can't get a result returned by function . */
> -		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> -			 space ? space_name(space) : "",
> -			 diag_last_error(diag_get())->errmsg);
> +		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> +			 space != NULL ? space_name(space) : "",
> +			 "can't get a value returned by function");
>  		return -1;
>  	}
>  
> @@ -170,9 +170,9 @@ key_list_iterator_next(struct key_list_iterator *it, const char **value)
>  		 * The key doesn't follow functional index key
>  		 * definition.
>  		 */
> -		diag_set(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
> +		diag_add(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
>  			 space ? space_name(space) : "",
> -			 diag_last_error(diag_get())->errmsg);
> +			 "key does not follow functional index definition");
>  		return -1;
>  	}
>  
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index f1bbde7f0..5d3579eff 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -687,9 +687,9 @@ func_persistent_lua_load(struct func_lua *func)
>  	if (func->base.def->is_sandboxed) {
>  		if (prepare_lua_sandbox(tarantool_L, default_sandbox_exports,
>  					nelem(default_sandbox_exports)) != 0) {
> -			diag_set(ClientError, ER_LOAD_FUNCTION,
> -				func->base.def->name,
> -				diag_last_error(diag_get())->errmsg);
> +			diag_add(ClientError, ER_LOAD_FUNCTION,
> +				 func->base.def->name,
> +				 "can't prepare a Lua sandbox");
>  			goto end;
>  		}
>  	} else {
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index c350abb4a..1776dc8cf 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -34,6 +34,55 @@
>  /* Must be set by the library user */
>  struct error_factory *error_factory = NULL;
>  
> +struct error *
> +error_prev(struct error *e)
> +{
> +	assert(e != NULL);
> +	return e->next;
> +}
> +
> +int
> +error_set_prev(struct error *e, struct error *prev)
> +{
> +	/*
> +	 * Make sure that adding error won't result in cycles.
> +	 * Don't bother with sophisticated cycle-detection
> +	 * algorithms, simple iteration is OK since as a rule
> +	 * list contains a dozen errors at maximum.
> +	 */
> +	struct error *tmp = prev;
> +	while (tmp != NULL) {
> +		if (tmp == e)
> +			return -1;
> +		tmp = tmp->next;
> +	}
> +	/*
> +	 * At once error can be reason for only one error.
> +	 * So unlink previous 'prev' node.
> +	 *
> +	 * +--------+ NEXT +--------+
> +	 * |    e   | ---> |old prev|
> +	 * +--------+      +--------+
> +	 *     ^               |
> +	 *     |      PREV     |
> +	 *     +-------X-------+
> +	 *
> +	 */
> +	if (e->next != NULL)
> +		e->next->prev = NULL;
> +	/* Set new 'prev' node. */
> +	e->next = prev;
> +	/*
> +	 * Unlink new 'prev' node from its old stack.
> +	 * nil can be also passed as an argument.
> +	 */
> +	if (prev != NULL) {
> +		error_unlink_tail(prev);
> +		prev->prev = e;
> +	}
> +	return 0;
> +}
> +
>  void
>  error_create(struct error *e,
>  	     error_f destroy, error_f raise, error_f log,
> @@ -53,6 +102,8 @@ error_create(struct error *e,
>  		e->line = 0;
>  	}
>  	e->errmsg[0] = '\0';
> +	e->prev = NULL;
> +	e->next = NULL;
>  }
>  
>  struct diag *
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 7e1e1a174..5271733cb 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -37,6 +37,7 @@
>  #include <stdbool.h>
>  #include <assert.h>
>  #include "say.h"
> +#include "small/rlist.h"
>  
>  #if defined(__cplusplus)
>  extern "C" {
> @@ -84,6 +85,17 @@ struct error {
>  	char file[DIAG_FILENAME_MAX];
>  	/* Error description. */
>  	char errmsg[DIAG_ERRMSG_MAX];
> +	/**
> +	 * Link to the next and previous errors.
> +	 * RLIST implementation is not really suitable here
> +	 * since it is organized as circular list. In such
> +	 * a case it is impossible to start an iteration
> +	 * from any node and finish at the logical end of the
> +	 * list. Double-linked list is required to allow deletion
> +	 * from the middle of the list.
> +	 */
> +	struct error *next;
> +	struct error *prev;
>  };
>  
>  static inline void
> @@ -92,15 +104,61 @@ error_ref(struct error *e)
>  	e->refs++;
>  }
>  
> +/**
> + * Unlink error from any error which point to it. For instance:
> + * e1 -> e2 -> e3 -> e4  (e1:set_prev(e2); e2:set_prev(33) ...)
> + * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
> + */
> +static inline void
> +error_unlink_tail(struct error *e)
> +{
> +	if (e->prev != NULL)
> +		e->prev->next = NULL;
> +	e->prev = NULL;
> +}
> +
> +
>  static inline void
>  error_unref(struct error *e)
>  {
>  	assert(e->refs > 0);
>  	--e->refs;
> -	if (e->refs == 0)
> +	if (e->refs == 0) {
> +		/* Unlink error from the list completely. */
> +		if (e->prev != NULL)
> +			e->prev->next = e->next;
> +		if (e->next != NULL)
> +			e->next->prev = e->prev;
> +		e->next = NULL;
> +		e->prev = NULL;
>  		e->destroy(e);
> +	}
>  }
>  
> +/**
> + * Return previous (for given error) error. Result can be NULL
> + * which means that there's no previous error. Simple getter
> + * to be used as ffi method in lua/error.lua.
> + */
> +struct error *
> +error_prev(struct error *e);
> +
> +/**
> + * Set previous error: remove @a prev from its current stack and
> + * link to the one @a e belongs to. Note that all previous errors
> + * starting from @a prev->next are transferred with it as well
> + * (i.e. reasons for given error are not erased). For instance:
> + * e1 -> e2 -> NULL; e3 -> e4 -> NULL;
> + * e2:set_prev(e3): e1 -> e2 -> e3 -> e4 -> NULL
> + *
> + * @a prev can be  NULL. To be used as ffi method in lua/error.lua.
> + *
> + * @retval -1 in case adding @a prev results in list cycles;
> + *          0 otherwise.
> + */
> +int
> +error_set_prev(struct error *e, struct error *prev);
> +
>  NORETURN static inline void
>  error_raise(struct error *e)
>  {
> @@ -163,7 +221,12 @@ diag_clear(struct diag *diag)
>  {
>  	if (diag->last == NULL)
>  		return;
> -	error_unref(diag->last);
> +	struct error *last = diag->last;
> +	while (last != NULL) {
> +		struct error *tmp = last->next;
> +		error_unref(last);
> +		last = tmp;
> +	}
>  	diag->last = NULL;

Hi! Please, read what I wrote in the ticket about box.error.new(). You
should not clear all the errors. The diag owns only the head ref. Head
destruction should unref a next error. Destruction of a next error
should unref next next error and so on.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area
  2020-02-19 21:10   ` Vladislav Shpilevoy
@ 2020-02-20 11:53     ` Nikita Pettik
  2020-02-20 18:29       ` Nikita Pettik
  0 siblings, 1 reply; 27+ messages in thread
From: Nikita Pettik @ 2020-02-20 11:53 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On 19 Feb 22:10, Vladislav Shpilevoy wrote:
> 
> 
> On 19/02/2020 15:16, Nikita Pettik wrote:
> > @@ -163,7 +221,12 @@ diag_clear(struct diag *diag)
> >  {
> >  	if (diag->last == NULL)
> >  		return;
> > -	error_unref(diag->last);
> > +	struct error *last = diag->last;
> > +	while (last != NULL) {
> > +		struct error *tmp = last->next;
> > +		error_unref(last);
> > +		last = tmp;
> > +	}
> >  	diag->last = NULL;
> 
> Hi! Please, read what I wrote in the ticket about box.error.new(). You
> should not clear all the errors. The diag owns only the head ref. Head
> destruction should unref a next error. Destruction of a next error
> should unref next next error and so on.

Hi,

I've realized that current approach is a bit broken only by now.
You are right: diag_clear() should unref only head  meanwhile error
destruction should result in decrementing reference counter of previous
error. Just for the record why my implementation doesn't work correct:

e1 = box.error.new(...) -- e1 has 1 ref
e2 = box.error.new(...) -- e2 has 1 ref

e1:set_prev(e2)
box.error.set(e1) -- e1 has 2 ref as a head of diag, e2 still has 1 ref
box.error.clear() -- e1 has 1 ref, but e2 has 0 so it is destroyed.
-- However, it still has Lua reference, so when GC will attempt to
-- destroy it again, it will lead to crash.

Will fix it soon, re-send patch and update rfc.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area
  2020-02-20 11:53     ` Nikita Pettik
@ 2020-02-20 18:29       ` Nikita Pettik
  0 siblings, 0 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-02-20 18:29 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On 20 Feb 14:53, Nikita Pettik wrote:
> On 19 Feb 22:10, Vladislav Shpilevoy wrote:
> > 
> > 
> > On 19/02/2020 15:16, Nikita Pettik wrote:
> > > @@ -163,7 +221,12 @@ diag_clear(struct diag *diag)
> > >  {
> > >  	if (diag->last == NULL)
> > >  		return;
> > > -	error_unref(diag->last);
> > > +	struct error *last = diag->last;
> > > +	while (last != NULL) {
> > > +		struct error *tmp = last->next;
> > > +		error_unref(last);
> > > +		last = tmp;
> > > +	}
> > >  	diag->last = NULL;
> > 
> > Hi! Please, read what I wrote in the ticket about box.error.new(). You
> > should not clear all the errors. The diag owns only the head ref. Head
> > destruction should unref a next error. Destruction of a next error
> > should unref next next error and so on.
> 
> Hi,
> 
> I've realized that current approach is a bit broken only by now.
> You are right: diag_clear() should unref only head  meanwhile error
> destruction should result in decrementing reference counter of previous
> error. Just for the record why my implementation doesn't work correct:
> 
> e1 = box.error.new(...) -- e1 has 1 ref
> e2 = box.error.new(...) -- e2 has 1 ref
> 
> e1:set_prev(e2)
> box.error.set(e1) -- e1 has 2 ref as a head of diag, e2 still has 1 ref
> box.error.clear() -- e1 has 1 ref, but e2 has 0 so it is destroyed.
> -- However, it still has Lua reference, so when GC will attempt to
> -- destroy it again, it will lead to crash.
> 
> Will fix it soon, re-send patch and update rfc.
>

Patch containing fixes:


commit 0e569dd23fabdd76889f9306425620ffbe7caccb
Author: Nikita Pettik <korablev@tarantool.org>
Date:   Tue Feb 18 17:30:03 2020 +0300

    box: introduce stacked diagnostic area
    
    In terms of implementation, now struct error objects can be organized
    into double-linked lists. To achieve this pointers to the next and
    previous elements have been added to struct error. It is worth
    mentioning that already existing rlist and stailq list implementations
    are not suitable: rlist is cycled list, as a result it is impossible to
    start iteration over the list from random list entry and finish it at
    the logical end of the list; stailq is single-linked list leaving no
    possibility to remove elements from the middle of the list.
    
    As a part of C interface, box_error_add() has been introduced. In
    contrast to box_error_set() it does not replace last raised error, but
    instead it adds error to the list of diagnostic errors having already
    been set. If error is to be deleted (its reference counter hits 0 value)
    it is unlinked from the list it belongs to and destroyed. Meanwhile,
    error destruction leads to decrement of reference counter of its
    previous error and so on.
    
    To organize errors into lists in Lua, table representing error object in
    Lua now has .prev field (corresponding to 'previous' error) and method
    :set_prev(e). The latter accepts error object (i.e. created via
    box.error.new() or box.error.last()) and nil value. Both field .prev and
    :set_prev() method are implemented as ffi functions. Also note that
    cycles are now allowed while organizing errors into lists:
    e1 -> e2 -> e3; e3:set_prev(e1) -- would lead to error.
    
    Part of #1148

diff --git a/extra/exports b/extra/exports
index 7b84a1452..94cbdd210 100644
--- a/extra/exports
+++ b/extra/exports
@@ -246,6 +246,8 @@ clock_monotonic64
 clock_process64
 clock_thread64
 string_strip_helper
+error_prev
+error_set_prev
 
 # Lua / LuaJIT
 
diff --git a/src/box/key_list.c b/src/box/key_list.c
index 3d736b55f..a766ce0ec 100644
--- a/src/box/key_list.c
+++ b/src/box/key_list.c
@@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
 	if (rc != 0) {
 		/* Can't evaluate function. */
 		struct space *space = space_by_id(index_def->space_id);
-		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
-			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
+			 space != NULL ? space_name(space) : "",
+			 "can't evaluate function");
 		return -1;
 	}
 	uint32_t key_data_sz;
@@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
 	if (key_data == NULL) {
 		struct space *space = space_by_id(index_def->space_id);
 		/* Can't get a result returned by function . */
-		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
-			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
+			 space != NULL ? space_name(space) : "",
+			 "can't get a value returned by function");
 		return -1;
 	}
 
@@ -170,9 +170,9 @@ key_list_iterator_next(struct key_list_iterator *it, const char **value)
 		 * The key doesn't follow functional index key
 		 * definition.
 		 */
-		diag_set(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
+		diag_add(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
 			 space ? space_name(space) : "",
-			 diag_last_error(diag_get())->errmsg);
+			 "key does not follow functional index definition");
 		return -1;
 	}
 
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index f1bbde7f0..5d3579eff 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -687,9 +687,9 @@ func_persistent_lua_load(struct func_lua *func)
 	if (func->base.def->is_sandboxed) {
 		if (prepare_lua_sandbox(tarantool_L, default_sandbox_exports,
 					nelem(default_sandbox_exports)) != 0) {
-			diag_set(ClientError, ER_LOAD_FUNCTION,
-				func->base.def->name,
-				diag_last_error(diag_get())->errmsg);
+			diag_add(ClientError, ER_LOAD_FUNCTION,
+				 func->base.def->name,
+				 "can't prepare a Lua sandbox");
 			goto end;
 		}
 	} else {
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index c350abb4a..17d3b0d7b 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -34,6 +34,56 @@
 /* Must be set by the library user */
 struct error_factory *error_factory = NULL;
 
+struct error *
+error_prev(struct error *e)
+{
+	assert(e != NULL);
+	return e->next;
+}
+
+int
+error_set_prev(struct error *e, struct error *prev)
+{
+	/*
+	 * Make sure that adding error won't result in cycles.
+	 * Don't bother with sophisticated cycle-detection
+	 * algorithms, simple iteration is OK since as a rule
+	 * list contains a dozen errors at maximum.
+	 */
+	struct error *tmp = prev;
+	while (tmp != NULL) {
+		if (tmp == e)
+			return -1;
+		tmp = tmp->next;
+	}
+	/*
+	 * At once error can be reason for only one error.
+	 * So unlink previous 'prev' node.
+	 *
+	 * +--------+ NEXT +--------+
+	 * |    e   | ---> |old prev|
+	 * +--------+      +--------+
+	 *     ^               |
+	 *     |      PREV     |
+	 *     +-------X-------+
+	 *
+	 */
+	if (e->next != NULL)
+		e->next->prev = NULL;
+	/* Set new 'prev' node. */
+	e->next = prev;
+	/*
+	 * Unlink new 'prev' node from its old stack.
+	 * nil can be also passed as an argument.
+	 */
+	if (prev != NULL) {
+		error_unlink_tail(prev);
+		prev->prev = e;
+		error_ref(prev);
+	}
+	return 0;
+}
+
 void
 error_create(struct error *e,
 	     error_f destroy, error_f raise, error_f log,
@@ -53,6 +103,8 @@ error_create(struct error *e,
 		e->line = 0;
 	}
 	e->errmsg[0] = '\0';
+	e->prev = NULL;
+	e->next = NULL;
 }
 
 struct diag *
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 7e1e1a174..fc7996c25 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -37,6 +37,7 @@
 #include <stdbool.h>
 #include <assert.h>
 #include "say.h"
+#include "small/rlist.h"
 
 #if defined(__cplusplus)
 extern "C" {
@@ -84,6 +85,17 @@ struct error {
 	char file[DIAG_FILENAME_MAX];
 	/* Error description. */
 	char errmsg[DIAG_ERRMSG_MAX];
+	/**
+	 * Link to the next and previous errors.
+	 * RLIST implementation is not really suitable here
+	 * since it is organized as circular list. In such
+	 * a case it is impossible to start an iteration
+	 * from any node and finish at the logical end of the
+	 * list. Double-linked list is required to allow deletion
+	 * from the middle of the list.
+	 */
+	struct error *next;
+	struct error *prev;
 };
 
 static inline void
@@ -97,10 +109,60 @@ error_unref(struct error *e)
 {
 	assert(e->refs > 0);
 	--e->refs;
-	if (e->refs == 0)
+	if (e->refs == 0) {
+		/* Unlink error from the list completely. */
+		if (e->prev != NULL)
+			e->prev->next = e->next;
+		if (e->next != NULL) {
+			e->next->prev = e->prev;
+			error_unref(e->next);
+		}
+		e->next = NULL;
+		e->prev = NULL;
 		e->destroy(e);
+	}
+}
+
+/**
+ * Unlink error from any error which point to it. For instance:
+ * e1 -> e2 -> e3 -> e4  (e1:set_prev(e2); e2:set_prev(33) ...)
+ * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
+ */
+static inline void
+error_unlink_tail(struct error *e)
+{
+	if (e->prev != NULL) {
+		assert(e->refs > 1);
+		error_unref(e);
+		e->prev->next = NULL;
+	}
+	e->prev = NULL;
 }
 
+/**
+ * Return previous (for given error) error. Result can be NULL
+ * which means that there's no previous error. Simple getter
+ * to be used as ffi method in lua/error.lua.
+ */
+struct error *
+error_prev(struct error *e);
+
+/**
+ * Set previous error: remove @a prev from its current stack and
+ * link to the one @a e belongs to. Note that all previous errors
+ * starting from @a prev->next are transferred with it as well
+ * (i.e. reasons for given error are not erased). For instance:
+ * e1 -> e2 -> NULL; e3 -> e4 -> NULL;
+ * e2:set_prev(e3): e1 -> e2 -> e3 -> e4 -> NULL
+ *
+ * @a prev can be  NULL. To be used as ffi method in lua/error.lua.
+ *
+ * @retval -1 in case adding @a prev results in list cycles;
+ *          0 otherwise.
+ */
+int
+error_set_prev(struct error *e, struct error *prev);
+
 NORETURN static inline void
 error_raise(struct error *e)
 {
@@ -178,6 +240,25 @@ diag_set_error(struct diag *diag, struct error *e)
 	assert(e != NULL);
 	error_ref(e);
 	diag_clear(diag);
+	error_unlink_tail(e);
+	diag->last = e;
+}
+
+/**
+ * Add a new error to the diagnostics area. It is added to the
+ * tail, so that list forms stack.
+ * @param diag Diagnostics area.
+ * @param e Error to be added.
+ */
+static inline void
+diag_add_error(struct diag *diag, struct error *e)
+{
+	assert(e != NULL);
+	error_ref(e);
+	error_unlink_tail(e);
+	e->next = diag->last;
+	if (diag->last != NULL)
+		diag->last->prev = e;
 	diag->last = e;
 }
 
@@ -280,6 +361,15 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
 	errno = save_errno;                                             \
 } while (0)
 
+#define diag_add(class, ...) do {					\
+	int save_errno = errno;						\
+	say_debug("%s at %s:%i", #class, __FILE__, __LINE__);		\
+	struct error *e;						\
+	e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__);		\
+	diag_add_error(diag_get(), e);					\
+	errno = save_errno;						\
+} while (0)
+
 #if defined(__cplusplus)
 } /* extern "C" */
 #endif /* defined(__cplusplus) */
diff --git a/src/lib/core/exception.cc b/src/lib/core/exception.cc
index 0ab10c4bd..180cb0e97 100644
--- a/src/lib/core/exception.cc
+++ b/src/lib/core/exception.cc
@@ -114,6 +114,7 @@ Exception::~Exception()
 	if (this != &out_of_memory) {
 		assert(refs == 0);
 	}
+	TRASH((struct error *) this);
 }
 
 Exception::Exception(const struct type_info *type_arg, const char *file,
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 7f249864a..222b5e273 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -11,6 +11,11 @@ enum {
 
 typedef void (*error_f)(struct error *e);
 
+struct rlist {
+   struct rlist *prev;
+   struct rlist *next;
+};
+
 struct error {
     error_f _destroy;
     error_f _raise;
@@ -24,12 +29,20 @@ struct error {
     char _file[DIAG_FILENAME_MAX];
     /* Error description. */
     char _errmsg[DIAG_ERRMSG_MAX];
+    struct error *_next;
+    struct error *_prev;
 };
 
 char *
 exception_get_string(struct error *e, const struct method_info *method);
 int
 exception_get_int(struct error *e, const struct method_info *method);
+
+struct error *
+error_prev(struct error *e);
+
+int
+error_set_prev(struct error *e, struct error *prev);
 ]]
 
 local REFLECTION_CACHE = {}
@@ -95,11 +108,37 @@ local function error_errno(err)
     return e
 end
 
+local function error_prev(err)
+    local e = ffi.C.error_prev(err);
+    if e ~= nil then
+        return e
+    else
+        return nil
+    end
+end
+
+local function error_set_prev(err, prev)
+    -- First argument must be error.
+    if not ffi.istype('struct error', err) then
+        error("Usage: error1:set_prev(error2)")
+    end
+    -- Second argument must be error or nil.
+    if not ffi.istype('struct error', prev) and prev ~= nil then
+        error("Usage: error1:set_prev(error2)")
+    end
+    local ok = tonumber(ffi.C.error_set_prev(err, prev));
+    if ok ~= 0 then
+        error("Cycles are not allowed")
+    end
+
+end
+
 local error_fields = {
     ["type"]        = error_type;
     ["message"]     = error_message;
     ["trace"]       = error_trace;
     ["errno"]       = error_errno;
+    ["prev"]        = error_prev;
 }
 
 local function error_unpack(err)
@@ -143,6 +182,7 @@ local error_methods = {
     ["raise"] = error_raise;
     ["match"] = error_match; -- Tarantool 1.6 backward compatibility
     ["__serialize"] = error_serialize;
+    ["set_prev"] = error_set_prev;
 }
 
 local function error_index(err, key)
diff --git a/test/box/misc.result b/test/box/misc.result
index b0a81a055..1e235e8a1 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -352,6 +352,282 @@ box.error.clear()
 box.error()
 ---
 ...
+-- gh-1148: erros can be arranged into list (so called
+-- stacked diagnostics).
+--
+e1 = box.error.new({code = 111, reason = "cause"})
+---
+...
+assert(e1.prev == nil)
+---
+- true
+...
+e1:set_prev(e1)
+---
+- error: 'builtin/error.lua: Cycles are not allowed'
+...
+assert(e1.prev == nil)
+---
+- true
+...
+e2 = box.error.new({code = 111, reason = "cause of cause"})
+---
+...
+e1:set_prev(e2)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+e2:set_prev(e1)
+---
+- error: 'builtin/error.lua: Cycles are not allowed'
+...
+assert(e2.prev == nil)
+---
+- true
+...
+-- At this point stack is following: e1 -> e2
+-- Let's test following cases:
+-- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
+-- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
+-- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
+-- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
+--
+e3 = box.error.new({code = 111, reason = "another cause"})
+---
+...
+e3:set_prev(e2)
+---
+...
+assert(e3.prev == e2)
+---
+- true
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e1.prev == nil)
+---
+- true
+...
+-- Reset stack to e1 -> e2 and test case 2.
+--
+e1:set_prev(e2)
+---
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e3.prev == nil)
+---
+- true
+...
+e1:set_prev(e3)
+---
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e1.prev == e3)
+---
+- true
+...
+assert(e3.prev == nil)
+---
+- true
+...
+-- Reset stack to e1 -> e2 and test case 3.
+--
+e1:set_prev(e2)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e3.prev == nil)
+---
+- true
+...
+e3:set_prev(e1)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e3.prev == e1)
+---
+- true
+...
+-- Unlink errors and test case 4.
+--
+e1:set_prev(nil)
+---
+...
+e2:set_prev(nil)
+---
+...
+e3:set_prev(nil)
+---
+...
+e1:set_prev(e2)
+---
+...
+e2:set_prev(e3)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == e3)
+---
+- true
+...
+assert(e3.prev == nil)
+---
+- true
+...
+-- Test circle detecting. At the moment stack is
+-- following: e1 -> e2 -> e3
+--
+e3:set_prev(e1)
+---
+- error: 'builtin/error.lua: Cycles are not allowed'
+...
+assert(e3.prev == nil)
+---
+- true
+...
+e3:set_prev(e2)
+---
+- error: 'builtin/error.lua: Cycles are not allowed'
+...
+assert(e3.prev == nil)
+---
+- true
+...
+-- Test splitting list into two ones.
+-- After that we will get two lists: e1->e2->e5 and e3->e4
+--
+e4 = box.error.new({code = 111, reason = "yet another cause"})
+---
+...
+e5 = box.error.new({code = 111, reason = "and another one"})
+---
+...
+e3:set_prev(e4)
+---
+...
+e2:set_prev(e5)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == e5)
+---
+- true
+...
+assert(e3.prev == e4)
+---
+- true
+...
+assert(e5.prev == nil)
+---
+- true
+...
+assert(e4.prev == nil)
+---
+- true
+...
+-- Another splitting option: e1->e2 and e5->e3->e4
+-- But firstly restore to one single list e1->e2->e3->e4
+--
+e2:set_prev(e3)
+---
+...
+e5:set_prev(e3)
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.prev == nil)
+---
+- true
+...
+assert(e5.prev == e3)
+---
+- true
+...
+assert(e3.prev == e4)
+---
+- true
+...
+assert(e4.prev == nil)
+---
+- true
+...
+-- In case error is destroyed, it unrefs reference counter
+-- of its previous error. In turn, box.error.clear() refs/unrefs
+-- only head and doesn't touch other errors.
+--
+e2:set_prev(nil)
+---
+...
+box.error.set(e1)
+---
+...
+assert(box.error.last() == e1)
+---
+- true
+...
+assert(box.error.last().prev == e2)
+---
+- true
+...
+box.error.clear()
+---
+...
+assert(box.error.last() == nil)
+---
+- true
+...
+assert(e1.prev == e2)
+---
+- true
+...
+assert(e2.code  == 111)
+---
+- true
+...
+box.error.set(e1)
+---
+...
+box.error.clear()
+---
+...
+assert(e1.prev == e2)
+---
+- true
+...
 ----------------
 -- # box.stat
 ----------------
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index 0351317dd..0ea5a6ae0 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -121,6 +121,111 @@ box.error.new(err)
 box.error.clear()
 box.error()
 
+-- gh-1148: erros can be arranged into list (so called
+-- stacked diagnostics).
+--
+e1 = box.error.new({code = 111, reason = "cause"})
+assert(e1.prev == nil)
+e1:set_prev(e1)
+assert(e1.prev == nil)
+e2 = box.error.new({code = 111, reason = "cause of cause"})
+e1:set_prev(e2)
+assert(e1.prev == e2)
+e2:set_prev(e1)
+assert(e2.prev == nil)
+-- At this point stack is following: e1 -> e2
+-- Let's test following cases:
+-- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
+-- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
+-- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
+-- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
+--
+e3 = box.error.new({code = 111, reason = "another cause"})
+e3:set_prev(e2)
+assert(e3.prev == e2)
+assert(e2.prev == nil)
+assert(e1.prev == nil)
+
+-- Reset stack to e1 -> e2 and test case 2.
+--
+e1:set_prev(e2)
+assert(e2.prev == nil)
+assert(e3.prev == nil)
+e1:set_prev(e3)
+assert(e2.prev == nil)
+assert(e1.prev == e3)
+assert(e3.prev == nil)
+
+-- Reset stack to e1 -> e2 and test case 3.
+--
+e1:set_prev(e2)
+assert(e1.prev == e2)
+assert(e2.prev == nil)
+assert(e3.prev == nil)
+e3:set_prev(e1)
+assert(e1.prev == e2)
+assert(e2.prev == nil)
+assert(e3.prev == e1)
+
+-- Unlink errors and test case 4.
+--
+e1:set_prev(nil)
+e2:set_prev(nil)
+e3:set_prev(nil)
+e1:set_prev(e2)
+e2:set_prev(e3)
+assert(e1.prev == e2)
+assert(e2.prev == e3)
+assert(e3.prev == nil)
+
+-- Test circle detecting. At the moment stack is
+-- following: e1 -> e2 -> e3
+--
+e3:set_prev(e1)
+assert(e3.prev == nil)
+e3:set_prev(e2)
+assert(e3.prev == nil)
+
+-- Test splitting list into two ones.
+-- After that we will get two lists: e1->e2->e5 and e3->e4
+--
+e4 = box.error.new({code = 111, reason = "yet another cause"})
+e5 = box.error.new({code = 111, reason = "and another one"})
+e3:set_prev(e4)
+e2:set_prev(e5)
+assert(e1.prev == e2)
+assert(e2.prev == e5)
+assert(e3.prev == e4)
+assert(e5.prev == nil)
+assert(e4.prev == nil)
+
+-- Another splitting option: e1->e2 and e5->e3->e4
+-- But firstly restore to one single list e1->e2->e3->e4
+--
+e2:set_prev(e3)
+e5:set_prev(e3)
+assert(e1.prev == e2)
+assert(e2.prev == nil)
+assert(e5.prev == e3)
+assert(e3.prev == e4)
+assert(e4.prev == nil)
+
+-- In case error is destroyed, it unrefs reference counter
+-- of its previous error. In turn, box.error.clear() refs/unrefs
+-- only head and doesn't touch other errors.
+--
+e2:set_prev(nil)
+box.error.set(e1)
+assert(box.error.last() == e1)
+assert(box.error.last().prev == e2)
+box.error.clear()
+assert(box.error.last() == nil)
+assert(e1.prev == e2)
+assert(e2.code  == 111)
+box.error.set(e1)
+box.error.clear()
+assert(e1.prev == e2)
+
 ----------------
 -- # box.stat
 ----------------
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index 84cb83022..8f92fcf11 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -5,6 +5,10 @@ test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
  | ---
  | ...
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
+ | ---
+ | - true
+ | ...
 
 --
 -- gh-1260: Func index.
@@ -158,8 +162,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn1.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -197,8 +200,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -217,8 +219,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn4.id, parts = {{1, 'un
 s:insert({1})
  | ---
  | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
- |     ''withdata'': Supplied key type of part 0 does not match index part type: expected
- |     unsigned'
+ |     ''withdata'': key does not follow functional index definition'
  | ...
 idx:drop()
  | ---
@@ -264,8 +265,41 @@ idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'stri
 s:insert({1})
  | ---
  | - error: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
- |     [string "return function(tuple)                 local ..."]:1: attempt to call
- |     global ''require'' (a nil value)'
+ |     can''t evaluate function'
+ | ...
+e = box.error.last()
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 198
+ |   trace:
+ |   - file: <filename>
+ |     line: 68
+ |   type: ClientError
+ |   message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ |     can''t evaluate function'
+ |   prev: '[string "return function(tuple)                 local ..."]:1: attempt to
+ |     call global ''require'' (a nil value)'
+ | ...
+e = e.prev
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - type: LuajitError
+ |   message: '[string "return function(tuple)                 local ..."]:1: attempt
+ |     to call global ''require'' (a nil value)'
+ |   trace:
+ |   - file: <filename>
+ |     line: 1028
+ | ...
+e = e.prev
+ | ---
+ | ...
+e == nil
+ | ---
+ | - true
  | ...
 idx:drop()
  | ---
diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua
index f31162c97..0e4043260 100644
--- a/test/engine/func_index.test.lua
+++ b/test/engine/func_index.test.lua
@@ -1,5 +1,6 @@
 test_run = require('test_run').new()
 engine = test_run:get_cfg('engine')
+test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
 
 --
 -- gh-1260: Func index.
@@ -99,6 +100,12 @@ test_run:cmd("setopt delimiter ''");
 box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true})
 idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}})
 s:insert({1})
+e = box.error.last()
+e:unpack()
+e = e.prev
+e:unpack()
+e = e.prev
+e == nil
 idx:drop()
 
 -- Remove old persistent functions

 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag Nikita Pettik
@ 2020-02-22 17:18   ` Vladislav Shpilevoy
  2020-03-25  1:02     ` Nikita Pettik
  0 siblings, 1 reply; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-22 17:18 UTC (permalink / raw)
  To: Nikita Pettik, tarantool-patches

Thanks for the patch!

See 6 comments below.

On 19/02/2020 15:16, Nikita Pettik wrote:
> To achieve this let's refactor luaT_error_create() to return error
> object instead of setting it via box_error_set().
> luaT_error_created() is used both to handle box.error() and

1. luaT_error_created() -> luaT_error_create().

> box.error.new() invocations, and box.error() is still expected to set
> error to diagnostic area. So, luaT_error_call() which implements
> box.error() processing at the end calls diag_set_error().
> It is worth mentioning that net.box module relied on the fact that
> box.error.new() set error to diagnostic area: otherwise request errors
> don't get to diagnostic area on client side.
> 
> @TarantoolBot document
> Title: Don't promote error created via box.error.new to diagnostic area
> 
> Now box.error.new() only creates error object, but doesn't set it to
> Tarantool's diagnostic area:
> box.error.clear()
> e = box.error.new({code = 111, reason = "cause"})
> assert(box.error.last() == nil)
> ---
> - true
> ...

2. It is better to wrap the code into ```, because otherwise it will
look like tickets created by Peter, with weird formatting, huge headers,
etc.

> To set error in diagnostic area explicitly box.error.set() has been
> introduced. It accepts error object which is set as last system error
> (i.e. becomes available via box.error.last()).
> Finally, box.error.new() does not longer accept error object as an
> argument (this was undocumented feature).
> Note that patch does not affect box.error(), which still pushed error to
> diagnostic area. This fact is reflected in docs:
> '''
> Emulate a request error, with text based on one of the pre-defined
> Tarantool errors...
> '''
> 
> Needed for #1148
> Closes #4778

3. This should be above docbot request. Otherwise it will appear in the
doc ticket.

> ---
>  src/box/error.cc       | 16 +++++++++++++++
>  src/box/error.h        |  8 ++++++++
>  src/box/lua/error.cc   | 56 ++++++++++++++++++++++++++++----------------------
>  test/box/misc.result   | 30 ++++++++++++++++++++++++++-
>  test/box/misc.test.lua | 16 +++++++++++++++
>  5 files changed, 101 insertions(+), 25 deletions(-)
> 
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 7dfe1b3ee..824a4617c 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -86,6 +86,22 @@ box_error_set(const char *file, unsigned line, uint32_t code,
>  	return -1;
>  }
>  
> +struct error *
> +box_error_construct(const char *file, unsigned line, uint32_t code,
> +		    const char *fmt, ...)
> +{
> +	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> +	ClientError *client_error = type_cast(ClientError, e);
> +	if (client_error != NULL) {
> +		client_error->m_errcode = code;

4. You can now make box_error_set() call box_error_construct() +
diag_set(), to avoid code duplication.

> +		va_list ap;
> +		va_start(ap, fmt);
> +		error_vformat_msg(e, fmt, ap);
> +		va_end(ap);
> +	}
> +	return e;
> +}
> +
>  /* }}} */
>  
>  struct rmean *rmean_error = NULL;
> diff --git a/src/box/error.h b/src/box/error.h
> index b8c7cf73d..42043ef80 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -129,6 +129,14 @@ int
>  box_error_set(const char *file, unsigned line, uint32_t code,
>  	      const char *format, ...);
>  
> +/**
> + * Construct error object without setting it in the diagnostics
> + * area. On the memory allocation fail returns NULL.
> + */
> +struct error *
> +box_error_construct(const char *file, unsigned line, uint32_t code,
> +		    const char *fmt, ...);

5. You added the method to the public C API (it is inside 'cond public'
comments). I don't think we should do that. At least until someone
asked so.

Also why not box_error_new()? 'Construct' seems to be a very strange name.

> +
>  /**
>   * A backward-compatible API define.
>   */
> diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
> index 7311cb2ce..17d4de653 100644
> --- a/src/box/lua/error.cc
> +++ b/src/box/lua/error.cc
> @@ -42,7 +42,14 @@ extern "C" {
>  #include "lua/utils.h"
>  #include "box/error.h"
>  
> -static void
> +/**
> + * Parse Lua arguments (they can come as single table
> + * f({code : number, reason : string}) or as separate members
> + * f(code, reason)) and construct struct error with given values.
> + * In case one of arguments is missing it corresponding field

6. it -> its.

> + * in struct error is filled with default value.
> + */
> +static struct error *
>  luaT_error_create(lua_State *L, int top_base)
>  {
>  	uint32_t code = 0;

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area
  2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
                   ` (6 preceding siblings ...)
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area Nikita Pettik
@ 2020-02-22 17:18 ` Vladislav Shpilevoy
  7 siblings, 0 replies; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-22 17:18 UTC (permalink / raw)
  To: Nikita Pettik, tarantool-patches

Hi! Thanks for the patchset!

Please, add a changelog record. I think it should be two separate
records, for each ticket.

On 19/02/2020 15:16, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-1148-stacked-diag
> Issue:
> https://github.com/tarantool/tarantool/issues/1148
> https://github.com/tarantool/tarantool/issues/4778
> 
> Patch-set basically consists of two parts: first one fixes undocumented
> behaviour of box.error.new() which sets created error to diagnostics
> area. The reason why it is required is described in the corresponding
> github ticked. Second part is about stacked diagnostics itself: patch
> error structure extending it with double linked list, provide Lua and
> C interfaces to interact with it, support stacked diagnostics in IProto
> protocol and net.box module (all points according to the rfc: 
> https://github.com/tarantool/tarantool/commit/1acd32d98f628431429b427df19caa9d269bb9c8).

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area Nikita Pettik
  2020-02-19 21:10   ` Vladislav Shpilevoy
@ 2020-02-23 17:43   ` Vladislav Shpilevoy
  2020-03-25  1:34     ` Nikita Pettik
  1 sibling, 1 reply; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-23 17:43 UTC (permalink / raw)
  To: Nikita Pettik, tarantool-patches

Hi! Thanks for the patch, welcome to the first review iteration!

Finally something good, not related to SQL/replication/application server
modules.

See 21 comments below tho.

>     box: introduce stacked diagnostic area
>     
>     In terms of implementation, now struct error objects can be organized
>     into double-linked lists. To achieve this pointers to the next and
>     previous elements have been added to struct error. It is worth
>     mentioning that already existing rlist and stailq list implementations
>     are not suitable: rlist is cycled list, as a result it is impossible to
>     start iteration over the list from random list entry and finish it at
>     the logical end of the list; stailq is single-linked list leaving no
>     possibility to remove elements from the middle of the list.
>     
>     As a part of C interface, box_error_add() has been introduced. In
>     contrast to box_error_set() it does not replace last raised error, but
>     instead it adds error to the list of diagnostic errors having already
>     been set. If error is to be deleted (its reference counter hits 0 value)
>     it is unlinked from the list it belongs to and destroyed. Meanwhile,
>     error destruction leads to decrement of reference counter of its
>     previous error and so on.
>     
>     To organize errors into lists in Lua, table representing error object in
>     Lua now has .prev field (corresponding to 'previous' error) and method
>     :set_prev(e). The latter accepts error object (i.e. created via
>     box.error.new() or box.error.last()) and nil value. Both field .prev and
>     :set_prev() method are implemented as ffi functions. Also note that
>     cycles are now allowed while organizing errors into lists:

1. Now -> not.

>     e1 -> e2 -> e3; e3:set_prev(e1) -- would lead to error.
>     
>     Part of #1148
> 
> diff --git a/extra/exports b/extra/exports
> index 7b84a1452..94cbdd210 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -246,6 +246,8 @@ clock_monotonic64
>  clock_process64
>  clock_thread64
>  string_strip_helper
> +error_prev
> +error_set_prev

2. I would move this to other exported error functions, above.

>  
>  # Lua / LuaJIT
>  
> diff --git a/src/box/key_list.c b/src/box/key_list.c
> index 3d736b55f..a766ce0ec 100644
> --- a/src/box/key_list.c
> +++ b/src/box/key_list.c
> @@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
>  	if (rc != 0) {
>  		/* Can't evaluate function. */
>  		struct space *space = space_by_id(index_def->space_id);
> -		diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> -			 space ? space_name(space) : "",
> -			 diag_last_error(diag_get())->errmsg);
> +		diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> +			 space != NULL ? space_name(space) : "",
> +			 "can't evaluate function");
>  		return -1;

3. I would split stack diag introduction and its appliance into
separate commits. But up to you, it is not so critical here.

>  	}
>  	uint32_t key_data_sz;
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index c350abb4a..17d3b0d7b 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -34,6 +34,56 @@
>  /* Must be set by the library user */
>  struct error_factory *error_factory = NULL;
>  
> +struct error *
> +error_prev(struct error *e)
> +{
> +	assert(e != NULL);
> +	return e->next;

4. error_prev() returns next? We should do something
with that. It is very counter-intuitive. Maybe just
change next <-> prev names in struct error.

Besides, you don't need this function, especially in
extern, because in Lua you can access ._next/._prev members
right away, transparently, like a table member, thanks to
FFI.

> +}
> +
> +int
> +error_set_prev(struct error *e, struct error *prev)
> +{
> +	/*
> +	 * Make sure that adding error won't result in cycles.
> +	 * Don't bother with sophisticated cycle-detection
> +	 * algorithms, simple iteration is OK since as a rule
> +	 * list contains a dozen errors at maximum.
> +	 */
> +	struct error *tmp = prev;
> +	while (tmp != NULL) {
> +		if (tmp == e)
> +			return -1;
> +		tmp = tmp->next;
> +	}

5. I would turn this into an assertion here, and extract into a
separate function, which would be called only when an error is
added from some public function like Lua API, or C public API.
Up to you, this also looks ok.

> +	/*
> +	 * At once error can be reason for only one error.
> +	 * So unlink previous 'prev' node.
> +	 *
> +	 * +--------+ NEXT +--------+
> +	 * |    e   | ---> |old prev|
> +	 * +--------+      +--------+
> +	 *     ^               |
> +	 *     |      PREV     |
> +	 *     +-------X-------+
> +	 *
> +	 */

6. Nice picture, but it confused me a little. I don't even know
why. Perhaps because it is really simple code, and the picture
made me think, that there is something non trivial. I would drop
it, up to you.

> +	if (e->next != NULL)
> +		e->next->prev = NULL;
> +	/* Set new 'prev' node. */
> +	e->next = prev;

7. You don't unref old e->next value. As a result, there is a leak
(maybe, if I correctly understand, that 'next' is the reference
counter keeper). A test:

	e1 = box.error.new({code = 0, reason = 'e1'})
	e2 = box.error.new({code = 0, reason = 'e2'})
	e1:set_prev(e2)

	e3 = box.error.new({code = 0, reason = 'e3'})
	e1:set_prev(e3)

I added printf() to error_create() and before a call of
error->destroy. Then I nullified the references, and called GC.
Only e1 and e3 were destroyed.

	e1 = nil
	e2 = nil
	e3 = nil
	collectgarbage('collect')

Output of all of this was:

	error created 0x102a01e68 # e1 created
	error created 0x102b93f68 # e2 created
	error created 0x102e2ab08 # e3 created
	error destroyed 0x102e2ab08 # e3 destroyed
	error destroyed 0x102a01e68 # e1 destroyed

The bug is easy to fix. But don't know how to test it. The trick
with setmetatable({}, {__mode = 'v'}) does not work, because Lua
object e2 is deleted fine. C object is still alive. Another
option - use error injections like I did with ERRINJ_DYN_MODULE_COUNT.

> +	/*
> +	 * Unlink new 'prev' node from its old stack.
> +	 * nil can be also passed as an argument.
> +	 */

8. I didn't understand the comment, because this code works
fine:

	e1 = box.error.new({code = 0, reason = 'e1'})
	e2 = box.error.new({code = 0, reason = 'e2'})
	e1:set_prev(e2)

	e3 = box.error.new({code = 0, reason = 'e3'})
	e4 = box.error.new({code = 0, reason = 'e4'})
	e3:set_prev(e4)

	e2:set_prev(e3)

From the comment it looks like e2:set_prev(e3) should
detach e3 from its old stack, so a result would look
like:

	e1->e2->e3
        e4

But really it looks like:

	e1->e2->e3->e4

And this is good. So the comment is misleading. You probably
meant unlink it from its old 'newer' errors.

> +	if (prev != NULL) {
> +		error_unlink_tail(prev);
> +		prev->prev = e;
> +		error_ref(prev);
> +	}
> +	return 0;
> +}
> +
>  void
>  error_create(struct error *e,
>  	     error_f destroy, error_f raise, error_f log,
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 7e1e1a174..fc7996c25 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -37,6 +37,7 @@
>  #include <stdbool.h>
>  #include <assert.h>
>  #include "say.h"
> +#include "small/rlist.h"

9. An artifact from the time when you wanted to use rlist?

>  #if defined(__cplusplus)
>  extern "C" {
> @@ -84,6 +85,17 @@ struct error {
>  	char file[DIAG_FILENAME_MAX];
>  	/* Error description. */
>  	char errmsg[DIAG_ERRMSG_MAX];
> +	/**
> +	 * Link to the next and previous errors.
> +	 * RLIST implementation is not really suitable here
> +	 * since it is organized as circular list. In such
> +	 * a case it is impossible to start an iteration
> +	 * from any node and finish at the logical end of the
> +	 * list. Double-linked list is required to allow deletion
> +	 * from the middle of the list.
> +	 */
> +	struct error *next;
> +	struct error *prev;

10. Please, add an example here, and say explicitly, which error
is newer, which is older. This would help a lot to read the
code.

Also we can make it more straight, and rename next->older, prev->newer.
Or rename next->reason, prev->conseq/outcome. Don't know. But currently
it is really hard to read, when you want to operate on a previous error,
but use the 'next' member. Even if we swap next and prev, it is still
unclear what is 'tail', what is 'reason'. I would go for reason and
conseq/outcome.

>  };
>  
>  static inline void
> @@ -97,10 +109,60 @@ error_unref(struct error *e)
>  {
>  	assert(e->refs > 0);
>  	--e->refs;
> -	if (e->refs == 0)
> +	if (e->refs == 0) {
> +		/* Unlink error from the list completely. */
> +		if (e->prev != NULL)
> +			e->prev->next = e->next;
> +		if (e->next != NULL) {
> +			e->next->prev = e->prev;
> +			error_unref(e->next);
> +		}

11. Only 'next' link keeps a reference, right? It is worth
commenting in struct error. And why too (I guess because otherwise
two errors linking each other as prev and next would never be
deleted).

> +		e->next = NULL;
> +		e->prev = NULL;
>  		e->destroy(e);
> +	}
> +}

12. I think you know it was coming :D

	head = box.error.new({code = 0, reason = 'r'})
	tail = head
	for i = 1, 100000 do
	    local next = box.error.new({code = 0, reason = 'r'})
	    tail:set_prev(next)
	    tail = next
	end
	tail = nil
	head = nil

Stupid recursion test.

    tarantool> collectgarbage('collect')
    Process 58860 stopped
    * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x106101ff8)
        frame #0: 0x00000001001772e3 tarantool`error_unref(e=0x0000000106ee6d78) at diag.h:118:4
       115              e->prev->next = e->next;
       116          if (e->next != NULL) {
       117              e->next->prev = e->prev;
    -> 118              error_unref(e->next);
       119          }
       120          e->next = NULL;
       121          e->prev = NULL;
    Target 0: (tarantool) stopped.

We need to either limit error stack size, or remove the recursion.
Recursion removal should be simple. First you iterate through the
list and check which errors are going to be deleted (have ref
count 1). When you see a ref count 2 first time, you stop. Now
you start their deletion from tail. So first you delete the error,
which has prev error with ref count 2. It won't start a recursion,
because prev error is not deleted. Then you delete next error. It
won't start recursion, because its prev error is already null. And
so on.

Example:

    errors: e1 -> e2 -> e3 -> e4 -> e5 ->
    refs:   1     1     1     1     2     ...

Assume you want to delete e1. Currently you delete it
as recursion e1 deletes e2 deletes e3 deletes e4. With
my algo above you find e4. Now you know e1 - e4 should
be deleted.

Then you go in a second while cycle from e4 back to e1.
Each deletion won't trigger a recursion, because prev error
always will be null or something with ref count 2.

Also you could do it in one cycle probably. Then you would
need a temporary ref. For example, when you delete e1, you
give ++ref to its prev error. After e1 is deleted, you check
whether --ref to the prev error would delete it too. If yes,
you repeat it again - ++ to prev prev error, delete prev,
check prev prev deletion, and so on. It also removes the
recursion.

Huge number of errors easily may happen, if a user had a bug
in his code about an infinite cycle adding errors, or something
like that.

Also huge number of errors may affect the whole tx thread
each time when we send an error to iproto, or delete it. Because
we will iterate without any yields in a huge list. Don't know
what to do with that. Probably nothing for now.

> +
> +/**
> + * Unlink error from any error which point to it. For instance:
> + * e1 -> e2 -> e3 -> e4  (e1:set_prev(e2); e2:set_prev(33) ...)

13. 33 -> e3.

> + * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
> + */
> +static inline void
> +error_unlink_tail(struct error *e)
> +{
> +	if (e->prev != NULL) {
> +		assert(e->refs > 1);
> +		error_unref(e);
> +		e->prev->next = NULL;
> +	}
> +	e->prev = NULL;
>  }
>  
> +/**
> + * Return previous (for given error) error. Result can be NULL
> + * which means that there's no previous error. Simple getter
> + * to be used as ffi method in lua/error.lua.
> + */
> +struct error *
> +error_prev(struct error *e);
> +
> +/**
> + * Set previous error: remove @a prev from its current stack and

14. As I see from the code - not from its stack, but cut its previous
'tail' of newer errors. Right?

> + * link to the one @a e belongs to. Note that all previous errors
> + * starting from @a prev->next are transferred with it as well
> + * (i.e. reasons for given error are not erased). For instance:
> + * e1 -> e2 -> NULL; e3 -> e4 -> NULL;
> + * e2:set_prev(e3): e1 -> e2 -> e3 -> e4 -> NULL
> + *
> + * @a prev can be  NULL. To be used as ffi method in lua/error.lua.
> + *
> + * @retval -1 in case adding @a prev results in list cycles;
> + *          0 otherwise.
> + */
> +int
> +error_set_prev(struct error *e, struct error *prev);
> +
>  NORETURN static inline void
>  error_raise(struct error *e)
>  {
> @@ -178,6 +240,25 @@ diag_set_error(struct diag *diag, struct error *e)
>  	assert(e != NULL);
>  	error_ref(e);
>  	diag_clear(diag);
> +	error_unlink_tail(e);
> +	diag->last = e;
> +}
> +
> +/**
> + * Add a new error to the diagnostics area. It is added to the
> + * tail, so that list forms stack.
> + * @param diag Diagnostics area.
> + * @param e Error to be added.
> + */
> +static inline void
> +diag_add_error(struct diag *diag, struct error *e)
> +{
> +	assert(e != NULL);
> +	error_ref(e);
> +	error_unlink_tail(e);
> +	e->next = diag->last;

15. I thought, that 'next' member is responsible for keeping a
reference, but since you don't ref 'diag->last' here, I am not
sure now. Could you please clarify?

> +	if (diag->last != NULL)
> +		diag->last->prev = e;
>  	diag->last = e;
>  }
>  
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 7f249864a..222b5e273 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -11,6 +11,11 @@ enum {
>  
>  typedef void (*error_f)(struct error *e);
>  
> +struct rlist {
> +   struct rlist *prev;
> +   struct rlist *next;
> +};

16. Nope.

> +
>  struct error {
>      error_f _destroy;
>      error_f _raise;
> @@ -95,11 +108,37 @@ local function error_errno(err)
>      return e
>  end
>  
> +local function error_prev(err)
> +    local e = ffi.C.error_prev(err);
> +    if e ~= nil then
> +        return e
> +    else
> +        return nil
> +    end

17. I said it above, but just in case repeat it here: you can
directly return err._next, because FFI allows to access struct
members in Lua. Strictly speaking you can do the same below
for set_prev, but it is not so trivial, and it is right to keep
it in C and extern.

> +end
> +
> +local function error_set_prev(err, prev)
> +    -- First argument must be error.
> +    if not ffi.istype('struct error', err) then
> +        error("Usage: error1:set_prev(error2)")
> +    end
> +    -- Second argument must be error or nil.
> +    if not ffi.istype('struct error', prev) and prev ~= nil then
> +        error("Usage: error1:set_prev(error2)")
> +    end
> +    local ok = tonumber(ffi.C.error_set_prev(err, prev));

18. Why do you need tonumber? 'int' should be fine to compare
with 0.

> +    if ok ~= 0 then
> +        error("Cycles are not allowed")
> +    end
> +
> +end
> +
> diff --git a/test/box/misc.result b/test/box/misc.result
> index b0a81a055..1e235e8a1 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -352,6 +352,282 @@ box.error.clear()
>  box.error()
>  ---
>  ...
> +-- gh-1148: erros can be arranged into list (so called

19. erros -> errors. If you work in Sublime, it has nice spell
checker.

20. I don't like that we have error module tests in misc.test.lua.
Especially when there is already so many of them. Could you please
move them all (including old tests) into a new file error.test.lua?

> +-- stacked diagnostics).
> +--
> +e1 = box.error.new({code = 111, reason = "cause"})
> +---
> +...
> +assert(e1.prev == nil)
> +---
> +- true
> +...
> +e1:set_prev(e1)
> +---
> +- error: 'builtin/error.lua: Cycles are not allowed'
> +...
> +assert(e1.prev == nil)
> +---
> +- true
> +...
> +e2 = box.error.new({code = 111, reason = "cause of cause"})
> +---
> +...
> +e1:set_prev(e2)
> +---
> +...
> +assert(e1.prev == e2)
> +---
> +- true
> +...
> +e2:set_prev(e1)
> +---
> +- error: 'builtin/error.lua: Cycles are not allowed'
> +...
> +assert(e2.prev == nil)
> +---
> +- true
> +...
> +-- At this point stack is following: e1 -> e2
> +-- Let's test following cases:
> +-- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
> +-- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
> +-- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
> +-- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
> +--
> +e3 = box.error.new({code = 111, reason = "another cause"})
> +---
> +...
> +e3:set_prev(e2)
> +---
> +...
> +assert(e3.prev == e2)

21. It is not really necessary to use assertion here. It will fail
without assertion too in case of a problem. Assertion is needed,
when you want an exception. For example, when you call a function,
inside which you do tests.

Instead of an assertion it is better to write something like that:

    e3.prev == e2 or {e3, e3.prev, e2}

In that case, if the == is false, you will see not only a false in
.reject file, but also actual values of e3, e3.prev, and e2. This
is especially helpful, when error is not stable, and in Travis/Gitlab,
because you see values right in the web console.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area Nikita Pettik
@ 2020-02-23 17:43   ` Vladislav Shpilevoy
  2020-03-25  1:38     ` Nikita Pettik
  0 siblings, 1 reply; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-23 17:43 UTC (permalink / raw)
  To: Nikita Pettik, tarantool-patches

Thanks for the patch!

See 10 comments below.

On 19/02/2020 15:16, Nikita Pettik wrote:
> This patch introduces support of stacked errors in IProto protocol and
> in net.box module.
> 
> @TarantoolBot document
> Title: Stacked error diagnostic area
> 
> Starting from now errors can be organized into lists. To achieve this
> Lua table representing error object is extended with .prev field and
> e:set_prev(err) method. .prev field returns previous error if any exist.
> e:set_prev(err) method expects err to be error object or nil and sets
> err as previous error of e. For instance:
> 
> e1 = box.error.new({code = 111, reason = "cause"})
> e2 = box.error.new({code = 111, reason = "cause of cause"})
> 
> e1:set_prev(e2)
> assert(e1.prev == e2) -- true
> 
> Cycles are not allowed for error lists:
> 
> e2:set_prev(e1)
> - error: 'builtin/error.lua: Cycles are not allowed'
> 
> Nil is valid input to :set_prev() method:
> 
> e1:set_prev(nil)
> assert(e1.prev == nil) -- true
> 
> Note that error can be 'previous' only to the one error at once:
> 
> e1:set_prev(e2)
> e3:set_prev(e2)
> assert(e1.prev == nil) -- true
> assert(e3.prev == e2) -- true
> 
> Setting previous error does not erase its own previous members:
> 
> -- e1 -> e2 -> e3 -> e4
> e1:set_prev(e2)
> e2:set_prev(e3)
> e3:set_prev(e4)
> e2:set_prev(e5)
> -- Now there are two lists: e1->e2->e5 and e3->e4
> assert(e1.prev == e2) -- true
> assert(e2.prev == e5) -- true
> assert(e3.prev == e4) -- true
> 
> Alternatively:
> 
> e1:set_prev(e2)
> e2:set_prev(e3)
> e3:set_prev(e4)
> e5:set_prev(e3)
> -- Now there are two lists: e1->e2 and e5->e3->e4
> assert(e1.prev == e2) -- true
> assert(e2.prev == nil) -- true
> assert(e5.prev == e3) -- true
> assert(e3.prev == e4) -- true
> 
> Stacked diagnostics is also supported by IProto protocol. Now responses
> containing errors always (even there's only one error to be returned)

1. there's -> if there's.

> include new IProto key: IPROTO_ERROR_STACK (0x51). So, body corresponding to
> error response now looks like:
> MAP{IPROTO_ERROR : string, IPROTO_ERROR_STACK : ARRAY[MAP{ERROR_CODE : uint, ERROR_MESSAGE : string}, MAP{...}, ...]}
> 
> where IPROTO_ERROR is 0x31 key, IPROTO_ERROR_STACK is 0x51, ERROR_CODE
> is 0x01 and ERROR_MESSAGE is 0x02.
> Instances of older versions (without support of stacked errors in
> protocol) simply ignore unknown keys and still rely only on IPROTO_ERROR
> key.
> 
> Closes #1148
> ---
>  src/box/error.cc           |  17 +++++
>  src/box/error.h            |  16 +++++
>  src/box/iproto_constants.h |   6 ++
>  src/box/lua/net_box.lua    |  32 ++++++++-
>  src/box/xrow.c             |  78 +++++++++++++++++++--
>  test/box-py/iproto.result  |   6 +-
>  test/box-py/iproto.test.py |   6 +-
>  test/box/iproto.result     | 166 +++++++++++++++++++++++++++++++++++++++++++++
>  test/box/iproto.test.lua   |  73 ++++++++++++++++++++
>  test/box/net.box.result    |  65 ++++++++++++++++++
>  test/box/net.box.test.lua  |  25 +++++++
>  11 files changed, 475 insertions(+), 15 deletions(-)
>  create mode 100644 test/box/iproto.result
>  create mode 100644 test/box/iproto.test.lua
> 
> diff --git a/src/box/error.h b/src/box/error.h
> index 42043ef80..626701f27 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -137,6 +137,22 @@ struct error *
>  box_error_construct(const char *file, unsigned line, uint32_t code,
>  		    const char *fmt, ...);
>  
> +/**
> + * Add error to the diagnostic area. In contrast to box_error_set()
> + * it does not replace previous error being set, but rather link
> + * them into list.
> + *
> + * \param code IPROTO error code (enum \link box_error_code \endlink)
> + * \param format (const char * ) - printf()-like format string
> + * \param ... - format arguments
> + * \returns -1 for convention use
> + *
> + * \sa enum box_error_code
> + */
> +int
> +box_error_add(const char *file, unsigned line, uint32_t code,
> +	      const char *fmt, ...);

2. Lets keep it out of the public C API for now. We can add it later,
when somebody asks.

> +
>  /**
>   * A backward-compatible API define.
>   */
> diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
> index b66c05c06..a77660018 100644
> --- a/src/box/iproto_constants.h
> +++ b/src/box/iproto_constants.h> @@ -149,6 +150,11 @@ enum iproto_ballot_key {
>  	IPROTO_BALLOT_IS_LOADING = 0x04,
>  };
>  
> +enum iproto_error_key {
> +	IPROTO_ERROR_CODE = 0x01,
> +	IPROTO_ERROR_MESSAGE = 0x02,

3. I would use normal decimal numbers, and start from 0.
There is no any reason why should it be hex and start from 1.
Up to you.

> +};
> +
>  #define bit(c) (1ULL<<IPROTO_##c)
>  
>  #define IPROTO_HEAD_BMAP (bit(REQUEST_TYPE) | bit(SYNC) | bit(REPLICA_ID) |\
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index b4811edfa..9a619e3d4 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -277,8 +280,24 @@ local function create_transport(host, port, user, password, callback,
>      --
>      function request_index:result()
>          if self.errno then
> -            return nil, box.error.new({code = self.errno,
> -                                       reason = self.response})
> +            if type(self.response) == 'table' then
> +                -- Decode and fill in error stack.
> +                local prev = nil
> +                for i = #self.response, 1, -1 do

4. Why do you start from the end? Seems like you could easily
do the same with the direct iteration. Your way is not worse,
but it raises unnecessary questions.

> +                    local error = self.response[i]
> +                    local code = error[IPROTO_ERROR_CODE]
> +                    local msg = error[IPROTO_ERROR_MESSAGE]
> +                    assert(type(code) == 'number')
> +                    assert(type(msg) == 'string')
> +                    local new_err = box.error.new({code = code, reason = msg})
> +                    new_err:set_prev(prev)
> +                    prev = new_err
> +                end
> +                return nil, prev
> +            else
> +                return nil, box.error.new({code = self.errno,
> +                                           reason = self.response})
> +            end
>          elseif not self.id then
>              return self.response
>          elseif not worker_fiber then
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index 3f1c90c87..b8c78cbc5 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -1072,6 +1085,48 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len,
>  	return 0;
>  }
>  
> +static int
> +iproto_decode_error_stack(const char **pos)
> +{
> +	char *reason = tt_static_buf();
> +	static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\
> +		      "expected to be larger than error message max length");
> +	/*
> +	 * Erase previously set diag errors. It is required since
> +	 * box_error_add() does not replace previous errors.
> +	 */
> +	box_error_clear();
> +	uint32_t stack_sz = mp_decode_array(pos);
> +	for (uint32_t i = 0; i < stack_sz; i++) {
> +		uint32_t code = 0;
> +		if (mp_typeof(**pos) != MP_MAP)
> +			return -1;
> +		uint32_t map_sz = mp_decode_map(pos);

5. Before calling any decode() you need to call a corresponding
check(). Otherwise a truncated packet can crash Tarantool. Check
other xrow decoders and net.box.test.lua for corruption tests.

> +		for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
> +			if (mp_typeof(**pos) != MP_UINT)
> +				return -1;
> +			uint8_t key = mp_decode_uint(pos);
> +			if (key == IPROTO_ERROR_CODE) {
> +				if (mp_typeof(**pos) != MP_UINT)
> +					return -1;
> +				code = mp_decode_uint(pos);
> +			} else if (key == IPROTO_ERROR_MESSAGE) {
> +				if (mp_typeof(**pos) != MP_STR)
> +					return -1;
> +				uint32_t len;
> +				const char *str = mp_decode_str(pos, &len);
> +				snprintf(reason, TT_STATIC_BUF_LEN, "%.*s",
> +					 len, str);

6. Specifically for this we have tt_cstr() function. Lets use it
here.

> +			} else {
> +				mp_next(pos);
> +				continue;
> +			}
> +			box_error_add(__FILE__, __LINE__, code, reason);

7. Someday we should send file and line in iproto too. Not related
to your patch tho.

> +		}
> +	}
> +	return 0;
> +}
> +
>  void
>  xrow_decode_error(struct xrow_header *row)
>  {
> @@ -1098,15 +1153,26 @@ xrow_decode_error(struct xrow_header *row)
>  			continue;
>  		}
>  		uint8_t key = mp_decode_uint(&pos);
> -		if (key != IPROTO_ERROR || mp_typeof(*pos) != MP_STR) {
> -			mp_next(&pos); /* value */
> +		if (key == IPROTO_ERROR && mp_typeof(*pos) == MP_STR) {
> +			/*
> +			 * Obsolete way of sending error responses.
> +			 * To be deprecated but still should be supported
> +			 * to not break backward compatibility.
> +			 */
> +			uint32_t len;
> +			const char *str = mp_decode_str(&pos, &len);
> +			snprintf(error, sizeof(error), "%.*s", len, str);
> +			box_error_set(__FILE__, __LINE__, code, error);
> +		} else if (key == IPROTO_ERROR_STACK &&
> +			   mp_typeof(*pos) == MP_ARRAY) {

8. If we got an error stack, but it is not an array, this looks like a broken
packet.

> +			if (iproto_decode_error_stack(&pos) != 0)
> +				continue;
> +		} else {
> +			mp_next(&pos);
>  			continue;
>  		}
> -
> -		uint32_t len;
> -		const char *str = mp_decode_str(&pos, &len);
> -		snprintf(error, sizeof(error), "%.*s", len, str);
>  	}
> diff --git a/test/box/iproto.result b/test/box/iproto.result
> new file mode 100644
> index 000000000..28b8bf140
> --- /dev/null
> +++ b/test/box/iproto.result
> @@ -0,0 +1,166 @@
> +test_run = require('test_run').new()
> +---
> +...
> +net_box = require('net.box')
> +---
> +...
> +urilib = require('uri')
> +---
> +...
> +msgpack = require('msgpack')
> +---
> +...
> +IPROTO_REQUEST_TYPE   = 0x00
> +---
> +...
> +IPROTO_INSERT         = 0x02
> +---
> +...
> +IPROTO_SYNC           = 0x01
> +---
> +...
> +IPROTO_SPACE_ID       = 0x10
> +---
> +...
> +IPROTO_TUPLE          = 0x21
> +---
> +...
> +IPROTO_ERROR          = 0x31
> +---
> +...
> +IPROTO_ERROR_STACK    = 0x51
> +---
> +...
> +IPROTO_ERROR_CODE     = 0x01
> +---
> +...
> +IPROTO_ERROR_MESSAGE  = 0x02
> +---
> +...
> +IPROTO_OK             = 0x00
> +---
> +...
> +IPROTO_SCHEMA_VERSION = 0x05
> +---
> +...
> +IPROTO_STATUS_KEY     = 0x00
> +---
> +...
> +-- gh-1148: test capabilities of stacked diagnostics bypassing net.box.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
> +test_run:cmd("setopt delimiter ''");

9. Why do you need a custom delimiter for one line? The same in the
net.box test.

> diff --git a/test/box/net.box.result b/test/box/net.box.result
> index e3dabf7d9..c5d5d3743 100644
> --- a/test/box/net.box.result
> +++ b/test/box/net.box.result
> @@ -3902,6 +3902,71 @@ sock:close()
>  ---
>  - true
>  ...
> +-- gh-1148: test stacked diagnostics.
> +--
> +test_run:cmd("push filter \"file: .*\" to \"file: <filename>\"")
> +---
> +- true
> +...
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]]
> +test_run:cmd("setopt delimiter ''");
> +---
> +...
> +box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true})
> +---
> +...
> +s = box.schema.space.create('s')
> +---
> +...
> +pk = s:create_index('pk')
> +---
> +...
> +idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}})
> +---
> +...
> +box.schema.user.grant('guest', 'read,write,execute', 'universe')
> +---
> +...
> +c = net.connect(box.cfg.listen)
> +---
> +...
> +f = function(...) return c.space.s:insert(...) end
> +---
> +...
> +_, e = pcall(f, {1})
> +---
> +...
> +assert(e ~= nil)
> +---
> +- true
> +...
> +e:unpack().message

10. You don't need to call unpack() to get a message. Just write e.message.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 5/7] box/error: clarify purpose of reference counting in struct error
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 5/7] box/error: clarify purpose of reference counting in struct error Nikita Pettik
@ 2020-02-23 17:43   ` Vladislav Shpilevoy
  2020-03-25  1:40     ` Nikita Pettik
  0 siblings, 1 reply; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-23 17:43 UTC (permalink / raw)
  To: Nikita Pettik, tarantool-patches

Thanks for the patch!

How about merging this commit into the previous one?

On 19/02/2020 15:16, Nikita Pettik wrote:
> ---
>  src/lib/core/diag.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 5271733cb..a9181c522 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -72,6 +72,15 @@ struct error {
>  	error_f raise;
>  	error_f log;
>  	const struct type_info *type;
> +	/**
> +	 * Reference counting is basically required since
> +	 * instances of this structure are available in Lua
> +	 * as well (as cdata with overloaded fields and methods
> +	 * by the means of introspection). Thus, it may turn
> +	 * out that Lua's GC attempts at releasing object
> +	 * meanwhile it is still used in C internals or vice
> +	 * versa. For details see luaT_pusherror().

Hmm, no, it can't anymore. Lua can't delete an error object,
if it is still used in C. This looks like an artifact from the
previous patch version, were you unreferenced all errors when
diag was cleared.

> +	 */
>  	int refs;
>  	/**
>  	 * Errno at the moment of the error
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream
  2020-02-19 14:16 ` [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream Nikita Pettik
@ 2020-02-23 17:44   ` Vladislav Shpilevoy
  2020-03-25  1:42     ` Nikita Pettik
  0 siblings, 1 reply; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-02-23 17:44 UTC (permalink / raw)
  To: Nikita Pettik, tarantool-patches

Thanks for the patch!

See 2 comments below.

On 19/02/2020 15:16, Nikita Pettik wrote:
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> 
> Refactor iproto_reply_error and iproto_write_error with a new
> mpstream-based helper mpstream_iproto_encode_error that encodes
> error object for iproto protocol on a given stream object.
> Previously each routine implemented an own error encoding, but
> with the increasing complexity of encode operation with following
> patches we need a uniform way to do it.
> 
> The iproto_write_error routine starts using region location
> to use region-based mpstream. It is not a problem itself, because
> errors reporting is not really performance-critical path.
> 
> Needed for #1148
> ---
>  src/box/xrow.c | 72 +++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 51 insertions(+), 21 deletions(-)
> 
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index 968c3a202..3f1c90c87 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -36,12 +36,14 @@
>  #include "third_party/base64.h"
>  
>  #include "fiber.h"
> +#include "reflection.h"

1. I removed this header and nothing changed. So why do you need it?

>  #include "version.h"
>  #include "tt_static.h"
>  #include "error.h"
>  #include "vclock.h"
>  #include "scramble.h"
>  #include "iproto_constants.h"
> +#include "mpstream.h"
>  
>  static_assert(IPROTO_DATA < 0x7f && IPROTO_METADATA < 0x7f &&
>  	      IPROTO_SQL_INFO < 0x7f, "encoded IPROTO_BODY keys must fit into "\
> @@ -478,46 +476,78 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot,
>  	return 0;
>  }
>  
> +static void
> +mpstream_error_handler(void *error_ctx)
> +{
> +	*(bool *)error_ctx = true;
> +}
> +
> +static void
> +mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
> +{
> +	mpstream_encode_map(stream, 2);
> +	mpstream_encode_uint(stream, IPROTO_ERROR);
> +	mpstream_encode_str(stream, error->errmsg);
> +}
> +
>  int
>  iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
>  		   uint32_t schema_version)
>  {
> -	uint32_t msg_len = strlen(e->errmsg);
> -	uint32_t errcode = box_error_code(e);
> -
> -	struct iproto_body_bin body = iproto_error_bin;
>  	char *header = (char *)obuf_alloc(out, IPROTO_HEADER_LEN);
>  	if (header == NULL)
>  		return -1;
>  
> +	/* The obuf-based stream has reserved area for header. */

2. No, you reserved space for the header just a few lines above.
Mpstream does not know anything about headers.

> +	bool is_error = false;
> +	struct mpstream stream;
> +	mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
> +		      mpstream_error_handler, &is_error);
> +
> +	uint32_t used = obuf_size(out);
> +	mpstream_iproto_encode_error(&stream, e);
> +	mpstream_flush(&stream);
> +
> +	uint32_t errcode = box_error_code(e);
>  	iproto_header_encode(header, iproto_encode_error(errcode), sync,
> -			     schema_version, sizeof(body) + msg_len);
> -	body.v_data_len = mp_bswap_u32(msg_len);
> +			     schema_version, obuf_size(out) - used);
> +
>  	/* Malformed packet appears to be a lesser evil than abort. */
> -	return obuf_dup(out, &body, sizeof(body)) != sizeof(body) ||
> -	       obuf_dup(out, e->errmsg, msg_len) != msg_len ? -1 : 0;
> +	return is_error;
>  }
>  
>  void
>  iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
>  		   uint64_t sync)
>  {
> -	uint32_t msg_len = strlen(e->errmsg);
> -	uint32_t errcode = box_error_code(e);
> +	bool is_error = false;
> +	struct mpstream stream;
> +	struct region *region = &fiber()->gc;
> +	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> +		      mpstream_error_handler, &is_error);
> +
> +	size_t region_svp = region_used(region);
> +	mpstream_iproto_encode_error(&stream, e);
> +	mpstream_flush(&stream);
> +	if (is_error)
> +		goto cleanup;
> +
> +	size_t payload_size = region_used(region) - region_svp;
> +	char *payload = region_join(region, payload_size);
> +	if (payload == NULL)
> +		goto cleanup;
>  
> +	uint32_t errcode = box_error_code(e);
>  	char header[IPROTO_HEADER_LEN];
> -	struct iproto_body_bin body = iproto_error_bin;
> -
>  	iproto_header_encode(header, iproto_encode_error(errcode), sync,
> -			     schema_version, sizeof(body) + msg_len);
> -
> -	body.v_data_len = mp_bswap_u32(msg_len);
> +			     schema_version, payload_size);
>  
>  	ssize_t unused;
>  	unused = write(fd, header, sizeof(header));
> -	unused = write(fd, &body, sizeof(body));
> -	unused = write(fd, e->errmsg, msg_len);
> +	unused = write(fd, payload, payload_size);
>  	(void) unused;
> +cleanup:
> +	region_truncate(region, region_svp);
>  }
>  
>  int
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag
  2020-02-22 17:18   ` Vladislav Shpilevoy
@ 2020-03-25  1:02     ` Nikita Pettik
  2020-03-26  0:22       ` Vladislav Shpilevoy
  0 siblings, 1 reply; 27+ messages in thread
From: Nikita Pettik @ 2020-03-25  1:02 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On 22 Feb 18:18, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/error.cc b/src/box/error.cc
> > index 7dfe1b3ee..824a4617c 100644
> > --- a/src/box/error.cc
> > +++ b/src/box/error.cc
> > @@ -86,6 +86,22 @@ box_error_set(const char *file, unsigned line, uint32_t code,
> >  	return -1;
> >  }
> >  
> > +struct error *
> > +box_error_construct(const char *file, unsigned line, uint32_t code,
> > +		    const char *fmt, ...)
> > +{
> > +	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> > +	ClientError *client_error = type_cast(ClientError, e);
> > +	if (client_error != NULL) {
> > +		client_error->m_errcode = code;
> 
> 4. You can now make box_error_set() call box_error_construct() +
> diag_set(), to avoid code duplication.

I don't think it is worth it since I have to introduce one more
'proxy' function which accepts va_list arguments. Skipped.
 
> > +		va_list ap;
> > +		va_start(ap, fmt);
> > +		error_vformat_msg(e, fmt, ap);
> > +		va_end(ap);
> > +	}
> > +	return e;
> > +}
> > +
> >  /* }}} */
> >  
> >  struct rmean *rmean_error = NULL;
> > diff --git a/src/box/error.h b/src/box/error.h
> > index b8c7cf73d..42043ef80 100644
> > --- a/src/box/error.h
> > +++ b/src/box/error.h
> > @@ -129,6 +129,14 @@ int
> >  box_error_set(const char *file, unsigned line, uint32_t code,
> >  	      const char *format, ...);
> >  
> > +/**
> > + * Construct error object without setting it in the diagnostics
> > + * area. On the memory allocation fail returns NULL.
> > + */
> > +struct error *
> > +box_error_construct(const char *file, unsigned line, uint32_t code,
> > +		    const char *fmt, ...);
> 
> 5. You added the method to the public C API (it is inside 'cond public'
> comments). I don't think we should do that. At least until someone
> asked so.
> 
> Also why not box_error_new()? 'Construct' seems to be a very strange name.

IMHO _new 'suffix' implies allocating new object, meanwhile _construct - 
filling in already allocated object. It was my understanding.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area
  2020-02-23 17:43   ` Vladislav Shpilevoy
@ 2020-03-25  1:34     ` Nikita Pettik
  0 siblings, 0 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-03-25  1:34 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On 23 Feb 18:43, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch, welcome to the first review iteration!
> 
> Finally something good, not related to SQL/replication/application server
> modules.
> 
> > +/**
> > + * Add a new error to the diagnostics area. It is added to the
> > + * tail, so that list forms stack.
> > + * @param diag Diagnostics area.
> > + * @param e Error to be added.
> > + */
> > +static inline void
> > +diag_add_error(struct diag *diag, struct error *e)
> > +{
> > +	assert(e != NULL);
> > +	error_ref(e);
> > +	error_unlink_tail(e);
> > +	e->next = diag->last;
> 
> 15. I thought, that 'next' member is responsible for keeping a
> reference, but since you don't ref 'diag->last' here, I am not
> sure now. Could you please clarify?

Yes, but since diag->last is not longer the head of diag list,
it also should be unrefed. One ref and one unref - as a result
no ref actions. 
 
> > +	if (diag->last != NULL)
> > +		diag->last->prev = e;
> >  	diag->last = e;
> >  }
> >  
> 
> 21. It is not really necessary to use assertion here. It will fail
> without assertion too in case of a problem. Assertion is needed,
> when you want an exception. For example, when you call a function,
> inside which you do tests.

IMHO assertions underline the fact of testing result of particular
action (since we don't have explicit test case borders, it may turn
out to be vital while attempting at exploring test).
We can discuss it in server chat with other members, I do not mind both
approaches.
 
> Instead of an assertion it is better to write something like that:
> 
>     e3.prev == e2 or {e3, e3.prev, e2}
> 
> In that case, if the == is false, you will see not only a false in
> .reject file, but also actual values of e3, e3.prev, and e2. This
> is especially helpful, when error is not stable, and in Travis/Gitlab,
> because you see values right in the web console.

These tests are quite easy to reproduce copying them to Tarantool's
console.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area
  2020-02-23 17:43   ` Vladislav Shpilevoy
@ 2020-03-25  1:38     ` Nikita Pettik
  0 siblings, 0 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-03-25  1:38 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On 23 Feb 18:43, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index b4811edfa..9a619e3d4 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> > @@ -277,8 +280,24 @@ local function create_transport(host, port, user, password, callback,
> >      --
> >      function request_index:result()
> >          if self.errno then
> > -            return nil, box.error.new({code = self.errno,
> > -                                       reason = self.response})
> > +            if type(self.response) == 'table' then
> > +                -- Decode and fill in error stack.
> > +                local prev = nil
> > +                for i = #self.response, 1, -1 do
> 
> 4. Why do you start from the end? Seems like you could easily
> do the same with the direct iteration. Your way is not worse,
> but it raises unnecessary questions.

Hm, it's like stack restoration from top to bottom. On the contrary
it seems to be more rational to start iterating from the top (IMHO).
 
> > +                    local error = self.response[i]
> > +                    local code = error[IPROTO_ERROR_CODE]
> > +                    local msg = error[IPROTO_ERROR_MESSAGE]
> > +                    assert(type(code) == 'number')
> > +                    assert(type(msg) == 'string')
> > +                    local new_err = box.error.new({code = code, reason = msg})
> > +                    new_err:set_prev(prev)
> > +                    prev = new_err
> > +                end
> > +                return nil, prev
> > +            else
> > +                return nil, box.error.new({code = self.errno,
> > +                                           reason = self.response})
> > +            end
> >          elseif not self.id then
> >              return self.response
> >          elseif not worker_fiber then

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 5/7] box/error: clarify purpose of reference counting in struct error
  2020-02-23 17:43   ` Vladislav Shpilevoy
@ 2020-03-25  1:40     ` Nikita Pettik
  0 siblings, 0 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-03-25  1:40 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On 23 Feb 18:43, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> How about merging this commit into the previous one?
> 
> On 19/02/2020 15:16, Nikita Pettik wrote:
> > ---
> >  src/lib/core/diag.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> > index 5271733cb..a9181c522 100644
> > --- a/src/lib/core/diag.h
> > +++ b/src/lib/core/diag.h
> > @@ -72,6 +72,15 @@ struct error {
> >  	error_f raise;
> >  	error_f log;
> >  	const struct type_info *type;
> > +	/**
> > +	 * Reference counting is basically required since
> > +	 * instances of this structure are available in Lua
> > +	 * as well (as cdata with overloaded fields and methods
> > +	 * by the means of introspection). Thus, it may turn
> > +	 * out that Lua's GC attempts at releasing object
> > +	 * meanwhile it is still used in C internals or vice
> > +	 * versa. For details see luaT_pusherror().
> 
> Hmm, no, it can't anymore.

Now it can't. But it used to, until ref in luaT_pusherror() was
introduced. So may comment refer to that quite ancient fix (I just
wondered why can't we get rid of ref counter and came up with
this explanation).

> Lua can't delete an error object,
> if it is still used in C. This looks like an artifact from the
> previous patch version, were you unreferenced all errors when
> diag was cleared.
> 
> > +	 */
> >  	int refs;
> >  	/**
> >  	 * Errno at the moment of the error
> > 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream
  2020-02-23 17:44   ` Vladislav Shpilevoy
@ 2020-03-25  1:42     ` Nikita Pettik
  0 siblings, 0 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-03-25  1:42 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On 23 Feb 18:44, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 2 comments below.
> 
> On 19/02/2020 15:16, Nikita Pettik wrote:
> > From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> > 
> > +}
> > +
> >  int
> >  iproto_reply_error(struct obuf *out, const struct error *e, uint64_t sync,
> >  		   uint32_t schema_version)
> >  {
> > -	uint32_t msg_len = strlen(e->errmsg);
> > -	uint32_t errcode = box_error_code(e);
> > -
> > -	struct iproto_body_bin body = iproto_error_bin;
> >  	char *header = (char *)obuf_alloc(out, IPROTO_HEADER_LEN);
> >  	if (header == NULL)
> >  		return -1;
> >  
> > +	/* The obuf-based stream has reserved area for header. */
> 
> 2. No, you reserved space for the header just a few lines above.
> Mpstream does not know anything about headers.

Didn't really get what you mean here, so just in case dropped this
comment at all :)
 
> > +	bool is_error = false;
> > +	struct mpstream stream;
> > +	mpstream_init(&stream, out, obuf_reserve_cb, obuf_alloc_cb,
> > +		      mpstream_error_handler, &is_error);
> > +
> > +	uint32_t used = obuf_size(out);
> > +	mpstream_iproto_encode_error(&stream, e);
> > +	mpstream_flush(&stream);
> > +
> > +	uint32_t errcode = box_error_code(e);
> >  	iproto_header_encode(header, iproto_encode_error(errcode), sync,
> > -			     schema_version, sizeof(body) + msg_len);
> > -	body.v_data_len = mp_bswap_u32(msg_len);
> > +			     schema_version, obuf_size(out) - used);
> > +
> >  	/* Malformed packet appears to be a lesser evil than abort. */
> > -	return obuf_dup(out, &body, sizeof(body)) != sizeof(body) ||
> > -	       obuf_dup(out, e->errmsg, msg_len) != msg_len ? -1 : 0;
> > +	return is_error;
> >  }

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag
  2020-03-25  1:02     ` Nikita Pettik
@ 2020-03-26  0:22       ` Vladislav Shpilevoy
  2020-03-26  1:03         ` Nikita Pettik
  0 siblings, 1 reply; 27+ messages in thread
From: Vladislav Shpilevoy @ 2020-03-26  0:22 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches

On 25/03/2020 02:02, Nikita Pettik wrote:
> On 22 Feb 18:18, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>>> diff --git a/src/box/error.cc b/src/box/error.cc
>>> index 7dfe1b3ee..824a4617c 100644
>>> --- a/src/box/error.cc
>>> +++ b/src/box/error.cc
>>> @@ -86,6 +86,22 @@ box_error_set(const char *file, unsigned line, uint32_t code,
>>>  	return -1;
>>>  }
>>>  
>>> +struct error *
>>> +box_error_construct(const char *file, unsigned line, uint32_t code,
>>> +		    const char *fmt, ...)
>>> +{
>>> +	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
>>> +	ClientError *client_error = type_cast(ClientError, e);
>>> +	if (client_error != NULL) {
>>> +		client_error->m_errcode = code;
>>
>> 4. You can now make box_error_set() call box_error_construct() +
>> diag_set(), to avoid code duplication.
> 
> I don't think it is worth it since I have to introduce one more
> 'proxy' function which accepts va_list arguments. Skipped.

Yeah, I forgot that it is not as as trivial to forward ... as
in Lua.

>>> +		va_list ap;
>>> +		va_start(ap, fmt);
>>> +		error_vformat_msg(e, fmt, ap);
>>> +		va_end(ap);
>>> +	}
>>> +	return e;
>>> +}
>>> +
>>>  /* }}} */
>>>  
>>>  struct rmean *rmean_error = NULL;
>>> diff --git a/src/box/error.h b/src/box/error.h
>>> index b8c7cf73d..42043ef80 100644
>>> --- a/src/box/error.h
>>> +++ b/src/box/error.h
>>> @@ -129,6 +129,14 @@ int
>>>  box_error_set(const char *file, unsigned line, uint32_t code,
>>>  	      const char *format, ...);
>>>  
>>> +/**
>>> + * Construct error object without setting it in the diagnostics
>>> + * area. On the memory allocation fail returns NULL.
>>> + */
>>> +struct error *
>>> +box_error_construct(const char *file, unsigned line, uint32_t code,
>>> +		    const char *fmt, ...);
>>
>> 5. You added the method to the public C API (it is inside 'cond public'
>> comments). I don't think we should do that. At least until someone
>> asked so.
>>
>> Also why not box_error_new()? 'Construct' seems to be a very strange name.
> 
> IMHO _new 'suffix' implies allocating new object, meanwhile _construct - 
> filling in already allocated object. It was my understanding.
Exactly. And box_error_construct() allocates a new object. You even said
that in the comment. So where am I wrong?

Btw, for initialization of an object in-place we use '_create'.
'_init' is used for initialization of a module. But '_construct' - never.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag
  2020-03-26  0:22       ` Vladislav Shpilevoy
@ 2020-03-26  1:03         ` Nikita Pettik
  0 siblings, 0 replies; 27+ messages in thread
From: Nikita Pettik @ 2020-03-26  1:03 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

On 26 Mar 01:22, Vladislav Shpilevoy wrote:
> On 25/03/2020 02:02, Nikita Pettik wrote:
> > On 22 Feb 18:18, Vladislav Shpilevoy wrote:
> >> Thanks for the patch!
> >>
> >>> diff --git a/src/box/error.h b/src/box/error.h
> >>> index b8c7cf73d..42043ef80 100644
> >>> --- a/src/box/error.h
> >>> +++ b/src/box/error.h
> >>> @@ -129,6 +129,14 @@ int
> >>>  box_error_set(const char *file, unsigned line, uint32_t code,
> >>>  	      const char *format, ...);
> >>>  
> >>> +/**
> >>> + * Construct error object without setting it in the diagnostics
> >>> + * area. On the memory allocation fail returns NULL.
> >>> + */
> >>> +struct error *
> >>> +box_error_construct(const char *file, unsigned line, uint32_t code,
> >>> +		    const char *fmt, ...);
> >>
> >> 5. You added the method to the public C API (it is inside 'cond public'
> >> comments). I don't think we should do that. At least until someone
> >> asked so.
> >>
> >> Also why not box_error_new()? 'Construct' seems to be a very strange name.
> > 
> > IMHO _new 'suffix' implies allocating new object, meanwhile _construct - 
> > filling in already allocated object. It was my understanding.
> Exactly. And box_error_construct() allocates a new object. You even said
> that in the comment. So where am I wrong?

Indeed. I should have paid more attention. Let's rename it to _new().
Otherwise patch looks good to you?
 
> Btw, for initialization of an object in-place we use '_create'.
> '_init' is used for initialization of a module. But '_construct' - never.

Ok, will keep in mind.

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2020-03-26  1:03 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-19 14:16 [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Nikita Pettik
2020-02-19 14:16 ` [Tarantool-patches] [PATCH 1/7] box: rename diag_add_error to diag_set_error Nikita Pettik
2020-02-19 14:16 ` [Tarantool-patches] [PATCH 2/7] box/error: introduce box.error.set() method Nikita Pettik
2020-02-19 14:26   ` Cyrill Gorcunov
2020-02-19 14:30     ` Nikita Pettik
2020-02-19 14:53       ` Cyrill Gorcunov
2020-02-19 14:16 ` [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag Nikita Pettik
2020-02-22 17:18   ` Vladislav Shpilevoy
2020-03-25  1:02     ` Nikita Pettik
2020-03-26  0:22       ` Vladislav Shpilevoy
2020-03-26  1:03         ` Nikita Pettik
2020-02-19 14:16 ` [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area Nikita Pettik
2020-02-19 21:10   ` Vladislav Shpilevoy
2020-02-20 11:53     ` Nikita Pettik
2020-02-20 18:29       ` Nikita Pettik
2020-02-23 17:43   ` Vladislav Shpilevoy
2020-03-25  1:34     ` Nikita Pettik
2020-02-19 14:16 ` [Tarantool-patches] [PATCH 5/7] box/error: clarify purpose of reference counting in struct error Nikita Pettik
2020-02-23 17:43   ` Vladislav Shpilevoy
2020-03-25  1:40     ` Nikita Pettik
2020-02-19 14:16 ` [Tarantool-patches] [PATCH 6/7] iproto: refactor error encoding with mpstream Nikita Pettik
2020-02-23 17:44   ` Vladislav Shpilevoy
2020-03-25  1:42     ` Nikita Pettik
2020-02-19 14:16 ` [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area Nikita Pettik
2020-02-23 17:43   ` Vladislav Shpilevoy
2020-03-25  1:38     ` Nikita Pettik
2020-02-22 17:18 ` [Tarantool-patches] [PATCH 0/7] Stacked diagnostics area Vladislav Shpilevoy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox