From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 469BA4696C3 for ; Wed, 1 Apr 2020 19:26:19 +0300 (MSK) Date: Wed, 1 Apr 2020 16:26:18 +0000 From: Nikita Pettik Message-ID: <20200401162618.GD26447@tarantool.org> References: <3bb7c695b34356137897b953281543c46fe3bafa.1585097339.git.korablev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 31 Mar 01:24, Vladislav Shpilevoy wrote: > Thanks for the patchset! > > On future - it would be good to have diffs and answers on > my comments inlined. Otherwise I need to jump between the > new email, the old one, and the commit's diff to check what > and how was fixed. And I likely forget something. It's quite complicated to extract certain diff while working on whole patch-set and changes are big enough. Will try next time. (All non-optional requested fixes are done; I answered explicitly on skipped comments). > See 10 comments below. > > On 25/03/2020 02:43, Nikita Pettik wrote: > > This patch introduces support of stacked errors in IProto protocol and > > in net.box module. > > > > @TarantoolBot document > > Title: Stacked error diagnostic area > > > > Starting from now errors can be organized into lists. To achieve this > > Lua table representing error object is extended with .prev field and > > e:set_prev(err) method. .prev field returns previous error if any exist. > > e:set_prev(err) method expects err to be error object or nil and sets > > err as previous error of e. For instance: > > > > e1 = box.error.new({code = 111, reason = "cause"}) > > e2 = box.error.new({code = 111, reason = "cause of cause"}) > > 1. I would wrap these code samples into ```. Otherwise you may > screw the whole ticket formatting accidentally. You can check how > will it look by clicking 'New issue', pasting the request, and > clicking 'Preview'. Fixed. > > > > Closes #1148 > > 2. Propose to move this to before the docbot request. So as not > to include it into the doc ticket. Fixed. > > diff --git a/src/box/xrow.c b/src/box/xrow.c > > index 256dd4d91..ceb38b040 100644 > > --- a/src/box/xrow.c > > +++ b/src/box/xrow.c > > @@ -1070,19 +1083,66 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len, > > return 0; > > } > > > > +static int > > +iproto_decode_error_stack(const char **pos, const char *end) > > +{ > > + const char *reason = NULL; > > + static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\ > > + "expected to be larger than error message max length"); > > + /* > > + * Erase previously set diag errors. It is required since > > + * box_error_add() does not replace previous errors. > > + */ > > + box_error_clear(); > > + if (mp_check_array(*pos, end) != 0) > > 3. mp_check() functions return > 0 on error. > > When you call mp_check_array() without checking if it is > MP_ARRAY, it will crash. Also it will crash if *pos == end > in the checks below. But it does not matter anymore. > > I noticed that we don't need MessagePack validness checking > here. I see now that mp_check() is called by some upper function > always. You need only check that MP_ types are correct. That it > is MP_ARRAY, keys are MP_UINT and so on. Fixed: diff --git a/src/box/xrow.c b/src/box/xrow.c index e84c6f758..596c765b7 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -1084,7 +1084,7 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len, } static int -iproto_decode_error_stack(const char **pos, const char *end) +iproto_decode_error_stack(const char **pos) { const char *reason = NULL; static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\ @@ -1099,22 +1099,19 @@ iproto_decode_error_stack(const char **pos, const char *end) uint32_t stack_sz = mp_decode_array(pos); for (uint32_t i = 0; i < stack_sz; i++) { uint32_t code = 0; - if (mp_typeof(**pos) != MP_MAP || mp_check_map(*pos, end) != 0) + if (mp_typeof(**pos) != MP_MAP) return -1; uint32_t map_sz = mp_decode_map(pos); for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) { - if (mp_typeof(**pos) != MP_UINT || - mp_check_uint(*pos, end) != 0) + if (mp_typeof(**pos) != MP_UINT) return -1; uint8_t key = mp_decode_uint(pos); if (key == IPROTO_ERROR_CODE) { - if (mp_typeof(**pos) != MP_UINT || - mp_check_uint(*pos, end) != 0) + if (mp_typeof(**pos) != MP_UINT) return -1; code = mp_decode_uint(pos); } else if (key == IPROTO_ERROR_MESSAGE) { - if (mp_typeof(**pos) != MP_STR || - mp_check_strl(*pos, end) != 0) + if (mp_typeof(**pos) != MP_STR) return -1; uint32_t len; const char *str = mp_decode_str(pos, &len); @@ -1169,7 +1166,7 @@ xrow_decode_error(struct xrow_header *row) } else if (key == IPROTO_ERROR_STACK ) { if (mp_check_array(pos, end) != 0) goto error; - if (iproto_decode_error_stack(&pos, end) != 0) + if (iproto_decode_error_stack(&pos) != 0) continue; } else { mp_next(&pos); > I don't see any tests on that. Are there some? Maybe at least > unit tests, in xrow.cc? Although a proper test would try to > send an error stack through applier. Because xrow_decode_error() > is used only there. I'm a bit new to replication tests. I've been working on it. Could you please review the rest of changes so far? > > diff --git a/test/box/iproto.result b/test/box/iproto.result > > new file mode 100644 > > index 000000000..bb369be5a > > --- /dev/null > > +++ b/test/box/iproto.result > > @@ -0,0 +1,141 @@ > > +test_run = require('test_run').new() > > 4. test_run objects seems to be not necessary here. Reworked whole iproto.test.lua according to your suggestion to use separate function setting error stack. See diff below. > > +... > > +IPROTO_ERROR_MESSAGE = 0x02 > > +--- > > +... > > +IPROTO_OK = 0x00 > > +--- > > +... > > +IPROTO_SCHEMA_VERSION = 0x05 > > +--- > > +... > > +IPROTO_STATUS_KEY = 0x00 > > 5. The last 3 keys are unused. For sure. Removed. > > +... > > +assert(response.body[IPROTO_ERROR] ~= nil) > > +--- > > +- true > > +... > > +err = response.body[IPROTO_ERROR_STACK][1] > > +--- > > +... > > +assert(err[IPROTO_ERROR_MESSAGE] == body[IPROTO_ERROR]) > > +--- > > +- error: assertion failed! > > 6.Shouldn't it be true? You meant response.body, not just body? Yep, fixed. Thx. > > +--- > > +- true > > +... > > +assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string') > > 7. This test covers the previous assertion. type(nil) > is nil, so it won't be equal to 'string'. Didn't get it. It was not nil, but string containing message. Nevermind, test is reworked. > > diff --git a/test/box/net.box.result b/test/box/net.box.result > > index e3dabf7d9..eeefc0e3f 100644 > > --- a/test/box/net.box.result > > +++ b/test/box/net.box.result > > @@ -3902,6 +3902,66 @@ sock:close() > > --- > > - true > > ... > > +-- gh-1148: test stacked diagnostics. > > +-- > > +test_run:cmd("push filter \"file: .*\" to \"file: \"") > > 8. I removed the filter and nothing changed. Why do you > need it? This test has been reworked as well. > 9. Functional index is an overkill for such a simple test. You > may prefer creating a stored function, which creates a stack of 2 > errors, and calls box.error(). That would be simpler IMO, and you can > test more than 2 errors too. The same for the iproto.test.lua. To get > that error you can use IPROTO_CALL. No need to create spaces, indexes, > bother with some 'determenistic sandboxed' shit. No even need to call > box.schema.func.create assuming you have 'universe execute' access. diff --git a/test/box/iproto.result b/test/box/iproto.result index d6411d71a..b6dc7ed4c 100644 --- a/test/box/iproto.result +++ b/test/box/iproto.result @@ -7,16 +7,19 @@ urilib = require('uri') msgpack = require('msgpack') --- ... -IPROTO_REQUEST_TYPE = 0x00 +test_run = require('test_run').new() --- ... -IPROTO_INSERT = 0x02 +IPROTO_REQUEST_TYPE = 0x00 --- ... IPROTO_SYNC = 0x01 --- ... -IPROTO_SPACE_ID = 0x10 +IPROTO_CALL = 0x0A +--- +... +IPROTO_FUNCTION_NAME = 0x22 --- ... IPROTO_TUPLE = 0x21 @@ -36,20 +39,25 @@ IPROTO_ERROR_MESSAGE = 0x02 ... -- gh-1148: test capabilities of stacked diagnostics bypassing net.box. -- -lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]] ---- -... -box.schema.func.create('f1', {body = lua_func, is_deterministic = true, is_sandboxed = true}) ---- -... -s = box.schema.space.create('s') +test_run:cmd("setopt delimiter ';'") --- +- true ... -pk = s:create_index('pk') +stack_err = function() + local e1 = box.error.new({code = 111, reason = "e1"}) + local e2 = box.error.new({code = 111, reason = "e2"}) + local e3 = box.error.new({code = 111, reason = "e3"}) + assert(e1 ~= nil) + e2:set_prev(e1) + assert(e2.prev == e1) + e3:set_prev(e2) + box.error(e3) +end; --- ... -idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) +test_run:cmd("setopt delimiter ''"); --- +- true ... box.schema.user.grant('guest', 'read, write, execute', 'universe') --- @@ -58,14 +66,14 @@ next_request_id = 16 --- ... header = { \ - [IPROTO_REQUEST_TYPE] = IPROTO_INSERT, \ + [IPROTO_REQUEST_TYPE] = IPROTO_CALL, \ [IPROTO_SYNC] = next_request_id, \ } --- ... body = { \ - [IPROTO_SPACE_ID] = s.id, \ - [IPROTO_TUPLE] = box.tuple.new({1}) \ + [IPROTO_FUNCTION_NAME] = 'stack_err', \ + [IPROTO_TUPLE] = box.tuple.new({nil}) \ } --- ... @@ -99,31 +107,36 @@ assert(err[IPROTO_ERROR_MESSAGE] == response.body[IPROTO_ERROR]) --- - true ... -err = response.body[IPROTO_ERROR_STACK][2] +assert(err[IPROTO_ERROR_MESSAGE] == 'e3') --- +- true ... -assert(err[IPROTO_ERROR_CODE] ~= nil) +assert(err[IPROTO_ERROR_CODE] == 111) --- - true ... -assert(type(err[IPROTO_ERROR_CODE]) == 'number') +err = response.body[IPROTO_ERROR_STACK][2] --- -- true ... -assert(err[IPROTO_ERROR_MESSAGE] ~= nil) +assert(err[IPROTO_ERROR_MESSAGE] == 'e2') --- - true ... -assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string') +assert(err[IPROTO_ERROR_CODE] == 111) --- - true ... -box.schema.user.revoke('guest', 'read,write,execute', 'universe') +err = response.body[IPROTO_ERROR_STACK][3] --- ... -s:drop() +assert(err[IPROTO_ERROR_MESSAGE] == 'e1') --- +- true ... -box.func.f1:drop() +assert(err[IPROTO_ERROR_CODE] == 111) +--- +- true +... +box.schema.user.revoke('guest', 'read,write,execute', 'universe') --- ... diff --git a/test/box/iproto.test.lua b/test/box/iproto.test.lua index d0b55467b..6402a22ba 100644 --- a/test/box/iproto.test.lua +++ b/test/box/iproto.test.lua @@ -1,11 +1,13 @@ net_box = require('net.box') urilib = require('uri') msgpack = require('msgpack') +test_run = require('test_run').new() IPROTO_REQUEST_TYPE = 0x00 -IPROTO_INSERT = 0x02 + IPROTO_SYNC = 0x01 -IPROTO_SPACE_ID = 0x10 +IPROTO_CALL = 0x0A +IPROTO_FUNCTION_NAME = 0x22 IPROTO_TUPLE = 0x21 IPROTO_ERROR = 0x31 IPROTO_ERROR_STACK = 0x52 @@ -14,24 +16,29 @@ IPROTO_ERROR_MESSAGE = 0x02 -- gh-1148: test capabilities of stacked diagnostics bypassing net.box. -- -lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]] - -box.schema.func.create('f1', {body = lua_func, is_deterministic = true, is_sandboxed = true}) -s = box.schema.space.create('s') -pk = s:create_index('pk') -idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) - +test_run:cmd("setopt delimiter ';'") +stack_err = function() + local e1 = box.error.new({code = 111, reason = "e1"}) + local e2 = box.error.new({code = 111, reason = "e2"}) + local e3 = box.error.new({code = 111, reason = "e3"}) + assert(e1 ~= nil) + e2:set_prev(e1) + assert(e2.prev == e1) + e3:set_prev(e2) + box.error(e3) +end; +test_run:cmd("setopt delimiter ''"); box.schema.user.grant('guest', 'read, write, execute', 'universe') next_request_id = 16 header = { \ - [IPROTO_REQUEST_TYPE] = IPROTO_INSERT, \ + [IPROTO_REQUEST_TYPE] = IPROTO_CALL, \ [IPROTO_SYNC] = next_request_id, \ } body = { \ - [IPROTO_SPACE_ID] = s.id, \ - [IPROTO_TUPLE] = box.tuple.new({1}) \ + [IPROTO_FUNCTION_NAME] = 'stack_err', \ + [IPROTO_TUPLE] = box.tuple.new({nil}) \ } uri = urilib.parse(box.cfg.listen) @@ -47,12 +54,13 @@ assert(response.body[IPROTO_ERROR] ~= nil) err = response.body[IPROTO_ERROR_STACK][1] assert(err[IPROTO_ERROR_MESSAGE] == response.body[IPROTO_ERROR]) +assert(err[IPROTO_ERROR_MESSAGE] == 'e3') +assert(err[IPROTO_ERROR_CODE] == 111) err = response.body[IPROTO_ERROR_STACK][2] -assert(err[IPROTO_ERROR_CODE] ~= nil) -assert(type(err[IPROTO_ERROR_CODE]) == 'number') -assert(err[IPROTO_ERROR_MESSAGE] ~= nil) -assert(type(err[IPROTO_ERROR_MESSAGE]) == 'string') +assert(err[IPROTO_ERROR_MESSAGE] == 'e2') +assert(err[IPROTO_ERROR_CODE] == 111) +err = response.body[IPROTO_ERROR_STACK][3] +assert(err[IPROTO_ERROR_MESSAGE] == 'e1') +assert(err[IPROTO_ERROR_CODE] == 111) box.schema.user.revoke('guest', 'read,write,execute', 'universe') -s:drop() -box.func.f1:drop() diff --git a/test/box/net.box.result b/test/box/net.box.result index 764eef2c4..6475707ef 100644 --- a/test/box/net.box.result +++ b/test/box/net.box.result @@ -3904,63 +3904,74 @@ sock:close() ... -- gh-1148: test stacked diagnostics. -- -test_run:cmd("push filter \"file: .*\" to \"file: \"") +test_run:cmd("setopt delimiter ';'") --- - true ... -lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]] +stack_err = function() + local e1 = box.error.new({code = 111, reason = "e1"}) + local e2 = box.error.new({code = 111, reason = "e2"}) + local e3 = box.error.new({code = 111, reason = "e3"}) + assert(e1 ~= nil) + e2:set_prev(e1) + assert(e2.prev == e1) + e3:set_prev(e2) + box.error(e3) +end; --- ... -box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true}) +test_run:cmd("setopt delimiter ''"); --- +- true ... -s = box.schema.space.create('s') +box.schema.user.grant('guest', 'read,write,execute', 'universe') --- ... -pk = s:create_index('pk') +c = net.connect(box.cfg.listen) --- ... -idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) +f = function(...) return c:call(...) end --- ... -box.schema.user.grant('guest', 'read,write,execute', 'universe') +r, e3 = pcall(f, 'stack_err') --- ... -c = net.connect(box.cfg.listen) +assert(r == false) --- +- true ... -f = function(...) return c.space.s:insert(...) end +e3 --- +- e3 ... -_, e = pcall(f, {1}) +e2 = e3.prev --- ... -assert(e ~= nil) +assert(e2 ~= nil) --- - true ... -e:unpack().message +e2 --- -- 'Failed to build a key for functional index ''idx'' of space ''s'': can''t evaluate - function' +- e2 ... -e.prev.message +e1 = e2.prev --- -- '[string "return function(tuple) local json = require(''..."]:1: attempt to call - global ''require'' (a nil value)' ... -box.schema.user.revoke('guest', 'read,write,execute', 'universe') +assert(e1 ~= nil) --- +- true ... -s:drop() +e1 --- +- e1 ... -box.func.f1:drop() +assert(e1.prev == nil) --- +- true ... -test_run:cmd("clear filter") +box.schema.user.revoke('guest', 'read,write,execute', 'universe') --- -- true ... test_run:wait_log('default', 'Got a corrupted row.*', nil, 10) --- diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua index 7e3571d43..72a4207ee 100644 --- a/test/box/net.box.test.lua +++ b/test/box/net.box.test.lua @@ -1576,26 +1576,34 @@ sock:close() -- gh-1148: test stacked diagnostics. -- -test_run:cmd("push filter \"file: .*\" to \"file: \"") -lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]] -box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true}) -s = box.schema.space.create('s') -pk = s:create_index('pk') -idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) +test_run:cmd("setopt delimiter ';'") +stack_err = function() + local e1 = box.error.new({code = 111, reason = "e1"}) + local e2 = box.error.new({code = 111, reason = "e2"}) + local e3 = box.error.new({code = 111, reason = "e3"}) + assert(e1 ~= nil) + e2:set_prev(e1) + assert(e2.prev == e1) + e3:set_prev(e2) + box.error(e3) +end; +test_run:cmd("setopt delimiter ''"); box.schema.user.grant('guest', 'read,write,execute', 'universe') c = net.connect(box.cfg.listen) -f = function(...) return c.space.s:insert(...) end -_, e = pcall(f, {1}) -assert(e ~= nil) - -e:unpack().message -e.prev.message +f = function(...) return c:call(...) end +r, e3 = pcall(f, 'stack_err') +assert(r == false) +e3 +e2 = e3.prev +assert(e2 ~= nil) +e2 +e1 = e2.prev +assert(e1 ~= nil) +e1 +assert(e1.prev == nil) box.schema.user.revoke('guest', 'read,write,execute', 'universe') -s:drop() -box.func.f1:drop() -test_run:cmd("clear filter") test_run:wait_log('default', 'Got a corrupted row.*', nil, 10) test_run:wait_log('default', '00000000:.*', nil, 10) > > +--- > > +... > > +box.schema.user.grant('guest', 'read,write,execute', 'universe') > > +--- > > +... > > +c = net.connect(box.cfg.listen) > > +--- > > +... > > +f = function(...) return c.space.s:insert(...) end > > +--- > > +... > > +_, e = pcall(f, {1}) > > +--- > > +... > > +assert(e ~= nil) > > +--- > > +- true > > +... > > +e:unpack().message > > +--- > > +- 'Failed to build a key for functional index ''idx'' of space ''s'': can''t evaluate > > + function' > > +... > > +e.prev.message > > +--- > > +- error: '[string "return e.prev.message "]:1: attempt to index field ''prev'' (a > > + nil value)' > > 10. Something is wrong here, no? Yes. After rebase on master branch, new IPROTO_ERROR_STACK code has changed, but I forgot to updated net.box sources. Fixed.