From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 525874696C6 for ; Wed, 19 Feb 2020 17:17:02 +0300 (MSK) From: Nikita Pettik Date: Wed, 19 Feb 2020 17:16:53 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: Subject: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org In terms of implementation, now struct error objects can be organized into double-linked lists. To achieve this pointers to the next and previous elements have been added to struct error. It is worth mentioning that already existing rlist and stailq list implementations are not suitable: rlist is cycled list, as a result it is impossible to start iteration over the list from random list entry and finish it at the logical end of the list; stailq is single-linked list leaving no possibility to remove elements from the middle of the list. As a part of C interface, box_error_add() has been introduced. In contrast to box_error_set() it does not replace last raised error, but instead it adds error to the list of diagnostic errors having already been set. If error is to be deleted (its reference counter hits 0 value) it is unlinked from the list it belongs to and destroyed. To organize errors into lists in Lua, table representing error object in Lua now has .prev field (corresponding to 'previous' error) and method :set_prev(e). The latter accepts error object (i.e. created via box.error.new() or box.error.last()) and nil value. Both field .prev and :set_prev() method are implemented as ffi functions. Also note that cycles are now allowed while organizing errors into lists: e1 -> e2 -> e3; e3:set_prev(e1) -- would lead to error. Part of #1148 --- extra/exports | 2 + src/box/key_list.c | 16 +-- src/box/lua/call.c | 6 +- src/lib/core/diag.c | 51 +++++++++ src/lib/core/diag.h | 95 +++++++++++++++- src/lua/error.lua | 40 +++++++ test/box/misc.result | 233 ++++++++++++++++++++++++++++++++++++++++ test/box/misc.test.lua | 89 +++++++++++++++ test/engine/func_index.result | 50 +++++++-- test/engine/func_index.test.lua | 7 ++ 10 files changed, 568 insertions(+), 21 deletions(-) diff --git a/extra/exports b/extra/exports index 7b84a1452..94cbdd210 100644 --- a/extra/exports +++ b/extra/exports @@ -246,6 +246,8 @@ clock_monotonic64 clock_process64 clock_thread64 string_strip_helper +error_prev +error_set_prev # Lua / LuaJIT diff --git a/src/box/key_list.c b/src/box/key_list.c index 3d736b55f..a766ce0ec 100644 --- a/src/box/key_list.c +++ b/src/box/key_list.c @@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple, if (rc != 0) { /* Can't evaluate function. */ struct space *space = space_by_id(index_def->space_id); - diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name, - space ? space_name(space) : "", - diag_last_error(diag_get())->errmsg); + diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name, + space != NULL ? space_name(space) : "", + "can't evaluate function"); return -1; } uint32_t key_data_sz; @@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple, if (key_data == NULL) { struct space *space = space_by_id(index_def->space_id); /* Can't get a result returned by function . */ - diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name, - space ? space_name(space) : "", - diag_last_error(diag_get())->errmsg); + diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name, + space != NULL ? space_name(space) : "", + "can't get a value returned by function"); return -1; } @@ -170,9 +170,9 @@ key_list_iterator_next(struct key_list_iterator *it, const char **value) * The key doesn't follow functional index key * definition. */ - diag_set(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name, + diag_add(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name, space ? space_name(space) : "", - diag_last_error(diag_get())->errmsg); + "key does not follow functional index definition"); return -1; } diff --git a/src/box/lua/call.c b/src/box/lua/call.c index f1bbde7f0..5d3579eff 100644 --- a/src/box/lua/call.c +++ b/src/box/lua/call.c @@ -687,9 +687,9 @@ func_persistent_lua_load(struct func_lua *func) if (func->base.def->is_sandboxed) { if (prepare_lua_sandbox(tarantool_L, default_sandbox_exports, nelem(default_sandbox_exports)) != 0) { - diag_set(ClientError, ER_LOAD_FUNCTION, - func->base.def->name, - diag_last_error(diag_get())->errmsg); + diag_add(ClientError, ER_LOAD_FUNCTION, + func->base.def->name, + "can't prepare a Lua sandbox"); goto end; } } else { diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c index c350abb4a..1776dc8cf 100644 --- a/src/lib/core/diag.c +++ b/src/lib/core/diag.c @@ -34,6 +34,55 @@ /* Must be set by the library user */ struct error_factory *error_factory = NULL; +struct error * +error_prev(struct error *e) +{ + assert(e != NULL); + return e->next; +} + +int +error_set_prev(struct error *e, struct error *prev) +{ + /* + * Make sure that adding error won't result in cycles. + * Don't bother with sophisticated cycle-detection + * algorithms, simple iteration is OK since as a rule + * list contains a dozen errors at maximum. + */ + struct error *tmp = prev; + while (tmp != NULL) { + if (tmp == e) + return -1; + tmp = tmp->next; + } + /* + * At once error can be reason for only one error. + * So unlink previous 'prev' node. + * + * +--------+ NEXT +--------+ + * | e | ---> |old prev| + * +--------+ +--------+ + * ^ | + * | PREV | + * +-------X-------+ + * + */ + if (e->next != NULL) + e->next->prev = NULL; + /* Set new 'prev' node. */ + e->next = prev; + /* + * Unlink new 'prev' node from its old stack. + * nil can be also passed as an argument. + */ + if (prev != NULL) { + error_unlink_tail(prev); + prev->prev = e; + } + return 0; +} + void error_create(struct error *e, error_f destroy, error_f raise, error_f log, @@ -53,6 +102,8 @@ error_create(struct error *e, e->line = 0; } e->errmsg[0] = '\0'; + e->prev = NULL; + e->next = NULL; } struct diag * diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h index 7e1e1a174..5271733cb 100644 --- a/src/lib/core/diag.h +++ b/src/lib/core/diag.h @@ -37,6 +37,7 @@ #include #include #include "say.h" +#include "small/rlist.h" #if defined(__cplusplus) extern "C" { @@ -84,6 +85,17 @@ struct error { char file[DIAG_FILENAME_MAX]; /* Error description. */ char errmsg[DIAG_ERRMSG_MAX]; + /** + * Link to the next and previous errors. + * RLIST implementation is not really suitable here + * since it is organized as circular list. In such + * a case it is impossible to start an iteration + * from any node and finish at the logical end of the + * list. Double-linked list is required to allow deletion + * from the middle of the list. + */ + struct error *next; + struct error *prev; }; static inline void @@ -92,15 +104,61 @@ error_ref(struct error *e) e->refs++; } +/** + * Unlink error from any error which point to it. For instance: + * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(33) ...) + * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL + */ +static inline void +error_unlink_tail(struct error *e) +{ + if (e->prev != NULL) + e->prev->next = NULL; + e->prev = NULL; +} + + static inline void error_unref(struct error *e) { assert(e->refs > 0); --e->refs; - if (e->refs == 0) + if (e->refs == 0) { + /* Unlink error from the list completely. */ + if (e->prev != NULL) + e->prev->next = e->next; + if (e->next != NULL) + e->next->prev = e->prev; + e->next = NULL; + e->prev = NULL; e->destroy(e); + } } +/** + * Return previous (for given error) error. Result can be NULL + * which means that there's no previous error. Simple getter + * to be used as ffi method in lua/error.lua. + */ +struct error * +error_prev(struct error *e); + +/** + * Set previous error: remove @a prev from its current stack and + * link to the one @a e belongs to. Note that all previous errors + * starting from @a prev->next are transferred with it as well + * (i.e. reasons for given error are not erased). For instance: + * e1 -> e2 -> NULL; e3 -> e4 -> NULL; + * e2:set_prev(e3): e1 -> e2 -> e3 -> e4 -> NULL + * + * @a prev can be NULL. To be used as ffi method in lua/error.lua. + * + * @retval -1 in case adding @a prev results in list cycles; + * 0 otherwise. + */ +int +error_set_prev(struct error *e, struct error *prev); + NORETURN static inline void error_raise(struct error *e) { @@ -163,7 +221,12 @@ diag_clear(struct diag *diag) { if (diag->last == NULL) return; - error_unref(diag->last); + struct error *last = diag->last; + while (last != NULL) { + struct error *tmp = last->next; + error_unref(last); + last = tmp; + } diag->last = NULL; } @@ -178,6 +241,25 @@ diag_set_error(struct diag *diag, struct error *e) assert(e != NULL); error_ref(e); diag_clear(diag); + error_unlink_tail(e); + diag->last = e; +} + +/** + * Add a new error to the diagnostics area. It is added to the + * tail, so that list forms stack. + * @param diag Diagnostics area. + * @param e Error to be added. + */ +static inline void +diag_add_error(struct diag *diag, struct error *e) +{ + assert(e != NULL); + error_ref(e); + error_unlink_tail(e); + e->next = diag->last; + if (diag->last != NULL) + diag->last->prev = e; diag->last = e; } @@ -280,6 +362,15 @@ BuildSocketError(const char *file, unsigned line, const char *socketname, errno = save_errno; \ } while (0) +#define diag_add(class, ...) do { \ + int save_errno = errno; \ + say_debug("%s at %s:%i", #class, __FILE__, __LINE__); \ + struct error *e; \ + e = Build##class(__FILE__, __LINE__, ##__VA_ARGS__); \ + diag_add_error(diag_get(), e); \ + errno = save_errno; \ +} while (0) + #if defined(__cplusplus) } /* extern "C" */ #endif /* defined(__cplusplus) */ diff --git a/src/lua/error.lua b/src/lua/error.lua index 7f249864a..222b5e273 100644 --- a/src/lua/error.lua +++ b/src/lua/error.lua @@ -11,6 +11,11 @@ enum { typedef void (*error_f)(struct error *e); +struct rlist { + struct rlist *prev; + struct rlist *next; +}; + struct error { error_f _destroy; error_f _raise; @@ -24,12 +29,20 @@ struct error { char _file[DIAG_FILENAME_MAX]; /* Error description. */ char _errmsg[DIAG_ERRMSG_MAX]; + struct error *_next; + struct error *_prev; }; char * exception_get_string(struct error *e, const struct method_info *method); int exception_get_int(struct error *e, const struct method_info *method); + +struct error * +error_prev(struct error *e); + +int +error_set_prev(struct error *e, struct error *prev); ]] local REFLECTION_CACHE = {} @@ -95,11 +108,37 @@ local function error_errno(err) return e end +local function error_prev(err) + local e = ffi.C.error_prev(err); + if e ~= nil then + return e + else + return nil + end +end + +local function error_set_prev(err, prev) + -- First argument must be error. + if not ffi.istype('struct error', err) then + error("Usage: error1:set_prev(error2)") + end + -- Second argument must be error or nil. + if not ffi.istype('struct error', prev) and prev ~= nil then + error("Usage: error1:set_prev(error2)") + end + local ok = tonumber(ffi.C.error_set_prev(err, prev)); + if ok ~= 0 then + error("Cycles are not allowed") + end + +end + local error_fields = { ["type"] = error_type; ["message"] = error_message; ["trace"] = error_trace; ["errno"] = error_errno; + ["prev"] = error_prev; } local function error_unpack(err) @@ -143,6 +182,7 @@ local error_methods = { ["raise"] = error_raise; ["match"] = error_match; -- Tarantool 1.6 backward compatibility ["__serialize"] = error_serialize; + ["set_prev"] = error_set_prev; } local function error_index(err, key) diff --git a/test/box/misc.result b/test/box/misc.result index b0a81a055..4e4825a76 100644 --- a/test/box/misc.result +++ b/test/box/misc.result @@ -352,6 +352,239 @@ box.error.clear() box.error() --- ... +-- gh-1148: erros can be arranged into list (so called +-- stacked diagnostics). +-- +e1 = box.error.new({code = 111, reason = "cause"}) +--- +... +assert(e1.prev == nil) +--- +- true +... +e1:set_prev(e1) +--- +- error: 'builtin/error.lua: Cycles are not allowed' +... +assert(e1.prev == nil) +--- +- true +... +e2 = box.error.new({code = 111, reason = "cause of cause"}) +--- +... +e1:set_prev(e2) +--- +... +assert(e1.prev == e2) +--- +- true +... +e2:set_prev(e1) +--- +- error: 'builtin/error.lua: Cycles are not allowed' +... +assert(e2.prev == nil) +--- +- true +... +-- At this point stack is following: e1 -> e2 +-- Let's test following cases: +-- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2)) +-- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3)) +-- 3. e3 -> e1 -> e2 (e3:set_prev(e1)) +-- 4. e1 -> e2 -> e3 (e2:set_prev(e3)) +-- +e3 = box.error.new({code = 111, reason = "another cause"}) +--- +... +e3:set_prev(e2) +--- +... +assert(e3.prev == e2) +--- +- true +... +assert(e2.prev == nil) +--- +- true +... +assert(e1.prev == nil) +--- +- true +... +-- Reset stack to e1 -> e2 and test case 2. +-- +e1:set_prev(e2) +--- +... +assert(e2.prev == nil) +--- +- true +... +assert(e3.prev == nil) +--- +- true +... +e1:set_prev(e3) +--- +... +assert(e2.prev == nil) +--- +- true +... +assert(e1.prev == e3) +--- +- true +... +assert(e3.prev == nil) +--- +- true +... +-- Reset stack to e1 -> e2 and test case 3. +-- +e1:set_prev(e2) +--- +... +assert(e1.prev == e2) +--- +- true +... +assert(e2.prev == nil) +--- +- true +... +assert(e3.prev == nil) +--- +- true +... +e3:set_prev(e1) +--- +... +assert(e1.prev == e2) +--- +- true +... +assert(e2.prev == nil) +--- +- true +... +assert(e3.prev == e1) +--- +- true +... +-- Unlink errors and test case 4. +-- +e1:set_prev(nil) +--- +... +e2:set_prev(nil) +--- +... +e3:set_prev(nil) +--- +... +e1:set_prev(e2) +--- +... +e2:set_prev(e3) +--- +... +assert(e1.prev == e2) +--- +- true +... +assert(e2.prev == e3) +--- +- true +... +assert(e3.prev == nil) +--- +- true +... +-- Test circle detecting. At the moment stack is +-- following: e1 -> e2 -> e3 +-- +e3:set_prev(e1) +--- +- error: 'builtin/error.lua: Cycles are not allowed' +... +assert(e3.prev == nil) +--- +- true +... +e3:set_prev(e2) +--- +- error: 'builtin/error.lua: Cycles are not allowed' +... +assert(e3.prev == nil) +--- +- true +... +-- Test splitting list into two ones. +-- After that we will get two lists: e1->e2->e5 and e3->e4 +-- +e4 = box.error.new({code = 111, reason = "yet another cause"}) +--- +... +e5 = box.error.new({code = 111, reason = "and another one"}) +--- +... +e3:set_prev(e4) +--- +... +e2:set_prev(e5) +--- +... +assert(e1.prev == e2) +--- +- true +... +assert(e2.prev == e5) +--- +- true +... +assert(e3.prev == e4) +--- +- true +... +assert(e5.prev == nil) +--- +- true +... +assert(e4.prev == nil) +--- +- true +... +-- Another splitting option: e1->e2 and e5->e3->e4 +-- But firstly restore to one single list e1->e2->e3->e4 +-- +e2:set_prev(e3) +--- +... +e5:set_prev(e3) +--- +... +assert(e1.prev == e2) +--- +- true +... +assert(e2.prev == nil) +--- +- true +... +assert(e5.prev == e3) +--- +- true +... +assert(e3.prev == e4) +--- +- true +... +assert(e4.prev == nil) +--- +- true +... ---------------- -- # box.stat ---------------- diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua index 0351317dd..bf429d18f 100644 --- a/test/box/misc.test.lua +++ b/test/box/misc.test.lua @@ -121,6 +121,95 @@ box.error.new(err) box.error.clear() box.error() +-- gh-1148: erros can be arranged into list (so called +-- stacked diagnostics). +-- +e1 = box.error.new({code = 111, reason = "cause"}) +assert(e1.prev == nil) +e1:set_prev(e1) +assert(e1.prev == nil) +e2 = box.error.new({code = 111, reason = "cause of cause"}) +e1:set_prev(e2) +assert(e1.prev == e2) +e2:set_prev(e1) +assert(e2.prev == nil) +-- At this point stack is following: e1 -> e2 +-- Let's test following cases: +-- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2)) +-- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3)) +-- 3. e3 -> e1 -> e2 (e3:set_prev(e1)) +-- 4. e1 -> e2 -> e3 (e2:set_prev(e3)) +-- +e3 = box.error.new({code = 111, reason = "another cause"}) +e3:set_prev(e2) +assert(e3.prev == e2) +assert(e2.prev == nil) +assert(e1.prev == nil) + +-- Reset stack to e1 -> e2 and test case 2. +-- +e1:set_prev(e2) +assert(e2.prev == nil) +assert(e3.prev == nil) +e1:set_prev(e3) +assert(e2.prev == nil) +assert(e1.prev == e3) +assert(e3.prev == nil) + +-- Reset stack to e1 -> e2 and test case 3. +-- +e1:set_prev(e2) +assert(e1.prev == e2) +assert(e2.prev == nil) +assert(e3.prev == nil) +e3:set_prev(e1) +assert(e1.prev == e2) +assert(e2.prev == nil) +assert(e3.prev == e1) + +-- Unlink errors and test case 4. +-- +e1:set_prev(nil) +e2:set_prev(nil) +e3:set_prev(nil) +e1:set_prev(e2) +e2:set_prev(e3) +assert(e1.prev == e2) +assert(e2.prev == e3) +assert(e3.prev == nil) + +-- Test circle detecting. At the moment stack is +-- following: e1 -> e2 -> e3 +-- +e3:set_prev(e1) +assert(e3.prev == nil) +e3:set_prev(e2) +assert(e3.prev == nil) + +-- Test splitting list into two ones. +-- After that we will get two lists: e1->e2->e5 and e3->e4 +-- +e4 = box.error.new({code = 111, reason = "yet another cause"}) +e5 = box.error.new({code = 111, reason = "and another one"}) +e3:set_prev(e4) +e2:set_prev(e5) +assert(e1.prev == e2) +assert(e2.prev == e5) +assert(e3.prev == e4) +assert(e5.prev == nil) +assert(e4.prev == nil) + +-- Another splitting option: e1->e2 and e5->e3->e4 +-- But firstly restore to one single list e1->e2->e3->e4 +-- +e2:set_prev(e3) +e5:set_prev(e3) +assert(e1.prev == e2) +assert(e2.prev == nil) +assert(e5.prev == e3) +assert(e3.prev == e4) +assert(e4.prev == nil) + ---------------- -- # box.stat ---------------- diff --git a/test/engine/func_index.result b/test/engine/func_index.result index 84cb83022..8f92fcf11 100644 --- a/test/engine/func_index.result +++ b/test/engine/func_index.result @@ -5,6 +5,10 @@ test_run = require('test_run').new() engine = test_run:get_cfg('engine') | --- | ... +test_run:cmd("push filter \"file: .*\" to \"file: \"") + | --- + | - true + | ... -- -- gh-1260: Func index. @@ -158,8 +162,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn1.id, parts = {{1, 'un s:insert({1}) | --- | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space - | ''withdata'': Supplied key type of part 0 does not match index part type: expected - | unsigned' + | ''withdata'': key does not follow functional index definition' | ... idx:drop() | --- @@ -197,8 +200,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn3.id, parts = {{1, 'un s:insert({1}) | --- | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space - | ''withdata'': Supplied key type of part 0 does not match index part type: expected - | unsigned' + | ''withdata'': key does not follow functional index definition' | ... idx:drop() | --- @@ -217,8 +219,7 @@ idx = s:create_index('idx', {func = box.func.invalidreturn4.id, parts = {{1, 'un s:insert({1}) | --- | - error: 'Key format doesn''t match one defined in functional index ''idx'' of space - | ''withdata'': Supplied key type of part 0 does not match index part type: expected - | unsigned' + | ''withdata'': key does not follow functional index definition' | ... idx:drop() | --- @@ -264,8 +265,41 @@ idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'stri s:insert({1}) | --- | - error: 'Failed to build a key for functional index ''idx'' of space ''withdata'': - | [string "return function(tuple) local ..."]:1: attempt to call - | global ''require'' (a nil value)' + | can''t evaluate function' + | ... +e = box.error.last() + | --- + | ... +e:unpack() + | --- + | - code: 198 + | trace: + | - file: + | line: 68 + | type: ClientError + | message: 'Failed to build a key for functional index ''idx'' of space ''withdata'': + | can''t evaluate function' + | prev: '[string "return function(tuple) local ..."]:1: attempt to + | call global ''require'' (a nil value)' + | ... +e = e.prev + | --- + | ... +e:unpack() + | --- + | - type: LuajitError + | message: '[string "return function(tuple) local ..."]:1: attempt + | to call global ''require'' (a nil value)' + | trace: + | - file: + | line: 1028 + | ... +e = e.prev + | --- + | ... +e == nil + | --- + | - true | ... idx:drop() | --- diff --git a/test/engine/func_index.test.lua b/test/engine/func_index.test.lua index f31162c97..0e4043260 100644 --- a/test/engine/func_index.test.lua +++ b/test/engine/func_index.test.lua @@ -1,5 +1,6 @@ test_run = require('test_run').new() engine = test_run:get_cfg('engine') +test_run:cmd("push filter \"file: .*\" to \"file: \"") -- -- gh-1260: Func index. @@ -99,6 +100,12 @@ test_run:cmd("setopt delimiter ''"); box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true}) idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}}) s:insert({1}) +e = box.error.last() +e:unpack() +e = e.prev +e:unpack() +e = e.prev +e == nil idx:drop() -- Remove old persistent functions -- 2.15.1