* [tarantool-patches] [PATCH v1 0/3] box: stacked diagnostics area in fiber
@ 2019-08-01 11:13 Kirill Shcherbatov
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Kirill Shcherbatov @ 2019-08-01 11:13 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Cc: alexander.turenko, kostja, Kirill Shcherbatov
Support stacked diagnostics for Tarantool allows to accumulate all occurred errors during processing a request. This allows to better understand what has happened and handle errors
correspondingly.
http://github.com/tarantool/tarantool/tree/kshch/gh-1148-stacked-errors
https://github.com/tarantool/tarantool/issues/1148
Kirill Shcherbatov (3):
box: rfc for stacked diagnostic area in Tarantool
box: stacked diagnostics area in fiber
box: extend ffi error object API
src/box/error.h | 23 +++
src/lib/core/diag.h | 45 ++++-
src/lib/core/exception.h | 2 +-
src/box/key_list.c | 16 +-
src/box/lua/call.c | 6 +-
src/box/vy_scheduler.c | 6 +-
src/lib/core/diag.c | 1 +
src/lua/utils.c | 2 +-
doc/rfc/1148-stacked-diagnostics.md | 136 ++++++++++++++
extra/exports | 2 +
src/box/applier.cc | 2 +-
src/box/error.cc | 27 ++-
src/box/relay.cc | 4 +-
src/lib/core/exception.cc | 2 +-
src/lua/error.lua | 75 +++++++-
test/app/fiber.result | 12 +-
test/box/access.result | 2 +-
test/box/access.test.lua | 2 +-
test/box/errors.result | 265 ++++++++++++++++++++++++++++
test/box/errors.test.lua | 53 ++++++
test/box/misc.result | 12 +-
test/engine/func_index.result | 65 ++++++-
test/engine/func_index.test.lua | 4 +
23 files changed, 713 insertions(+), 51 deletions(-)
create mode 100644 doc/rfc/1148-stacked-diagnostics.md
create mode 100644 test/box/errors.result
create mode 100644 test/box/errors.test.lua
--
2.22.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool
2019-08-01 11:13 [tarantool-patches] [PATCH v1 0/3] box: stacked diagnostics area in fiber Kirill Shcherbatov
@ 2019-08-01 11:13 ` Kirill Shcherbatov
2019-08-05 21:16 ` [tarantool-patches] " Vladislav Shpilevoy
2019-08-07 23:27 ` Alexander Turenko
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 2/3] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 3/3] box: extend ffi error object API Kirill Shcherbatov
2 siblings, 2 replies; 17+ messages in thread
From: Kirill Shcherbatov @ 2019-08-01 11:13 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Cc: alexander.turenko, kostja, Kirill Shcherbatov
Part of #1148
---
doc/rfc/1148-stacked-diagnostics.md | 136 ++++++++++++++++++++++++++++
1 file changed, 136 insertions(+)
create mode 100644 doc/rfc/1148-stacked-diagnostics.md
diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
new file mode 100644
index 000000000..a007491aa
--- /dev/null
+++ b/doc/rfc/1148-stacked-diagnostics.md
@@ -0,0 +1,136 @@
+# Stacked Diagnostics in Tarantool
+
+* **Status**: In progress
+* **Start date**: 30-08-2019
+* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org, Konstantin Osipov @kostja kostja@tarantool.org, Georgy Kirichenko @GeorgyKirichenko georgy@tarantool.org, @Totktonada Alexander Turenko alexander.turenko@tarantool.org
+* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
+
+## Summary
+Support stacked diagnostics for Tarantool allows to accumulate all occurred errors during processing a request. This allows to better understand what has happened and handle errors
+correspondingly.
+
+## Background and motivation
+
+Tarantool statements must produce diagnostic information that populates the diagnostics area. This is a Standard SQL requirement and other vendors and languages also have such feature.
+Diagnostics area stack must contain a diagnostics area for each nested execution context.
+
+### Current Tarantool's error diagnostics
+Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
+The last error is exported with `box.error.last()` endpoint.
+
+In total there are 5 error classes in Tarantool. Each error class is associated with set of error codes (@see ClientError::get_errcode(const struct error *e)).
+```
+ClientError -> ClientError::errcode() -> ...;
+OutOfMemory -> MEMORY_ISSUE
+SystemError -> SYSTEM
+CollationError -> CANT_CREATE_COLLATION
+Lua error -> PROC_LUA
+```
+All system-defined errors are exported with their unique error codes in box.error folder by NAME.
+
+You may use box.error.new endpoint to create a new error instance of predefined type:
+```
+tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't")
+tarantool> t:unpack()
+---
+- type: ClientError
+ code: 9
+ message: 'Failed to create space ''myspace'': just I can''t'
+ trace:
+ - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
+ line: 1
+```
+
+User is allowed to define own errors with any code (including system errors) with
+```
+box.error.new{code = user_code, reason = user_error_msg}
+```
+where `user_code` must be greater that the biggest registered system error, say `10000`.
+
+Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()` methods and uniform `.type`, `.message` and `.trace` fields.
+
+
+## Proposal
+In some cases a diagnostic information must be more complicated.
+For example, when persistent Lua function referenced by functional index has a bug in it's definition, Lua handler sets an diag message. Then functional index extractor code sets an own, more specialized error.
+
+In this and similar situations, we need stack-based error diagnostics:
+
+- The serialize method should return the most recent error in stack:
+```
+tarantool> box.error.last()
+Failed to build a key for functional index ''idx'' of space ''withdata': function error
+```
+
+- The `:unpack()` method must return the complete diagnostics stack - a table of errors sorted in their historical order:
+```
+tarantool> box.error.last():unpack()
+---
+ - type: LuajitError
+ message: '[string "return function(tuple)..."]:2: attempt to call global
+ ''require'' (a nil value)'
+ trace:
+ - file: /home/kir/WORK/tarantool/src/lua/utils.c
+ line: 1000
+ - type: ClientError
+ code: 198
+ message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ function error
+ trace:
+ - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ line: 68
+```
+
+- We need to introduce `next_error:wrap(error)` and `error:unwrap()` methods.
+The `wrap` function can take any previous and last error objects to append error to the end of next_error trace. The next_error backtrace is expected to be empty.
+The `unwrap` function removes the element's parent and returns the unwrapped content. This is effectively the inverse of the `wrap()` method.
+```
+Workaround:
+
+function create_new_user()
+ name, err = pcall(box.internal.func_call,
+ box.func.generate_user_name.name)
+ if not name then
+ local e = box.error.new({
+ code = APP_ERROR_CODE,
+ reason = "create_new_user"}):
+ e:wrap(err):raise()
+ end
+ box.schema.user.create(name)
+end
+```
+
+- We also need `error:has` method to test whether given error object contain given error code:
+```
+box.error.last():has(box.error.LuajitError)
+```
+
+## Detailed design
+
+Diagnostic area object has a good encapsulated API.
+
+Lets extend error object with a pointer to it's reason error and introduce a few new methods (we need to rename diag_add_error to diag_set_error because it replaces an existent diag error in fact):
+```
+struct error {
+...
+/* A pointer to the reason error. */
+ struct error *reason;
+};
+
+/**
+ * Add a new error to the diagnostics area: the previous error
+ * becomes a reason of a current.
+ * \param diag diagnostics area
+ * \param e error to add
+ */
+static inline void
+diag_add_error(struct diag *diag, struct error *e);
+
+/** Same as diag_set, but softly extend diagnostic information. */
+#define diag_add(class, ...)
+```
+
+### IPROTO Communication
+Currently errors are sent as (IPROTO_TYPE_ERROR | errcode) messages with error message string payload.
+Lets introduce a new IPROTO_TYPE_ERROR_TRACE error code with
+[[errcode1, "errstring1"], [errcode2, "errstring2"]..] payload, where errors are serialized in historical order.
--
2.22.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] [PATCH v1 2/3] box: stacked diagnostics area in fiber
2019-08-01 11:13 [tarantool-patches] [PATCH v1 0/3] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
@ 2019-08-01 11:13 ` Kirill Shcherbatov
2019-08-05 21:16 ` [tarantool-patches] " Vladislav Shpilevoy
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 3/3] box: extend ffi error object API Kirill Shcherbatov
2 siblings, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2019-08-01 11:13 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Cc: alexander.turenko, kostja, Kirill Shcherbatov
This patch introduces stacked errors. A new API diag_add allows
to extend an existent error information with a new one error.
The previous state becomes a "reason" of the last-set error.
Each error object takes a reference to it's reason error object.
The :unpack() method patched correspondingly to display the whole
errors trace.
Part of #1148
---
src/lib/core/diag.h | 45 +++++++++++++++++++++--
src/lib/core/exception.h | 2 +-
src/box/key_list.c | 16 ++++----
src/box/lua/call.c | 6 +--
src/box/vy_scheduler.c | 6 +--
src/lib/core/diag.c | 1 +
src/lua/utils.c | 2 +-
src/box/applier.cc | 2 +-
src/box/error.cc | 2 +-
src/box/relay.cc | 4 +-
src/lib/core/exception.cc | 2 +-
src/lua/error.lua | 25 +++++++++++--
test/app/fiber.result | 12 +++---
test/box/access.result | 2 +-
test/box/access.test.lua | 2 +-
test/box/misc.result | 12 +++---
test/engine/func_index.result | 65 +++++++++++++++++++++++++++++----
test/engine/func_index.test.lua | 4 ++
18 files changed, 159 insertions(+), 51 deletions(-)
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index fd3831e66..a70a0727d 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -78,6 +78,8 @@ struct error {
char file[DIAG_FILENAME_MAX];
/* Error description. */
char errmsg[DIAG_ERRMSG_MAX];
+ /* A pointer to the reason error. */
+ struct error *reason;
};
static inline void
@@ -90,9 +92,13 @@ static inline void
error_unref(struct error *e)
{
assert(e->refs > 0);
- --e->refs;
- if (e->refs == 0)
+ while (--e->refs == 0) {
+ struct error *reason = e->reason;
e->destroy(e);
+ e = reason;
+ if (e == NULL)
+ break;
+ }
}
NORETURN static inline void
@@ -162,7 +168,22 @@ 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_set_error(struct diag *diag, struct error *e)
+{
+ assert(e != NULL);
+ error_ref(e);
+ diag_clear(diag);
+ diag->last = e;
+}
+
+/**
+ * Add a new error to the diagnostics area: the previous error
+ * becomes a reason of a current.
* \param diag diagnostics area
* \param e error to add
*/
@@ -171,7 +192,12 @@ diag_add_error(struct diag *diag, struct error *e)
{
assert(e != NULL);
error_ref(e);
- diag_clear(diag);
+ /*
+ * Nominally e takes a reason's reference while diag
+ * releases it's reference because it holds e now
+ * instead. I.e. reason->refs kept unchanged.
+ */
+ e->reason = diag->last;
diag->last = e;
}
@@ -264,6 +290,17 @@ BuildSocketError(const char *file, unsigned line, const char *socketname,
const char *format, ...);
#define diag_set(class, ...) do { \
+ /* Preserve the original errno. */ \
+ int save_errno = errno; \
+ say_debug("%s at %s:%i", #class, __FILE__, __LINE__); \
+ struct error *e; \
+ e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__); \
+ diag_set_error(diag_get(), e); \
+ /* Restore the errno which might have been reset. */ \
+ errno = save_errno; \
+} while (0)
+
+#define diag_add(class, ...) do { \
/* Preserve the original errno. */ \
int save_errno = errno; \
say_debug("%s at %s:%i", #class, __FILE__, __LINE__); \
diff --git a/src/lib/core/exception.h b/src/lib/core/exception.h
index a29281427..e1a45b3fb 100644
--- a/src/lib/core/exception.h
+++ b/src/lib/core/exception.h
@@ -182,7 +182,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/box/key_list.c b/src/box/key_list.c
index e130d1c8c..5375e0823 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 key definition");
return -1;
}
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 0ac2eb7a6..cebd8a680 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -680,9 +680,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/box/vy_scheduler.c b/src/box/vy_scheduler.c
index f3bded209..8b1a26ef4 100644
--- a/src/box/vy_scheduler.c
+++ b/src/box/vy_scheduler.c
@@ -606,7 +606,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);
@@ -680,7 +680,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;
@@ -716,7 +716,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.c b/src/lib/core/diag.c
index 248277e74..c0aeb52b9 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -52,6 +52,7 @@ error_create(struct error *e,
e->line = 0;
}
e->errmsg[0] = '\0';
+ e->reason = NULL;
}
struct diag *
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 0a4bcf517..5a2514f3a 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -994,7 +994,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));
diff --git a/src/box/applier.cc b/src/box/applier.cc
index cf03ea160..dbc94c1a7 100644
--- a/src/box/applier.cc
+++ b/src/box/applier.cc
@@ -756,7 +756,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 e9f5bdca9..e44b88ea0 100644
--- a/src/box/relay.cc
+++ b/src/box/relay.cc
@@ -447,7 +447,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
@@ -621,7 +621,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/lib/core/exception.cc b/src/lib/core/exception.cc
index a6999af43..55ee8cd40 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/lua/error.lua b/src/lua/error.lua
index 28fc0377d..2bb8ccfe0 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -23,6 +23,8 @@ struct error {
char _file[DIAG_FILENAME_MAX];
/* Error description. */
char _errmsg[DIAG_ERRMSG_MAX];
+ /* A pointer to the reason error. */
+ struct error *_reason;
};
char *
@@ -92,10 +94,7 @@ local error_fields = {
["trace"] = error_trace;
}
-local function error_unpack(err)
- if not ffi.istype('struct error', err) then
- error("Usage: error:unpack()")
- end
+local function error_unpack_one(err)
local result = {}
for key, getter in pairs(error_fields) do
result[key] = getter(err)
@@ -109,6 +108,24 @@ local function error_unpack(err)
return result
end
+local function error_unpack(err)
+ if not ffi.istype('struct error', err) then
+ error("Usage: error:unpack()")
+ end
+ -- Unwind errors
+ local errors = {}
+ while err ~= nil do
+ table.insert(errors, err)
+ err = err._reason
+ end
+ -- Prepare a result in a reverse order.
+ local result = {}
+ for i = #errors, 1, -1 do
+ table.insert(result, error_unpack_one(errors[i]))
+ end
+ return result
+end
+
local function error_raise(err)
if not ffi.istype('struct error', err) then
error("Usage: error:raise()")
diff --git a/test/app/fiber.result b/test/app/fiber.result
index 94e690f6c..d9424499a 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1038,12 +1038,12 @@ st;
...
e:unpack();
---
-- type: ClientError
- code: 1
- message: Illegal parameters, oh my
- trace:
- - file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
- line: 1
+- - type: ClientError
+ code: 1
+ message: Illegal parameters, oh my
+ trace:
+ - file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
+ line: 1
...
flag = false;
---
diff --git a/test/box/access.result b/test/box/access.result
index ba72b5f74..12a0047f8 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -1340,7 +1340,7 @@ box.session.su("test_user")
st, e = pcall(s.select, s, {})
---
...
-e = e:unpack()
+e = e:unpack()[1]
---
...
e.type, e.access_type, e.object_type, e.message
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index 219cdb04a..24789a785 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -502,7 +502,7 @@ obj_name:match("function")
box.schema.user.revoke("test_user", "usage", "universe")
box.session.su("test_user")
st, e = pcall(s.select, s, {})
-e = e:unpack()
+e = e:unpack()[1]
e.type, e.access_type, e.object_type, e.message
obj_type, obj_name, op_type
euid, auid
diff --git a/test/box/misc.result b/test/box/misc.result
index 7a15dabf0..25dda30a7 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -125,12 +125,12 @@ e
...
e:unpack()
---
-- type: ClientError
- code: 1
- message: Illegal parameters, bla bla
- trace:
- - file: '[C]'
- line: 4294967295
+- - type: ClientError
+ code: 1
+ message: Illegal parameters, bla bla
+ trace:
+ - file: '[C]'
+ line: 4294967295
...
e.type
---
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index 877b76d5e..ae8873c9b 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -167,8 +167,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 key definition'
| ...
idx:drop()
| ---
@@ -189,6 +188,16 @@ s:insert({1})
| - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
| ''withdata'': to many values were returned'
| ...
+box.error.last():unpack()
+ | ---
+ | - - type: ClientError
+ | code: 199
+ | message: 'Key format doesn''t match one defined in functional index ''idx'' of
+ | space ''withdata'': to many values were returned'
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 95
+ | ...
idx:drop()
| ---
| ...
@@ -206,8 +215,24 @@ 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 key definition'
+ | ...
+box.error.last():unpack()
+ | ---
+ | - - type: ClientError
+ | code: 18
+ | message: 'Supplied key type of part 0 does not match index part type: expected
+ | unsigned'
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_def.h
+ | line: 534
+ | - type: ClientError
+ | code: 199
+ | message: 'Key format doesn''t match one defined in functional index ''idx'' of
+ | space ''withdata'': key does not follow functional index key definition'
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 175
| ...
idx:drop()
| ---
@@ -226,8 +251,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 key definition'
| ...
idx:drop()
| ---
@@ -248,6 +272,16 @@ s:insert({1})
| - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
| ''withdata'': a multikey function mustn''t return a scalar'
| ...
+box.error.last():unpack()
+ | ---
+ | - - type: ClientError
+ | code: 199
+ | message: 'Key format doesn''t match one defined in functional index ''idx'' of
+ | space ''withdata'': a multikey function mustn''t return a scalar'
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 109
+ | ...
idx:drop()
| ---
| ...
@@ -273,8 +307,23 @@ 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'
+ | ...
+box.error.last():unpack()
+ | ---
+ | - - type: LuajitError
+ | message: '[string "return function(tuple) local ..."]:1: attempt
+ | to call global ''require'' (a nil value)'
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/lua/utils.c
+ | line: 1000
+ | - type: ClientError
+ | code: 198
+ | message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ | can''t evaluate function'
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 68
| ...
idx:drop()
| ---
diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua
index 372ec800d..587d3c4b1 100644
--- a/test/engine/func_index.test.lua
+++ b/test/engine/func_index.test.lua
@@ -69,6 +69,7 @@ lua_code = [[function(tuple) return {"hello", "world"}, {1, 2} end]]
box.schema.func.create('invalidreturn2', {body = lua_code, is_deterministic = true, is_sandboxed = true, opts = {is_multikey = true}})
idx = s:create_index('idx', {func = box.func.invalidreturn2.id, parts = {{1, 'unsigned'}, {2, 'unsigned'}}})
s:insert({1})
+box.error.last():unpack()
idx:drop()
-- Invalid functional index extractor routine return: the second returned key invalid.
@@ -76,6 +77,7 @@ lua_code = [[function(tuple) return {{"hello", "world"}, {1, 2}} end]]
box.schema.func.create('invalidreturn3', {body = lua_code, is_deterministic = true, is_sandboxed = true, opts = {is_multikey = true}})
idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'unsigned'}, {2, 'unsigned'}}})
s:insert({1})
+box.error.last():unpack()
idx:drop()
-- Invalid functional index extractor routine return: multikey return in case of regular index.
@@ -90,6 +92,7 @@ lua_code = [[function(tuple) return "hello" end]]
box.schema.func.create('invalidreturn5', {body = lua_code, is_deterministic = true, is_sandboxed = true, opts = {is_multikey = true}})
idx = s:create_index('idx', {func = box.func.invalidreturn5.id, parts = {{1, 'unsigned'}, {2, 'unsigned'}}})
s:insert({1})
+box.error.last():unpack()
idx:drop()
-- Invalid function: runtime extractor error
@@ -102,6 +105,7 @@ 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})
+box.error.last():unpack()
idx:drop()
-- Remove old persistent functions
--
2.22.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] [PATCH v1 3/3] box: extend ffi error object API
2019-08-01 11:13 [tarantool-patches] [PATCH v1 0/3] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 2/3] box: stacked diagnostics area in fiber Kirill Shcherbatov
@ 2019-08-01 11:13 ` Kirill Shcherbatov
2019-08-05 21:18 ` [tarantool-patches] " Vladislav Shpilevoy
2 siblings, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2019-08-01 11:13 UTC (permalink / raw)
To: tarantool-patches, v.shpilevoy
Cc: alexander.turenko, kostja, Kirill Shcherbatov
Closes #1148
@TarantoolBot document
Title: Stacked diagnostics area in fiber
Tarantool errors subsystem had been refactored to support stacked
errors. Errors in Tarantool are represented as a
cdata<struct error *> objects with following fields and methods
error.type - get top-level error type
error.message - get top-level error description string
error.trace - get top-level file and line trace for error object
error.code - get top-level error code
error:raise() - raise error exception
error:match() - match a string in error message with given
pattern. Required to uniformly handle string
error messages and cdata error objects:
f1() return nil, "err1" end
f2() return nil, box.error.new(555, "err2") end
_, err1 = f1()
-, err2 = f2()
err1:match("err1") == true
err2:match("err2") == true
-- Methods with stacked error semantics
error:unpack() - return error's detailed stack diagnostics:
a table of errors in their historical order
with all related information
error:has(code)- test whether a given error code is met in
error's stacked representation
err1 =
err1:wrap(err) - add err error object as a reason for err1 object
this call modifies err1 object and doesn't
modify err object.
err1_parent =
err1:unwrap() - remove the most recent error in error object,
return it's parent. the call has no effect when
there is no parent in given error object.
Example:
errors = {ERR_IO = 1001, ERR_USER_CREATE = 1002}
e0 = box.error.new(errors.ERR_IO, 'IO error')
e1 = box.error.new(errors.ERR_USER_CREATE, 'Can\'t create a user')
e1 = e1:wrap(e0)
e1:unpack()
- - type: ClientError
message: Unknown error
code: 1001
trace:
- file: '[string "e0 = box.error.new(errors.ERR_IO, ''IO error'')"]'
line: 1
- type: ClientError
message: Unknown error
code: 1002
trace:
- file: '[string "e1 = box.error.new(errors.ERR_USER_CREATE,
''C..."]'
line: 1
e1:has(errors.ERR_IO)
- true
e1:has(errors.ERR_USER_CREATE)
- true
---
src/box/error.h | 23 ++++
| 2 +
src/box/error.cc | 25 ++++
src/lua/error.lua | 50 ++++++++
test/box/errors.result | 265 +++++++++++++++++++++++++++++++++++++++
test/box/errors.test.lua | 53 ++++++++
6 files changed, 418 insertions(+)
create mode 100644 test/box/errors.result
create mode 100644 test/box/errors.test.lua
diff --git a/src/box/error.h b/src/box/error.h
index b8c7cf73d..f45a0a661 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -85,6 +85,29 @@ box_error_code(const box_error_t *error);
const char *
box_error_message(const box_error_t *error);
+/**
+ * Wrap reason error object into given error.
+ * This API replaces box.error.last() value with an updated
+ * error object.
+ * \param error wrapper error object
+ * \param reason error object
+ * \return a pointer to the updated error object
+ */
+struct error *
+box_error_wrap(box_error_t *error, box_error_t *reason);
+
+/**
+ * Removes the element's parent and returns the
+ * unwrapped reason error object.
+ * This API replases box.error.last() value with an unwrapped
+ * reason reason error object. The original error object is
+ * modified an has no reason anymore.
+ * \param error error object
+ * \return a pointer to the updated error object
+ */
+struct error *
+box_error_unwrap(box_error_t *error);
+
/**
* Get the information about the last API call error.
*
--git a/extra/exports b/extra/exports
index b8c42c0df..17294ed8c 100644
--- a/extra/exports
+++ b/extra/exports
@@ -225,6 +225,8 @@ box_index_count
box_error_type
box_error_code
box_error_message
+box_error_wrap
+box_error_unwrap
box_error_last
box_error_clear
box_error_set
diff --git a/src/box/error.cc b/src/box/error.cc
index 7dfe1b3ee..4ed2a995b 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -57,6 +57,31 @@ box_error_message(const box_error_t *error)
return error->errmsg;
}
+struct error *
+box_error_wrap(box_error_t *error, box_error_t *reason)
+{
+ if (reason == NULL) {
+ diag_set_error(diag_get(), error);
+ return error;
+ }
+ assert(reason != NULL && error->reason == NULL);
+ error_ref(reason);
+ diag_set_error(diag_get(), error);
+ error->reason = reason;
+ return error;
+}
+
+struct error *
+box_error_unwrap(box_error_t *error)
+{
+ struct error *reason = error->reason;
+ assert(reason != NULL);
+ diag_set_error(diag_get(), reason);
+ error_unref(reason);
+ error->reason = NULL;
+ return reason;
+}
+
box_error_t *
box_error_last(void)
{
diff --git a/src/lua/error.lua b/src/lua/error.lua
index 2bb8ccfe0..9b8b22a64 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -31,6 +31,15 @@ char *
exception_get_string(struct error *e, const struct method_info *method);
int
exception_get_int(struct error *e, const struct method_info *method);
+
+typedef struct error box_error_t;
+
+uint32_t
+box_error_code(const box_error_t *error);
+box_error_t *
+box_error_unwrap(box_error_t *error);
+box_error_t *
+box_error_wrap(box_error_t *error, box_error_t *reason);
]]
local REFLECTION_CACHE = {}
@@ -92,6 +101,7 @@ local error_fields = {
["type"] = error_type;
["message"] = error_message;
["trace"] = error_trace;
+ ["code"] = ffi.C.box_error_code;
}
local function error_unpack_one(err)
@@ -126,6 +136,43 @@ local function error_unpack(err)
return result
end
+local function error_has(err, errcode)
+ if not ffi.istype('struct error', err) then
+ error("Usage: error:has()")
+ end
+ while err ~= nil do
+ if ffi.C.box_error_code(err) == errcode then
+ return true
+ end
+ err = err._reason
+ end
+ return false
+end
+
+local function error_wrap(err, reason)
+ if not ffi.istype('struct error', err) then
+ error("Usage: error:wrap()")
+ end
+ if err._reason ~= nil then
+ error("The error:wrap() method is only applicable for "..
+ "errors with no reason defined")
+ end
+ if reason == nil then
+ return err
+ end
+ return ffi.C.box_error_wrap(err, reason)
+end
+
+local function error_unwrap(err)
+ if not ffi.istype('struct error', err) then
+ error("Usage: error:unwrap()")
+ end
+ if err._reason == nil then
+ return err
+ end
+ return ffi.C.box_error_unwrap(err)
+end
+
local function error_raise(err)
if not ffi.istype('struct error', err) then
error("Usage: error:raise()")
@@ -149,6 +196,9 @@ local error_methods = {
["unpack"] = error_unpack;
["raise"] = error_raise;
["match"] = error_match; -- Tarantool 1.6 backward compatibility
+ ["has"] = error_has;
+ ["wrap"] = error_wrap;
+ ["unwrap"] = error_unwrap;
["__serialize"] = error_serialize;
}
diff --git a/test/box/errors.result b/test/box/errors.result
new file mode 100644
index 000000000..b0fb2f38d
--- /dev/null
+++ b/test/box/errors.result
@@ -0,0 +1,265 @@
+-- test-run result file version 2
+env = require('test_run')
+ | ---
+ | ...
+test_run = env.new()
+ | ---
+ | ...
+
+--
+-- gh-1148: Stacked diagnostics area in fiber
+--
+s = box.schema.space.create('withdata')
+ | ---
+ | ...
+pk = s:create_index('pk')
+ | ---
+ | ...
+lua_code = [[function(tuple) return {{"hello", "world"}, {1, 2}} end]]
+ | ---
+ | ...
+box.schema.func.create('invalidreturn3', {body = lua_code, is_deterministic = true, is_sandboxed = true, opts = {is_multikey = true}})
+ | ---
+ | ...
+idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'unsigned'}, {2, 'unsigned'}}})
+ | ---
+ | ...
+tuple, err2 = pcall(box.internal.insert, s.id, {1})
+ | ---
+ | ...
+assert(tuple == false)
+ | ---
+ | - true
+ | ...
+err2:has(box.error.KEY_PART_TYPE)
+ | ---
+ | - true
+ | ...
+err2:has(box.error.FUNC_INDEX_FORMAT)
+ | ---
+ | - true
+ | ...
+err2:has(box.error.PROC_LUA)
+ | ---
+ | - false
+ | ...
+err2.trace
+ | ---
+ | - - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 175
+ | ...
+
+-- Test wrap/unwrap cases and garbage collection
+err3 = box.error.new(box.error.PROC_LUA, "Can't initialize storage")
+ | ---
+ | ...
+err3:wrap(err2)
+ | ---
+ | - Can't initialize storage
+ | ...
+err2:has(box.error.PROC_LUA)
+ | ---
+ | - false
+ | ...
+err4 = box.error.new(box.error.PROC_LUA, "Can't initialize storage 2")
+ | ---
+ | ...
+err4:wrap(nil)
+ | ---
+ | - Can't initialize storage 2
+ | ...
+err4:unpack()
+ | ---
+ | - - type: ClientError
+ | message: Can't initialize storage 2
+ | code: 32
+ | trace:
+ | - file: '[string "err4 = box.error.new(box.error.PROC_LUA, "Can..."]'
+ | line: 1
+ | ...
+err4:wrap(err3)
+ | ---
+ | - Can't initialize storage 2
+ | ...
+err4:wrap(nil)
+ | ---
+ | - error: 'builtin/error.lua:157: The error:wrap() method is only applicable for errors
+ | with no reason defined'
+ | ...
+collectgarbage()
+ | ---
+ | - 0
+ | ...
+box.error.clear()
+ | ---
+ | ...
+err2:unpack()
+ | ---
+ | - - type: ClientError
+ | message: 'Supplied key type of part 0 does not match index part type: expected
+ | unsigned'
+ | code: 18
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_def.h
+ | line: 534
+ | - type: ClientError
+ | message: 'Key format doesn''t match one defined in functional index ''idx'' of
+ | space ''withdata'': key does not follow functional index key definition'
+ | code: 199
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 175
+ | ...
+err3:unpack()
+ | ---
+ | - - type: ClientError
+ | message: 'Supplied key type of part 0 does not match index part type: expected
+ | unsigned'
+ | code: 18
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_def.h
+ | line: 534
+ | - type: ClientError
+ | message: 'Key format doesn''t match one defined in functional index ''idx'' of
+ | space ''withdata'': key does not follow functional index key definition'
+ | code: 199
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 175
+ | - type: ClientError
+ | message: Can't initialize storage
+ | code: 32
+ | trace:
+ | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]'
+ | line: 1
+ | ...
+err4:unpack()
+ | ---
+ | - - type: ClientError
+ | message: 'Supplied key type of part 0 does not match index part type: expected
+ | unsigned'
+ | code: 18
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_def.h
+ | line: 534
+ | - type: ClientError
+ | message: 'Key format doesn''t match one defined in functional index ''idx'' of
+ | space ''withdata'': key does not follow functional index key definition'
+ | code: 199
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 175
+ | - type: ClientError
+ | message: Can't initialize storage
+ | code: 32
+ | trace:
+ | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]'
+ | line: 1
+ | - type: ClientError
+ | message: Can't initialize storage 2
+ | code: 32
+ | trace:
+ | - file: '[string "err4 = box.error.new(box.error.PROC_LUA, "Can..."]'
+ | line: 1
+ | ...
+
+err4 = nil
+ | ---
+ | ...
+collectgarbage()
+ | ---
+ | - 0
+ | ...
+err2:unpack()
+ | ---
+ | - - type: ClientError
+ | message: 'Supplied key type of part 0 does not match index part type: expected
+ | unsigned'
+ | code: 18
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_def.h
+ | line: 534
+ | - type: ClientError
+ | message: 'Key format doesn''t match one defined in functional index ''idx'' of
+ | space ''withdata'': key does not follow functional index key definition'
+ | code: 199
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 175
+ | ...
+err3:unpack()
+ | ---
+ | - - type: ClientError
+ | message: 'Supplied key type of part 0 does not match index part type: expected
+ | unsigned'
+ | code: 18
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_def.h
+ | line: 534
+ | - type: ClientError
+ | message: 'Key format doesn''t match one defined in functional index ''idx'' of
+ | space ''withdata'': key does not follow functional index key definition'
+ | code: 199
+ | trace:
+ | - file: /home/kir/WORK/tarantool/src/box/key_list.c
+ | line: 175
+ | - type: ClientError
+ | message: Can't initialize storage
+ | code: 32
+ | trace:
+ | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]'
+ | line: 1
+ | ...
+
+err3_head = err3
+ | ---
+ | ...
+err3 = err3:unwrap()
+ | ---
+ | ...
+err3 == err2
+ | ---
+ | - true
+ | ...
+box.error.last() == err3
+ | ---
+ | - true
+ | ...
+err3_head:unpack()
+ | ---
+ | - - type: ClientError
+ | message: Can't initialize storage
+ | code: 32
+ | trace:
+ | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]'
+ | line: 1
+ | ...
+err3_head = nil
+ | ---
+ | ...
+collectgarbage()
+ | ---
+ | - 0
+ | ...
+
+box.error.clear()
+ | ---
+ | ...
+err3 = nil
+ | ---
+ | ...
+collectgarbage()
+ | ---
+ | - 0
+ | ...
+err2 = nil
+ | ---
+ | ...
+collectgarbage()
+ | ---
+ | - 0
+ | ...
+
+s:drop()
+ | ---
+ | ...
diff --git a/test/box/errors.test.lua b/test/box/errors.test.lua
new file mode 100644
index 000000000..fac9cfbc2
--- /dev/null
+++ b/test/box/errors.test.lua
@@ -0,0 +1,53 @@
+env = require('test_run')
+test_run = env.new()
+
+--
+-- gh-1148: Stacked diagnostics area in fiber
+--
+s = box.schema.space.create('withdata')
+pk = s:create_index('pk')
+lua_code = [[function(tuple) return {{"hello", "world"}, {1, 2}} end]]
+box.schema.func.create('invalidreturn3', {body = lua_code, is_deterministic = true, is_sandboxed = true, opts = {is_multikey = true}})
+idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'unsigned'}, {2, 'unsigned'}}})
+tuple, err2 = pcall(box.internal.insert, s.id, {1})
+assert(tuple == false)
+err2:has(box.error.KEY_PART_TYPE)
+err2:has(box.error.FUNC_INDEX_FORMAT)
+err2:has(box.error.PROC_LUA)
+err2.trace
+
+-- Test wrap/unwrap cases and garbage collection
+err3 = box.error.new(box.error.PROC_LUA, "Can't initialize storage")
+err3:wrap(err2)
+err2:has(box.error.PROC_LUA)
+err4 = box.error.new(box.error.PROC_LUA, "Can't initialize storage 2")
+err4:wrap(nil)
+err4:unpack()
+err4:wrap(err3)
+err4:wrap(nil)
+collectgarbage()
+box.error.clear()
+err2:unpack()
+err3:unpack()
+err4:unpack()
+
+err4 = nil
+collectgarbage()
+err2:unpack()
+err3:unpack()
+
+err3_head = err3
+err3 = err3:unwrap()
+err3 == err2
+box.error.last() == err3
+err3_head:unpack()
+err3_head = nil
+collectgarbage()
+
+box.error.clear()
+err3 = nil
+collectgarbage()
+err2 = nil
+collectgarbage()
+
+s:drop()
--
2.22.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 2/3] box: stacked diagnostics area in fiber
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 2/3] box: stacked diagnostics area in fiber Kirill Shcherbatov
@ 2019-08-05 21:16 ` Vladislav Shpilevoy
0 siblings, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy @ 2019-08-05 21:16 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov; +Cc: alexander.turenko, kostja
Thanks for the patch! See 2 comments below.
On 01/08/2019 13:13, Kirill Shcherbatov wrote:
> This patch introduces stacked errors. A new API diag_add allows
> to extend an existent error information with a new one error.
1. 'a' assumes singular. You can and usually should omit 'one'.
> The previous state becomes a "reason" of the last-set error.
> Each error object takes a reference to it's reason error object.
>
> The :unpack() method patched correspondingly to display the whole
> errors trace.
>
> Part of #1148
> ---
> diff --git a/test/engine/func_index.result b/test/engine/func_index.result
> index 877b76d5e..ae8873c9b 100644
> --- a/test/engine/func_index.result
> +++ b/test/engine/func_index.result
> @@ -189,6 +188,16 @@ s:insert({1})
> | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space
> | ''withdata'': to many values were returned'
> | ...
> +box.error.last():unpack()
> + | ---
> + | - - type: ClientError
> + | code: 199
> + | message: 'Key format doesn''t match one defined in functional index ''idx'' of
> + | space ''withdata'': to many values were returned'
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + | line: 95
2. Firstly, if you change something on the branch, then please, write about in an email.
On the branch I see that you added filters. Lets filter out the line as well. Otherwise
that test will fail each time we add or delete any of first 95 lines of key_list.c.
> + | ...
> idx:drop()
> | ---
> | ...
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
@ 2019-08-05 21:16 ` Vladislav Shpilevoy
[not found] ` <06bd2140-3d2b-4bc3-7bc4-5f3d293bf891@tarantool.org>
2019-08-07 23:27 ` Alexander Turenko
1 sibling, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy @ 2019-08-05 21:16 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov; +Cc: alexander.turenko, kostja
Hi! Thanks for the patch!
See 9 comments below.
On 01/08/2019 13:13, Kirill Shcherbatov wrote:
> Part of #1148
> ---
> doc/rfc/1148-stacked-diagnostics.md | 136 ++++++++++++++++++++++++++++
> 1 file changed, 136 insertions(+)
> create mode 100644 doc/rfc/1148-stacked-diagnostics.md
>
> diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
> new file mode 100644
> index 000000000..a007491aa
> --- /dev/null
> +++ b/doc/rfc/1148-stacked-diagnostics.md
> @@ -0,0 +1,136 @@
> +# Stacked Diagnostics in Tarantool
> +
> +* **Status**: In progress
> +* **Start date**: 30-08-2019
1. Today is 5 August. I guess it is a typo.
> +* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org, Konstantin Osipov @kostja kostja@tarantool.org, Georgy Kirichenko @GeorgyKirichenko georgy@tarantool.org, @Totktonada Alexander Turenko alexander.turenko@tarantool.org
> +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
> +
> +## Summary
> +Support stacked diagnostics for Tarantool allows to accumulate all occurred errors during processing a request. This allows to better understand what has happened and handle errors
> +correspondingly.
> +
> +## Background and motivation
> +
> +Tarantool statements must produce diagnostic information that populates the diagnostics area. This is a Standard SQL requirement and other vendors and languages also have such feature.
> +Diagnostics area stack must contain a diagnostics area for each nested execution context.
> +
> +### Current Tarantool's error diagnostics
> +Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
> +The last error is exported with `box.error.last()` endpoint.
> +
> +In total there are 5 error classes in Tarantool.
> Each error class is associated with set of error codes (@see ClientError::get_errcode(const struct error *e)).
> +```
> +ClientError -> ClientError::errcode() -> ...;
> +OutOfMemory -> MEMORY_ISSUE
> +SystemError -> SYSTEM
> +CollationError -> CANT_CREATE_COLLATION
> +Lua error -> PROC_LUA
> +```
> +All system-defined errors are exported with their unique error codes in box.error folder by NAME.
> +
> +You may use box.error.new endpoint to create a new error instance of predefined type:
> +```
> +tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't")
> +tarantool> t:unpack()
> +---
> +- type: ClientError
> + code: 9
> + message: 'Failed to create space ''myspace'': just I can''t'
> + trace:
> + - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
> + line: 1
> +```
> +
> +User is allowed to define own errors with any code (including system errors) with
> +```
> +box.error.new{code = user_code, reason = user_error_msg}
> +```
> +where `user_code` must be greater that the biggest registered system error, say `10000`.
2. 'greater than'.
> +
> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()` methods and uniform `.type`, `.message` and `.trace` fields.
> +
> +
> +## Proposal
> +In some cases a diagnostic information must be more complicated.
> +For example, when persistent Lua function referenced by functional index has a bug in it's definition, Lua handler sets an diag message. Then functional index extractor code sets an own, more specialized error.
> +
> +In this and similar situations, we need stack-based error diagnostics:
> +
> +- The serialize method should return the most recent error in stack:
> +```
> +tarantool> box.error.last()
> +Failed to build a key for functional index ''idx'' of space ''withdata': function error
> +```
> +
> +- The `:unpack()` method must return the complete diagnostics stack - a table of errors sorted in their historical order:
> +```
> +tarantool> box.error.last():unpack()
> +---
> + - type: LuajitError
> + message: '[string "return function(tuple)..."]:2: attempt to call global
> + ''require'' (a nil value)'
> + trace:
> + - file: /home/kir/WORK/tarantool/src/lua/utils.c
> + line: 1000
> + - type: ClientError
> + code: 198
> + message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
> + function error
> + trace:
> + - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + line: 68
> +```
> +
3. VShard uses unpack: https://github.com/tarantool/vshard/blob/a6826b26f9ab38e338a34cb7dd7f46dabbc67af5/vshard/error.lua#L149
Is it compatible with what you are doing? Here I need to turn an error into a table to
set a different serialization method.
4. Have you considered an idea to unpack errors not as an array, but
as a list? When each error object has a field 'reason' pointing to
another object, and so on. It would allow not to change unpack() output
format - it would still return the newest error, but with a new field.
> +- We need to introduce `next_error:wrap(error)` and `error:unwrap()` methods.
> +The `wrap` function can take any previous and last error objects to append error to the end of next_error trace. The next_error backtrace is expected to be empty.
> +The `unwrap` function removes the element's parent and returns the unwrapped content. This is effectively the inverse of the `wrap()` method.
> +```
> +Workaround:
5. Workaround for what?
> +
> +function create_new_user()
> + name, err = pcall(box.internal.func_call,
> + box.func.generate_user_name.name)
> + if not name then
> + local e = box.error.new({
> + code = APP_ERROR_CODE,
> + reason = "create_new_user"}):
> + e:wrap(err):raise()
> + end
> + box.schema.user.create(name)
> +end
> +```
> +
> +- We also need `error:has` method to test whether given error object contain given error code:
6. 'object contains'.
> +```
> +box.error.last():has(box.error.LuajitError)
7. Something is wrong.
tarantool> box.error.LuajitError
---
- null
...
> +```
> +
> +## Detailed design
> +
> +Diagnostic area object has a good encapsulated API.
> +
> +Lets extend error object with a pointer to it's reason error and introduce a few new methods (we need to rename diag_add_error to diag_set_error because it replaces an existent diag error in fact):
> +```
> +struct error {
> +...
> +/* A pointer to the reason error. */
8. Broken alignment.
> + struct error *reason;
> +};
> +
> +/**
> + * Add a new error to the diagnostics area: the previous error
> + * becomes a reason of a current.
> + * \param diag diagnostics area
> + * \param e error to add
> + */
> +static inline void
> +diag_add_error(struct diag *diag, struct error *e);
> +
> +/** Same as diag_set, but softly extend diagnostic information. */
> +#define diag_add(class, ...)
> +```
> +
> +### IPROTO Communication
> +Currently errors are sent as (IPROTO_TYPE_ERROR | errcode) messages with error message string payload.
> +Lets introduce a new IPROTO_TYPE_ERROR_TRACE error code with
> +[[errcode1, "errstring1"], [errcode2, "errstring2"]..] payload, where errors are serialized in historical order.
>
9. Please, describe the new binary protocol in the similar way as it
is done here: https://github.com/tarantool/tarantool/blob/master/doc/rfc/3328-wire_protocol.md#body
And add it to the docbot request with code values included.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 3/3] box: extend ffi error object API Kirill Shcherbatov
@ 2019-08-05 21:18 ` Vladislav Shpilevoy
2019-08-06 7:56 ` Kirill Shcherbatov
2019-08-08 23:33 ` Alexander Turenko
0 siblings, 2 replies; 17+ messages in thread
From: Vladislav Shpilevoy @ 2019-08-05 21:18 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov; +Cc: alexander.turenko, kostja
Thanks for the patch! See 11 comments below.
On 01/08/2019 13:13, Kirill Shcherbatov wrote:
> Closes #1148
>
> @TarantoolBot document
> Title: Stacked diagnostics area in fiber
>
> Tarantool errors subsystem had been refactored to support stacked
1. 'error subsystem'.
2. 'has been'.
> errors. Errors in Tarantool are represented as a
> cdata<struct error *> objects with following fields and methods
>
> error.type - get top-level error type
> error.message - get top-level error description string
> error.trace - get top-level file and line trace for error object
> error.code - get top-level error code
>
> error:raise() - raise error exception
> error:match() - match a string in error message with given
> pattern. Required to uniformly handle string
> error messages and cdata error objects:
> f1() return nil, "err1" end
> f2() return nil, box.error.new(555, "err2") end
> _, err1 = f1()
> -, err2 = f2()
> err1:match("err1") == true
> err2:match("err2") == true
>
> -- Methods with stacked error semantics
> error:unpack() - return error's detailed stack diagnostics:
> a table of errors in their historical order
> with all related information
> error:has(code)- test whether a given error code is met in
> error's stacked representation
> err1 =
> err1:wrap(err) - add err error object as a reason for err1 object
> this call modifies err1 object and doesn't
> modify err object.
3. If it modifies err1, then why do I need to assign a result to
err1? The same below.
> err1_parent =
> err1:unwrap() - remove the most recent error in error object,
> return it's parent. the call has no effect when
> there is no parent in given error object.
>
> Example:
> errors = {ERR_IO = 1001, ERR_USER_CREATE = 1002}
> e0 = box.error.new(errors.ERR_IO, 'IO error')
> e1 = box.error.new(errors.ERR_USER_CREATE, 'Can\'t create a user')
> e1 = e1:wrap(e0)
> e1:unpack()
> - - type: ClientError
> message: Unknown error
> code: 1001
> trace:
> - file: '[string "e0 = box.error.new(errors.ERR_IO, ''IO error'')"]'
> line: 1
> - type: ClientError
> message: Unknown error
> code: 1002
> trace:
> - file: '[string "e1 = box.error.new(errors.ERR_USER_CREATE,
> ''C..."]'
> line: 1
> e1:has(errors.ERR_IO)
> - true
> e1:has(errors.ERR_USER_CREATE)
> - true
> ---
> diff --git a/src/box/error.h b/src/box/error.h
> index b8c7cf73d..f45a0a661 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -85,6 +85,29 @@ box_error_code(const box_error_t *error);
> const char *
> box_error_message(const box_error_t *error);
>
> +/**
> + * Wrap reason error object into given error.
> + * This API replaces box.error.last() value with an updated
> + * error object.
4. Why do you need to change the global error? And why is not
it mentioned in the docbot request?
> + * \param error wrapper error object
> + * \param reason error object
> + * \return a pointer to the updated error object
> + */
> +struct error *
> +box_error_wrap(box_error_t *error, box_error_t *reason);
> +
> +/**
> + * Removes the element's parent and returns the
> + * unwrapped reason error object.
> + * This API replases box.error.last() value with an unwrapped
5. 'replaces'.
> + * reason reason error object. The original error object is
> + * modified an has no reason anymore.
> + * \param error error object
> + * \return a pointer to the updated error object
> + */
> +struct error *
> +box_error_unwrap(box_error_t *error);
> +
> /**
> * Get the information about the last API call error.
> *
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 7dfe1b3ee..4ed2a995b 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -57,6 +57,31 @@ box_error_message(const box_error_t *error)
> return error->errmsg;
> }
>
> +struct error *
> +box_error_wrap(box_error_t *error, box_error_t *reason)
> +{
> + if (reason == NULL) {
> + diag_set_error(diag_get(), error);
> + return error;
> + }
> + assert(reason != NULL && error->reason == NULL);
> + error_ref(reason);
> + diag_set_error(diag_get(), error);
> + error->reason = reason;
> + return error;
> +}
> +
> +struct error *
6. Please, return box_error_t * here and above.
> +box_error_unwrap(box_error_t *error)
> +{
> + struct error *reason = error->reason;
> + assert(reason != NULL);
> + diag_set_error(diag_get(), reason);
> + error_unref(reason);
> + error->reason = NULL;
> + return reason;
7. Unwrap does not allow to unwrap a leaf error.
But there is no API to determine if the error is
leaf. So a user can't determine when to stop calling
unwrap.
I am talking about C public API which you have changed
here. A user can't check error->reason != NULL before
calling box_error_unwrap.
Moreover, it is inconsistent with Lua version. Lets
better return the argument when error->reason == NULL
in box_error_unwrap. Then a user of the C API would
just unwrap the stack until box_error_unwrap(e) == e.
Also it simplifies Lua version implementation.
> +}
> +
> box_error_t *
> box_error_last(void)
> {
> diff --git a/test/box/errors.result b/test/box/errors.result
> new file mode 100644
> index 000000000..b0fb2f38d
> --- /dev/null
> +++ b/test/box/errors.result
> @@ -0,0 +1,265 @@
> +-- test-run result file version 2
> +env = require('test_run')
> + | ---
> + | ...
> +test_run = env.new()
> + | ---
> + | ...
> +
8. On the branch here is a filter. Please, add a filter for
line numbers too.
> +--
> +-- gh-1148: Stacked diagnostics area in fiber
> +--
> +s = box.schema.space.create('withdata')
> + | ---
> + | ...
> +pk = s:create_index('pk')
> + | ---
> + | ...
> +lua_code = [[function(tuple) return {{"hello", "world"}, {1, 2}} end]]
> + | ---
> + | ...
> +box.schema.func.create('invalidreturn3', {body = lua_code, is_deterministic = true, is_sandboxed = true, opts = {is_multikey = true}})
> + | ---
> + | ...
> +idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'unsigned'}, {2, 'unsigned'}}})
> + | ---
> + | ...
> +tuple, err2 = pcall(box.internal.insert, s.id, {1})
> + | ---
> + | ...
> +assert(tuple == false)
> + | ---
> + | - true
> + | ...
> +err2:has(box.error.KEY_PART_TYPE)
> + | ---
> + | - true
> + | ...
> +err2:has(box.error.FUNC_INDEX_FORMAT)
> + | ---
> + | - true
> + | ...
> +err2:has(box.error.PROC_LUA)
> + | ---
> + | - false
> + | ...
> +err2.trace
> + | ---
> + | - - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + | line: 175
> + | ...
> +
> +-- Test wrap/unwrap cases and garbage collection
> +err3 = box.error.new(box.error.PROC_LUA, "Can't initialize storage")
> + | ---
> + | ...
> +err3:wrap(err2)
> + | ---
> + | - Can't initialize storage
> + | ...
> +err2:has(box.error.PROC_LUA)
> + | ---
> + | - false
> + | ...
> +err4 = box.error.new(box.error.PROC_LUA, "Can't initialize storage 2")
> + | ---
> + | ...
> +err4:wrap(nil)
> + | ---
> + | - Can't initialize storage 2
> + | ...
> +err4:unpack()
> + | ---
> + | - - type: ClientError
> + | message: Can't initialize storage 2
> + | code: 32
> + | trace:
> + | - file: '[string "err4 = box.error.new(box.error.PROC_LUA, "Can..."]'
> + | line: 1
> + | ...
> +err4:wrap(err3)
> + | ---
> + | - Can't initialize storage 2
> + | ...
> +err4:wrap(nil)
> + | ---
> + | - error: 'builtin/error.lua:157: The error:wrap() method is only applicable for errors
> + | with no reason defined'
9. This filename is not filtered on the branch.
> + | ...
> +collectgarbage()
> + | ---
> + | - 0
> + | ...
> +box.error.clear()
> + | ---
> + | ...
> +err2:unpack()
> + | ---
> + | - - type: ClientError
> + | message: 'Supplied key type of part 0 does not match index part type: expected
> + | unsigned'
> + | code: 18
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_def.h
> + | line: 534
> + | - type: ClientError
> + | message: 'Key format doesn''t match one defined in functional index ''idx'' of
> + | space ''withdata'': key does not follow functional index key definition'
> + | code: 199
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + | line: 175
> + | ...
> +err3:unpack()
> + | ---
> + | - - type: ClientError
> + | message: 'Supplied key type of part 0 does not match index part type: expected
> + | unsigned'
> + | code: 18
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_def.h
> + | line: 534
> + | - type: ClientError
> + | message: 'Key format doesn''t match one defined in functional index ''idx'' of
> + | space ''withdata'': key does not follow functional index key definition'
> + | code: 199
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + | line: 175
> + | - type: ClientError
> + | message: Can't initialize storage
> + | code: 32
> + | trace:
> + | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]'
> + | line: 1
> + | ...
> +err4:unpack()
> + | ---
> + | - - type: ClientError
> + | message: 'Supplied key type of part 0 does not match index part type: expected
> + | unsigned'
> + | code: 18
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_def.h
> + | line: 534
> + | - type: ClientError
> + | message: 'Key format doesn''t match one defined in functional index ''idx'' of
> + | space ''withdata'': key does not follow functional index key definition'
> + | code: 199
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + | line: 175
> + | - type: ClientError
> + | message: Can't initialize storage
> + | code: 32
> + | trace:
> + | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]'
> + | line: 1
> + | - type: ClientError
> + | message: Can't initialize storage 2
> + | code: 32
> + | trace:
> + | - file: '[string "err4 = box.error.new(box.error.PROC_LUA, "Can..."]'
> + | line: 1
> + | ...
> +
> +err4 = nil
> + | ---
> + | ...
> +collectgarbage()
> + | ---
> + | - 0
> + | ...
> +err2:unpack()
> + | ---
> + | - - type: ClientError
> + | message: 'Supplied key type of part 0 does not match index part type: expected
> + | unsigned'
> + | code: 18
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_def.h
> + | line: 534
> + | - type: ClientError
> + | message: 'Key format doesn''t match one defined in functional index ''idx'' of
> + | space ''withdata'': key does not follow functional index key definition'
> + | code: 199
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + | line: 175
> + | ...
> +err3:unpack()
> + | ---
> + | - - type: ClientError
> + | message: 'Supplied key type of part 0 does not match index part type: expected
> + | unsigned'
> + | code: 18
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_def.h
> + | line: 534
> + | - type: ClientError
> + | message: 'Key format doesn''t match one defined in functional index ''idx'' of
> + | space ''withdata'': key does not follow functional index key definition'
> + | code: 199
> + | trace:
> + | - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + | line: 175
> + | - type: ClientError
> + | message: Can't initialize storage
> + | code: 32
> + | trace:
> + | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]'
> + | line: 1
> + | ...
> +
> +err3_head = err3
> + | ---
> + | ...
> +err3 = err3:unwrap()
> + | ---
> + | ...
> +err3 == err2
> + | ---
> + | - true
> + | ...
> +box.error.last() == err3
> + | ---
> + | - true
> + | ...
> +err3_head:unpack()
> + | ---
> + | - - type: ClientError
> + | message: Can't initialize storage
> + | code: 32
> + | trace:
> + | - file: '[string "err3 = box.error.new(box.error.PROC_LUA, "Can..."]'
> + | line: 1
> + | ...
> +err3_head = nil
> + | ---
> + | ...
> +collectgarbage()
> + | ---
> + | - 0
> + | ...
> +
> +box.error.clear()
> + | ---
> + | ...
> +err3 = nil
> + | ---
> + | ...
> +collectgarbage()
> + | ---
> + | - 0
> + | ...
> +err2 = nil
> + | ---
> + | ...
> +collectgarbage()
> + | ---
> + | - 0
> + | ...
10. Nit: you could nullify all the errors at once, and call
collectgarbage.
> +
> +s:drop()
> + | ---
> + | ...
11. In the RFC you said, that IProto returns a list of error. Where
it is?
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API
2019-08-05 21:18 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-08-06 7:56 ` Kirill Shcherbatov
2019-08-06 20:50 ` Vladislav Shpilevoy
2019-08-08 23:33 ` Alexander Turenko
1 sibling, 1 reply; 17+ messages in thread
From: Kirill Shcherbatov @ 2019-08-06 7:56 UTC (permalink / raw)
To: Vladislav Shpilevoy, Tarantool MailList
I'll make review fixes and I'll send a corresponding letter later.Now I'd like to discuss a few unclear moments.
>> err1 =
>> err1:wrap(err) - add err error object as a reason for err1 object
>> this call modifies err1 object and doesn't
>> modify err object.
>
>> err1_parent =
>> err1:unwrap() - remove the most recent error in error object,
>> return it's parent. the call has no effect when
>> there is no parent in given error object.
> 3. If it modifies err1, then why do I need to assign a result to
> err1? The same below.
Let's start discussing this question with second :unwrap method.
At first, some Lua variables point to error object (sic: not diag area);
Therefore we need a way to return an unwrapped parent object to user.
The implemented error:unwrap() method modifies an original error
object(removing it's parent) and returns it's parent object.
next_err = err:unpack()
To make API consistent, I also return an error object in
error:wrap(reason) method. This is the only reason for :wrap.
We may get rid of it, if it is your strong opinion.
>> +/**
>> + * Wrap reason error object into given error.
>> + * This API replaces box.error.last() value with an updated
>> + * error object.
>
> 4. Why do you need to change the global error? And why is not
> it mentioned in the docbot request?
In many details my motivation is similar with (3.)th block:
to make my API consistent.
It is really important to enforce something taking a reference to parent
error before unref(ing) it for self (for :unwrap) object. We would like to return reason for
user, right? But self object make have the last reference to it. The delete method
mustn't be called. Partially :wrap and :unwrap operations are constructors that introduce
a new error. So changing box.error.last() seems for me reasonable.
> 7. Unwrap does not allow to unwrap a leaf error.
> But there is no API to determine if the error is
> leaf. So a user can't determine when to stop calling
> unwrap.>
> I am talking about C public API which you have changed
> here. A user can't check error->reason != NULL before
> calling box_error_unwrap.
I don't mind: I've had a draft with such implementation.
Let's do it so.
>> +err2 = nil
>> + | ---
>> + | ...
>> +collectgarbage()
>> + | ---
>> + | - 0
>> + | ...
>
> 10. Nit: you could nullify all the errors at once, and call
> collectgarbage.
>What do you mean?
I consciously clean up the errors and call the garbage collector in these places.
If you put an extra printf in error_unref/destructor you'll see why this is important.
(also see your 4th question - this is a coverage tests for this problem)
>> +
>> +s:drop()
>> + | ---
>> + | ...
> 11. In the RFC you said, that IProto returns a list of error. Where
> it is?
I haven't implemented this yet. Kostya said that we make do it later.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API
2019-08-06 7:56 ` Kirill Shcherbatov
@ 2019-08-06 20:50 ` Vladislav Shpilevoy
0 siblings, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy @ 2019-08-06 20:50 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov
Thanks for the answers!
There is a bug:
e1 = box.error.new({code = 123, reason = 'test'})
e2 = box.error.new({code = 123, reason = 'test2'})
e3 = box.error.new({code = 123, reason = 'test3'})
e2:wrap(e1)
e3:wrap(e2)
e1:wrap(e3)
e1:unpack()
This test leads to an infinite loop.
On 06/08/2019 09:56, Kirill Shcherbatov wrote:
> I'll make review fixes and I'll send a corresponding letter later.Now I'd like to discuss a few unclear moments.
>
>>> err1 =
>>> err1:wrap(err) - add err error object as a reason for err1 object
>>> this call modifies err1 object and doesn't
>>> modify err object.
>>
>>> err1_parent =
>>> err1:unwrap() - remove the most recent error in error object,
>>> return it's parent. the call has no effect when
>>> there is no parent in given error object.
>> 3. If it modifies err1, then why do I need to assign a result to
>> err1? The same below.
>
> Let's start discussing this question with second :unwrap method.
> At first, some Lua variables point to error object (sic: not diag area);
> Therefore we need a way to return an unwrapped parent object to user.
> The implemented error:unwrap() method modifies an original error
> object(removing it's parent) and returns it's parent object.
> next_err = err:unpack()
'unpack'? It does not return a next error, it returns an array of
errors now. What am I missing?
If you are talking about 'unwrap', then ok, why do I need to
modify the error object at all? As I understand, 'unwrap' just
returns the reason object. How does it require to modify the
original error? Also, looks like I can't unwrap an error stack
more than once because of that - I need to build the stack
back, if I want to have several hooks processing the stack.
Currently I don't have that problem with 'unpack' - I can call it
multiple times on one error object. But I can't call 'unwrap'
multiple times.
> To make API consistent, I also return an error object in
> error:wrap(reason) method. This is the only reason for :wrap.
> We may get rid of it, if it is your strong opinion.
No, for me it is already clear, that unwrap should return an
error, and consistency is a good point. Lets keep 'wrap'
returning an error too. It is worth mentioning in the doc
request.
>>> +/**
>>> + * Wrap reason error object into given error.
>>> + * This API replaces box.error.last() value with an updated
>>> + * error object.
>>
>> 4. Why do you need to change the global error? And why is not
>> it mentioned in the docbot request?
> In many details my motivation is similar with (3.)th block:
> to make my API consistent.
Consistent with what? We've never had a similar function.
The only function, setting an error, was box_error_set. All
other functions were just returning error meta.
>
> It is really important to enforce something taking a reference to parent
> error before unref(ing) it for self (for :unwrap) object. We would like to return reason for
> user, right? But self object make have the last reference to it. The delete method
> mustn't be called.
Not sure if I understood the sentences. What is a problem with
references? It is Lua, it manages references for you.
> Partially :wrap and :unwrap operations are constructors that introduce
> a new error.
Why 'partially'? What is missing?
> So changing box.error.last() seems for me reasonable.
In fact, there is another problem which you are trying to solve
with hacking behaviour of wrap and unwrap functions. The problem
is that neither C nor Lua API provide a way to add an error object
to the current one. They are missing 'diag_add'. You substituted
diag_add with wrap() changing the global diagnostics area. Although
perhaps it is ok. I don't think it would be better for users to
call box.error.add(box.error.wrap(reason)), so lets keep as is now.
TLDR: never mind.
>> 7. Unwrap does not allow to unwrap a leaf error.
>> But there is no API to determine if the error is
>> leaf. So a user can't determine when to stop calling
>> unwrap.>
>> I am talking about C public API which you have changed
>> here. A user can't check error->reason != NULL before
>> calling box_error_unwrap.
> I don't mind: I've had a draft with such implementation.
> Let's do it so.
>
>>> +err2 = nil
>>> + | ---
>>> + | ...
>>> +collectgarbage()
>>> + | ---
>>> + | - 0
>>> + | ...
>>
>> 10. Nit: you could nullify all the errors at once, and call
>> collectgarbage.
>> What do you mean?
> I consciously clean up the errors and call the garbage collector in these places.
> If you put an extra printf in error_unref/destructor you'll see why this is important.
Not sure if it is a good way to help with understanding tests. Please,
describe here the problem, in a comment.
> (also see your 4th question - this is a coverage tests for this problem)
I don't understand the problem in 4th point. Lua keeps references for you
until any singe ref is gone. Error object can't be deleted until there is
a reference on it.
>>> +
>>> +s:drop()
>>> + | ---
>>> + | ...
>> 11. In the RFC you said, that IProto returns a list of error. Where
>> it is?
> I haven't implemented this yet. Kostya said that we make do it later.
>
Is there an issue about that?
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool
[not found] ` <06bd2140-3d2b-4bc3-7bc4-5f3d293bf891@tarantool.org>
@ 2019-08-06 20:50 ` Vladislav Shpilevoy
0 siblings, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy @ 2019-08-06 20:50 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
Hi! Thanks for the answers!
CCed tarantool-patches back.
On 06/08/2019 10:05, Kirill Shcherbatov wrote:
>> 3. VShard uses unpack: https://github.com/tarantool/vshard/blob/a6826b26f9ab38e338a34cb7dd7f46dabbc67af5/vshard/error.lua#L149
>> Is it compatible with what you are doing? Here I need to turn an error into a table to
>> set a different serialization method.
>>
>> 4. Have you considered an idea to unpack errors not as an array, but
>> as a list? When each error object has a field 'reason' pointing to
>> another object, and so on. It would allow not to change unpack() output
>> format - it would still return the newest error, but with a new field.
>
> I designed :unpack() method to produce an output similar to backtrace.
> But maybe it is not really our priority and taking into account your (3)th comment
> we should better use reason-based list.
Please, ask other people.
>> 9. Please, describe the new binary protocol in the similar way as it
>> is done here: https://github.com/tarantool/tarantool/blob/master/doc/rfc/3328-wire_protocol.md#body
>> And add it to the docbot request with code values included.
>
> Is I sad in other letter, designing of iproto errors transfer may not be implemented in scope of this
> patch series. Maybe it worth to remove this paragraph from RFC at all.
>
Ok, then remove, please, or move to a section 'plans', 'alternatives', etc.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
2019-08-05 21:16 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-08-07 23:27 ` Alexander Turenko
2019-08-08 20:46 ` Vladislav Shpilevoy
1 sibling, 1 reply; 17+ messages in thread
From: Alexander Turenko @ 2019-08-07 23:27 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches, v.shpilevoy, kostja
Thanks for the document, the question is interesting.
There are three 'objects' we interested in this discussion: diagnostics
area, struct error and cdata<struct error> in Lua. Your document jumps
over all three objects and it is hard to understand what exactly you
propose.
I mean: what fields and functions are proposed to add for struct diag,
struct error and the Lua part? It would be easier to understand the
proposal if API changes will be split for those three parts and will be
described in an api-reference manner.
It also seems that you want to handle some part of #4398 ('Expose
box.error to be used by applications'). I propose to don't do it here.
I had the objection against a list of errors in the diagnostic area: a
user may want to save an error, perform some new commands, which can
rewrite a last diagnostic (replace the diagnostic list) and then (s)he
may want to look at the stack of reasons of the saved error. But now it
seems you discarded the idea. (BTW, it would be good to briefly mention
changes you made for a new revision of your document: the text looks
similar and it is hard to find differences.)
I think that struct error should have a pointer to its cause, which is
another error.
I like Java terminology here: cause is exactly what we want to save for
an error. Wrap / unwrap (Go terminology) look too abstract and maybe
even unclear for me. Maybe this terms appears from merry / xerror
packages, which were inspired to wrap built-in errors with additional
information (say, stack traces). It is up to maintainers, but my vote is
for 'cause' field, 'get_cause()' and 'set_cause()' functions (maybe with
some prefix, because it is C).
'Is' method (Go terminology, here it is named 'has') also don't look
self-descriptive enough for me. I would name it 'find_cause()'.
Despite I propose to concentrate changes on struct error and its Lua
counterpart, I think that it worth to add a helper to append an error to
the diagnostic area and set a previous last error as its cause. I would
name it 'diag_append()', but I don't have strict objections against
'diag_add()'. The former just looks a bit more intuitive for me.
BTW, it would be good to link documents you are lean on: I guess
https://go.googlesource.com/proposal/+/master/design/29934-error-values.md
is good candidate if you want to stick with Go terms. It also would be
good to mention stacked diagnostics in SQL and some Java reference /
article if you will agree with Java terms.
I doubt a bit that iproto change you proposed would be
backward-compatible: imagine that you connect from an old tarantool to a
new tarantool using net.box or handle errors using another existing
connector. I would send those messages as before, but add a field with
IPROTO_ERROR_LIST key in addition to existing IPROTO_ERROR (0x31) to a
body map. The value can be a list of maps like {code = <mp_uint>,
message = <mp_str>} or a list of list [<code>, <message>]. The
IPROTO_ERROR_LIST should start from a last (top) error I think.
Maybe we should use 'warnings' term here if this feature is intended to
be used for SQL warnings. If we want to use the proposed mechanics for
warnings, then my proposal re using 'cause' term looks doubtful. Don't
sure whether we should introduce some kind of warnings list for the
diagnostic area or reuse 'cause' / 'parent' / ... field of struct error.
I didn't look at your patch: I want to agree on terms and overall
approach first. At least one of maintainers should approve it.
WBR, Alexander Turenko.
On Thu, Aug 01, 2019 at 02:13:26PM +0300, Kirill Shcherbatov wrote:
> Part of #1148
> ---
> doc/rfc/1148-stacked-diagnostics.md | 136 ++++++++++++++++++++++++++++
> 1 file changed, 136 insertions(+)
> create mode 100644 doc/rfc/1148-stacked-diagnostics.md
>
> diff --git a/doc/rfc/1148-stacked-diagnostics.md b/doc/rfc/1148-stacked-diagnostics.md
> new file mode 100644
> index 000000000..a007491aa
> --- /dev/null
> +++ b/doc/rfc/1148-stacked-diagnostics.md
> @@ -0,0 +1,136 @@
> +# Stacked Diagnostics in Tarantool
> +
> +* **Status**: In progress
> +* **Start date**: 30-08-2019
> +* **Authors**: Kirill Shcherbatov @kshcherbatov kshcherbatov@tarantool.org, Konstantin Osipov @kostja kostja@tarantool.org, Georgy Kirichenko @GeorgyKirichenko georgy@tarantool.org, @Totktonada Alexander Turenko alexander.turenko@tarantool.org
> +* **Issues**: [#1148](https://github.com/tarantool/<repository\>/issues/1148)
> +
> +## Summary
> +Support stacked diagnostics for Tarantool allows to accumulate all occurred errors during processing a request. This allows to better understand what has happened and handle errors
> +correspondingly.
> +
> +## Background and motivation
> +
> +Tarantool statements must produce diagnostic information that populates the diagnostics area. This is a Standard SQL requirement and other vendors and languages also have such feature.
> +Diagnostics area stack must contain a diagnostics area for each nested execution context.
> +
> +### Current Tarantool's error diagnostics
> +Currently Tarantool has `diag_set()` mechanism to set a diagnostic error.
> +The last error is exported with `box.error.last()` endpoint.
> +
> +In total there are 5 error classes in Tarantool. Each error class is associated with set of error codes (@see ClientError::get_errcode(const struct error *e)).
> +```
> +ClientError -> ClientError::errcode() -> ...;
> +OutOfMemory -> MEMORY_ISSUE
> +SystemError -> SYSTEM
> +CollationError -> CANT_CREATE_COLLATION
> +Lua error -> PROC_LUA
> +```
> +All system-defined errors are exported with their unique error codes in box.error folder by NAME.
> +
> +You may use box.error.new endpoint to create a new error instance of predefined type:
> +```
> +tarantool> t = box.error.new(box.error.CREATE_SPACE, "myspace", "just I can't")
> +tarantool> t:unpack()
> +---
> +- type: ClientError
> + code: 9
> + message: 'Failed to create space ''myspace'': just I can''t'
> + trace:
> + - file: '[string "t = box.error.new(box.error.CREATE_SPACE, "my..."]'
> + line: 1
> +```
> +
> +User is allowed to define own errors with any code (including system errors) with
> +```
> +box.error.new{code = user_code, reason = user_error_msg}
> +```
> +where `user_code` must be greater that the biggest registered system error, say `10000`.
> +
> +Error cdata object has `:unpack()`, `:raise()`, `:match(...)`, `:__serialize()` methods and uniform `.type`, `.message` and `.trace` fields.
> +
> +
> +## Proposal
> +In some cases a diagnostic information must be more complicated.
> +For example, when persistent Lua function referenced by functional index has a bug in it's definition, Lua handler sets an diag message. Then functional index extractor code sets an own, more specialized error.
> +
> +In this and similar situations, we need stack-based error diagnostics:
> +
> +- The serialize method should return the most recent error in stack:
> +```
> +tarantool> box.error.last()
> +Failed to build a key for functional index ''idx'' of space ''withdata': function error
> +```
> +
> +- The `:unpack()` method must return the complete diagnostics stack - a table of errors sorted in their historical order:
> +```
> +tarantool> box.error.last():unpack()
> +---
> + - type: LuajitError
> + message: '[string "return function(tuple)..."]:2: attempt to call global
> + ''require'' (a nil value)'
> + trace:
> + - file: /home/kir/WORK/tarantool/src/lua/utils.c
> + line: 1000
> + - type: ClientError
> + code: 198
> + message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
> + function error
> + trace:
> + - file: /home/kir/WORK/tarantool/src/box/key_list.c
> + line: 68
> +```
> +
> +- We need to introduce `next_error:wrap(error)` and `error:unwrap()` methods.
> +The `wrap` function can take any previous and last error objects to append error to the end of next_error trace. The next_error backtrace is expected to be empty.
> +The `unwrap` function removes the element's parent and returns the unwrapped content. This is effectively the inverse of the `wrap()` method.
> +```
> +Workaround:
> +
> +function create_new_user()
> + name, err = pcall(box.internal.func_call,
> + box.func.generate_user_name.name)
> + if not name then
> + local e = box.error.new({
> + code = APP_ERROR_CODE,
> + reason = "create_new_user"}):
> + e:wrap(err):raise()
> + end
> + box.schema.user.create(name)
> +end
> +```
> +
> +- We also need `error:has` method to test whether given error object contain given error code:
> +```
> +box.error.last():has(box.error.LuajitError)
> +```
> +
> +## Detailed design
> +
> +Diagnostic area object has a good encapsulated API.
> +
> +Lets extend error object with a pointer to it's reason error and introduce a few new methods (we need to rename diag_add_error to diag_set_error because it replaces an existent diag error in fact):
> +```
> +struct error {
> +...
> +/* A pointer to the reason error. */
> + struct error *reason;
> +};
> +
> +/**
> + * Add a new error to the diagnostics area: the previous error
> + * becomes a reason of a current.
> + * \param diag diagnostics area
> + * \param e error to add
> + */
> +static inline void
> +diag_add_error(struct diag *diag, struct error *e);
> +
> +/** Same as diag_set, but softly extend diagnostic information. */
> +#define diag_add(class, ...)
> +```
> +
> +### IPROTO Communication
> +Currently errors are sent as (IPROTO_TYPE_ERROR | errcode) messages with error message string payload.
> +Lets introduce a new IPROTO_TYPE_ERROR_TRACE error code with
> +[[errcode1, "errstring1"], [errcode2, "errstring2"]..] payload, where errors are serialized in historical order.
> --
> 2.22.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool
2019-08-07 23:27 ` Alexander Turenko
@ 2019-08-08 20:46 ` Vladislav Shpilevoy
2019-08-08 23:29 ` Alexander Turenko
0 siblings, 1 reply; 17+ messages in thread
From: Vladislav Shpilevoy @ 2019-08-08 20:46 UTC (permalink / raw)
To: tarantool-patches, Alexander Turenko, Kirill Shcherbatov; +Cc: kostja
> I had the objection against a list of errors in the diagnostic area: a
> user may want to save an error, perform some new commands, which can
> rewrite a last diagnostic (replace the diagnostic list) and then (s)he
> may want to look at the stack of reasons of the saved error. But now it
> seems you discarded the idea. (BTW, it would be good to briefly mention
> changes you made for a new revision of your document: the text looks
> similar and it is hard to find differences.)
Not sure if I understood the problem. What is wrong the list of errors?
And how does it correlate with an error accidental removal? If I got
the problem right, it exists even now, and does not depend on structure
of an error. Even errno has that problem, and the only way to solve it -
save the error to a local variable, perform needed preparations, and look
at the saved value. You can do it now. Just save 'box.error.last()', do
your stuff, and deal with the saved value.
>
> I think that struct error should have a pointer to its cause, which is
> another error.
But it does exactly this now. It is called 'reason', which IMO is
easier to understand. My first association with 'cause' is an informal
writing of 'because', and it confuses a bit.
> I like Java terminology here: cause is exactly what we want to save for
> an error. Wrap / unwrap (Go terminology) look too abstract and maybe
> even unclear for me. Maybe this terms appears from merry / xerror
> packages, which were inspired to wrap built-in errors with additional
> information (say, stack traces). It is up to maintainers, but my vote is
> for 'cause' field, 'get_cause()' and 'set_cause()' functions (maybe with
> some prefix, because it is C).
Strangely, but I understood wrap/unwrap easily. So I vote to keep them.
But if there are more people who doesn't understand, then here is my
humble proposal:
C API:
box_error_reason(const struct error *) // To get a reason error.
box_error_set_reason(struct error *, struct error *) // To set a reason.
Lua API:
error:reason() -- Get a reason error.
error:set_reason(reason) -- Set a reason error.
> 'Is' method (Go terminology, here it is named 'has') also don't look
> self-descriptive enough for me. I would name it 'find_cause()'.
I vote for just 'find'. I don't think 'cause' or 'reason' should be a
part of that function's name, because a real reason of the final result
is only one - the first error. This function on the contrary is able
to find any error in stack.
>
> I doubt a bit that iproto change you proposed would be
> backward-compatible: imagine that you connect from an old tarantool to a
> new tarantool using net.box or handle errors using another existing
> connector. I would send those messages as before, but add a field with
> IPROTO_ERROR_LIST key in addition to existing IPROTO_ERROR (0x31) to a
> body map. The value can be a list of maps like {code = <mp_uint>,
> message = <mp_str>} or a list of list [<code>, <message>]. The
> IPROTO_ERROR_LIST should start from a last (top) error I think.
I agree. I forgot about backward compatibility. Indeed, we can send both
new single error, and a list of errors. The list can even contain the
last error second time, because anyway error packets are rare and not
normal. We may not optimize them too much in terms of space.
>
> Maybe we should use 'warnings' term here if this feature is intended to
> be used for SQL warnings. If we want to use the proposed mechanics for
> warnings, then my proposal re using 'cause' term looks doubtful. Don't
> sure whether we should introduce some kind of warnings list for the
> diagnostic area or reuse 'cause' / 'parent' / ... field of struct error.
>
> I didn't look at your patch: I want to agree on terms and overall
> approach first. At least one of maintainers should approve it.
Most of you objections are about names, so I guess you already can take
a look at the functionality and tests.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool
2019-08-08 20:46 ` Vladislav Shpilevoy
@ 2019-08-08 23:29 ` Alexander Turenko
2019-08-09 19:25 ` Vladislav Shpilevoy
2019-08-12 20:35 ` Konstantin Osipov
0 siblings, 2 replies; 17+ messages in thread
From: Alexander Turenko @ 2019-08-08 23:29 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Kirill Shcherbatov, kostja
On Thu, Aug 08, 2019 at 10:46:29PM +0200, Vladislav Shpilevoy wrote:
> > I had the objection against a list of errors in the diagnostic area: a
> > user may want to save an error, perform some new commands, which can
> > rewrite a last diagnostic (replace the diagnostic list) and then (s)he
> > may want to look at the stack of reasons of the saved error. But now it
> > seems you discarded the idea. (BTW, it would be good to briefly mention
> > changes you made for a new revision of your document: the text looks
> > similar and it is hard to find differences.)
>
> Not sure if I understood the problem. What is wrong the list of errors?
> And how does it correlate with an error accidental removal? If I got
> the problem right, it exists even now, and does not depend on structure
> of an error. Even errno has that problem, and the only way to solve it -
> save the error to a local variable, perform needed preparations, and look
> at the saved value. You can do it now. Just save 'box.error.last()', do
> your stuff, and deal with the saved value.
I meant a difference between the approach when struct error does not
have a pointer to its reason (but a diagnostics area is expanded to
store a list) and the approach when struct error stores a pointer to a
reason. The former requires to copy the entire list to push it to Lua
(or save to in C to handle later), the latter requires just save a
pointer (and implement proper reference counting). Anyway, it seems we
stick now with the latter way and all agreed that this is better. Hope
we now understood each other.
>
> >
> > I think that struct error should have a pointer to its cause, which is
> > another error.
>
> But it does exactly this now. It is called 'reason', which IMO is
> easier to understand. My first association with 'cause' is an informal
> writing of 'because', and it confuses a bit.
Maybe it is a kind of my baby duck syndrome. I understood wrap/unwrap
now, but was a bit confused when meet those terms at the first time. I
have no strong objections against this terms. Want to collect more
thoughts from the developers and maintainers.
>
> > I like Java terminology here: cause is exactly what we want to save for
> > an error. Wrap / unwrap (Go terminology) look too abstract and maybe
> > even unclear for me. Maybe this terms appears from merry / xerror
> > packages, which were inspired to wrap built-in errors with additional
> > information (say, stack traces). It is up to maintainers, but my vote is
> > for 'cause' field, 'get_cause()' and 'set_cause()' functions (maybe with
> > some prefix, because it is C).
>
> Strangely, but I understood wrap/unwrap easily. So I vote to keep them.
> But if there are more people who doesn't understand, then here is my
> humble proposal:
>
> C API:
> box_error_reason(const struct error *) // To get a reason error.
> box_error_set_reason(struct error *, struct error *) // To set a reason.
>
> Lua API:
> error:reason() -- Get a reason error.
> error:set_reason(reason) -- Set a reason error.
Just to compare, now we have:
| C API:
| struct error *
| box_error_unwrap(box_error_t *error); // Get a reason and detach from a
| // passed error.
| struct error *
| box_error_wrap(box_error_t *error, box_error_t *reason); // Set a reason.
| Lua API:
| error:unwrap() -> reason -- Get a reason and detach it from a passed
| -- error.
| error:wrap(reason) -> error -- Set a reason.
I doubt whether a function to get a reason should modify the original
error. The terms need to be discussed with others.
>
> > 'Is' method (Go terminology, here it is named 'has') also don't look
> > self-descriptive enough for me. I would name it 'find_cause()'.
>
> I vote for just 'find'. I don't think 'cause' or 'reason' should be a
> part of that function's name, because a real reason of the final result
> is only one - the first error. This function on the contrary is able
> to find any error in stack.
It seems here we agreed on terms 'find'. Neat.
>
> >
> > I doubt a bit that iproto change you proposed would be
> > backward-compatible: imagine that you connect from an old tarantool to a
> > new tarantool using net.box or handle errors using another existing
> > connector. I would send those messages as before, but add a field with
> > IPROTO_ERROR_LIST key in addition to existing IPROTO_ERROR (0x31) to a
> > body map. The value can be a list of maps like {code = <mp_uint>,
> > message = <mp_str>} or a list of list [<code>, <message>]. The
> > IPROTO_ERROR_LIST should start from a last (top) error I think.
>
> I agree. I forgot about backward compatibility. Indeed, we can send both
> new single error, and a list of errors. The list can even contain the
> last error second time, because anyway error packets are rare and not
> normal. We may not optimize them too much in terms of space.
Agreed.
>
> >
> > Maybe we should use 'warnings' term here if this feature is intended to
> > be used for SQL warnings. If we want to use the proposed mechanics for
> > warnings, then my proposal re using 'cause' term looks doubtful. Don't
> > sure whether we should introduce some kind of warnings list for the
> > diagnostic area or reuse 'cause' / 'parent' / ... field of struct error.
This is most confusing part for me. Say, we want to set a warning re
precision loss during execution a SQL query. The response will be
successful. There will not be an error to wrap this warning. How to
store the warning (it looks as a query-local object) and how to show it
in the response (in the binary protocol)?
Another case: we emit a warning re precission loss and an error occurs
afterwards during the query execution (say, a constraint violation). The
warning is not a reason / cause / parent for the error and it is not
logical to using our current terms for this case.
We want to support SQL stacked diagnostics and it seems that the current
proposal does not move us forward to them. I had read mysql docs on
that, but I hope the standard described quite same thing:
https://dev.mysql.com/doc/refman/5.6/en/diagnostics-area.html
I think we need at least keep SQL stacked diagnostics in a mind and
explicitly decide whether we'll going (a bit?) forward to support them
within this issue / proposal / discussion.
> >
> > I didn't look at your patch: I want to agree on terms and overall
> > approach first. At least one of maintainers should approve it.
>
> Most of you objections are about names, so I guess you already can take
> a look at the functionality and tests.
Okay. I gave a glance on the patches and don't find anything that would
confuse me outside of questions we discussing here: terms, binary
protocol support (+net.box), SQL warnings.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API
2019-08-05 21:18 ` [tarantool-patches] " Vladislav Shpilevoy
2019-08-06 7:56 ` Kirill Shcherbatov
@ 2019-08-08 23:33 ` Alexander Turenko
2019-08-09 19:27 ` Vladislav Shpilevoy
1 sibling, 1 reply; 17+ messages in thread
From: Alexander Turenko @ 2019-08-08 23:33 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Kirill Shcherbatov, kostja
> > +box_error_unwrap(box_error_t *error)
> > +{
> > + struct error *reason = error->reason;
> > + assert(reason != NULL);
> > + diag_set_error(diag_get(), reason);
> > + error_unref(reason);
> > + error->reason = NULL;
> > + return reason;
>
> 7. Unwrap does not allow to unwrap a leaf error.
> But there is no API to determine if the error is
> leaf. So a user can't determine when to stop calling
> unwrap.
>
> I am talking about C public API which you have changed
> here. A user can't check error->reason != NULL before
> calling box_error_unwrap.
>
> Moreover, it is inconsistent with Lua version. Lets
> better return the argument when error->reason == NULL
> in box_error_unwrap. Then a user of the C API would
> just unwrap the stack until box_error_unwrap(e) == e.
> Also it simplifies Lua version implementation.
Why not just return NULL when there is no a reason? It seems to be more
logical for me.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool
2019-08-08 23:29 ` Alexander Turenko
@ 2019-08-09 19:25 ` Vladislav Shpilevoy
2019-08-12 20:35 ` Konstantin Osipov
1 sibling, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy @ 2019-08-09 19:25 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches, Kirill Shcherbatov, kostja
>>> I like Java terminology here: cause is exactly what we want to save for
>>> an error. Wrap / unwrap (Go terminology) look too abstract and maybe
>>> even unclear for me. Maybe this terms appears from merry / xerror
>>> packages, which were inspired to wrap built-in errors with additional
>>> information (say, stack traces). It is up to maintainers, but my vote is
>>> for 'cause' field, 'get_cause()' and 'set_cause()' functions (maybe with
>>> some prefix, because it is C).
>>
>> Strangely, but I understood wrap/unwrap easily. So I vote to keep them.
>> But if there are more people who doesn't understand, then here is my
>> humble proposal:
>>
>> C API:
>> box_error_reason(const struct error *) // To get a reason error.
>> box_error_set_reason(struct error *, struct error *) // To set a reason.
>>
>> Lua API:
>> error:reason() -- Get a reason error.
>> error:set_reason(reason) -- Set a reason error.
>
> Just to compare, now we have:
>
> | C API:
> | struct error *
> | box_error_unwrap(box_error_t *error); // Get a reason and detach from a
> | // passed error.
> | struct error *
> | box_error_wrap(box_error_t *error, box_error_t *reason); // Set a reason.
>
> | Lua API:
> | error:unwrap() -> reason -- Get a reason and detach it from a passed
> | -- error.
> | error:wrap(reason) -> error -- Set a reason.
>
> I doubt whether a function to get a reason should modify the original
> error. The terms need to be discussed with others.
Agree, an attempt to get reason should not modify the original error. And
should not touch the global diagnostics area. Currently error:unwrap()
changes box.error.last() too.
>>> Maybe we should use 'warnings' term here if this feature is intended to
>>> be used for SQL warnings. If we want to use the proposed mechanics for
>>> warnings, then my proposal re using 'cause' term looks doubtful. Don't
>>> sure whether we should introduce some kind of warnings list for the
>>> diagnostic area or reuse 'cause' / 'parent' / ... field of struct error.
>
> This is most confusing part for me. Say, we want to set a warning re
> precision loss during execution a SQL query. The response will be
> successful. There will not be an error to wrap this warning. How to
> store the warning (it looks as a query-local object) and how to show it
> in the response (in the binary protocol)?
>
> Another case: we emit a warning re precission loss and an error occurs
> afterwards during the query execution (say, a constraint violation). The
> warning is not a reason / cause / parent for the error and it is not
> logical to using our current terms for this case.
>
> We want to support SQL stacked diagnostics and it seems that the current
> proposal does not move us forward to them. I had read mysql docs on
> that, but I hope the standard described quite same thing:
> https://dev.mysql.com/doc/refman/5.6/en/diagnostics-area.html
>
> I think we need at least keep SQL stacked diagnostics in a mind and
> explicitly decide whether we'll going (a bit?) forward to support them
> within this issue / proposal / discussion.
I didn't think it was related to warnings. I didn't even think, that we
are going to support warnings. After all, if something is not an error,
people will just ignore it. I bet, one of the first SQL related issues
after we release the warnings would be 'How to turn warnings off?'.
IMO, we should just treat everything as errors.
If warnings are already discussed and planned, then unfortunately I don't
have a strong opinion on the subject. Nonetheless, below are my first
thoughts about the warnings.
I think, that warnings should be stored in the same stack as errors,
and we need a flag saying if the stack contains at least one error. In
that case the whole stack is treated as an error.
Why in the same stack? Because it will look like a request execution
history. All errors and warnings would be sorted by occurrence time,
and it will be easy to locate in scope of what an error or a warning
has happened.
Talking of how would it look in code - perhaps we will add something
like 'struct warning', and both 'struct error' and 'struct warning'
would inherit from a base structure. Fiber would contain list of that
structures. This implementation lays good on the current version of
stacked diagnostics. Just raw thoughts.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API
2019-08-08 23:33 ` Alexander Turenko
@ 2019-08-09 19:27 ` Vladislav Shpilevoy
0 siblings, 0 replies; 17+ messages in thread
From: Vladislav Shpilevoy @ 2019-08-09 19:27 UTC (permalink / raw)
To: tarantool-patches, Alexander Turenko; +Cc: Kirill Shcherbatov, kostja
On 09/08/2019 01:33, Alexander Turenko wrote:
>>> +box_error_unwrap(box_error_t *error)
>>> +{
>>> + struct error *reason = error->reason;
>>> + assert(reason != NULL);
>>> + diag_set_error(diag_get(), reason);
>>> + error_unref(reason);
>>> + error->reason = NULL;
>>> + return reason;
>>
>> 7. Unwrap does not allow to unwrap a leaf error.
>> But there is no API to determine if the error is
>> leaf. So a user can't determine when to stop calling
>> unwrap.
>>
>> I am talking about C public API which you have changed
>> here. A user can't check error->reason != NULL before
>> calling box_error_unwrap.
>>
>> Moreover, it is inconsistent with Lua version. Lets
>> better return the argument when error->reason == NULL
>> in box_error_unwrap. Then a user of the C API would
>> just unwrap the stack until box_error_unwrap(e) == e.
>> Also it simplifies Lua version implementation.
>
> Why not just return NULL when there is no a reason? It seems to be more
> logical for me.
>
Perhaps. I don't mind, NULL is good too. But assertion is
not good.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool
2019-08-08 23:29 ` Alexander Turenko
2019-08-09 19:25 ` Vladislav Shpilevoy
@ 2019-08-12 20:35 ` Konstantin Osipov
1 sibling, 0 replies; 17+ messages in thread
From: Konstantin Osipov @ 2019-08-12 20:35 UTC (permalink / raw)
To: Alexander Turenko
Cc: Vladislav Shpilevoy, tarantool-patches, Kirill Shcherbatov
* Alexander Turenko <alexander.turenko@tarantool.org> [19/08/09 10:11]:
> >
> > >
> > > Maybe we should use 'warnings' term here if this feature is intended to
> > > be used for SQL warnings. If we want to use the proposed mechanics for
> > > warnings, then my proposal re using 'cause' term looks doubtful. Don't
> > > sure whether we should introduce some kind of warnings list for the
> > > diagnostic area or reuse 'cause' / 'parent' / ... field of struct error.
>
> This is most confusing part for me. Say, we want to set a warning re
> precision loss during execution a SQL query. The response will be
> successful. There will not be an error to wrap this warning. How to
> store the warning (it looks as a query-local object) and how to show it
> in the response (in the binary protocol)?
>
> Another case: we emit a warning re precission loss and an error occurs
> afterwards during the query execution (say, a constraint violation). The
> warning is not a reason / cause / parent for the error and it is not
> logical to using our current terms for this case.
Warnings are an entirely different beast to stacked errors.
When we get to supporting warnings, this will be a separate object
in the diagnostics.
> We want to support SQL stacked diagnostics and it seems that the current
> proposal does not move us forward to them. I had read mysql docs on
> that, but I hope the standard described quite same thing:
> https://dev.mysql.com/doc/refman/5.6/en/diagnostics-area.html
>
> I think we need at least keep SQL stacked diagnostics in a mind and
> explicitly decide whether we'll going (a bit?) forward to support them
> within this issue / proposal / discussion.
Yes.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-08-12 20:35 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-01 11:13 [tarantool-patches] [PATCH v1 0/3] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 1/3] box: rfc for stacked diagnostic area in Tarantool Kirill Shcherbatov
2019-08-05 21:16 ` [tarantool-patches] " Vladislav Shpilevoy
[not found] ` <06bd2140-3d2b-4bc3-7bc4-5f3d293bf891@tarantool.org>
2019-08-06 20:50 ` Vladislav Shpilevoy
2019-08-07 23:27 ` Alexander Turenko
2019-08-08 20:46 ` Vladislav Shpilevoy
2019-08-08 23:29 ` Alexander Turenko
2019-08-09 19:25 ` Vladislav Shpilevoy
2019-08-12 20:35 ` Konstantin Osipov
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 2/3] box: stacked diagnostics area in fiber Kirill Shcherbatov
2019-08-05 21:16 ` [tarantool-patches] " Vladislav Shpilevoy
2019-08-01 11:13 ` [tarantool-patches] [PATCH v1 3/3] box: extend ffi error object API Kirill Shcherbatov
2019-08-05 21:18 ` [tarantool-patches] " Vladislav Shpilevoy
2019-08-06 7:56 ` Kirill Shcherbatov
2019-08-06 20:50 ` Vladislav Shpilevoy
2019-08-08 23:33 ` Alexander Turenko
2019-08-09 19:27 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox