From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 05CA1469719 for ; Sun, 23 Feb 2020 20:43:56 +0300 (MSK) References: <98b175140b1bf2e2f9d3285ca281008b9d7b6c11.1582119629.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <2a5f311b-54cd-9b82-af60-1b95397ed687@tarantool.org> Date: Sun, 23 Feb 2020 18:43:55 +0100 MIME-Version: 1.0 In-Reply-To: <98b175140b1bf2e2f9d3285ca281008b9d7b6c11.1582119629.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Thanks for the patch! See 10 comments below. On 19/02/2020 15:16, Nikita Pettik wrote: > This patch introduces support of stacked errors in IProto protocol and > in net.box module. > > @TarantoolBot document > Title: Stacked error diagnostic area > > Starting from now errors can be organized into lists. To achieve this > Lua table representing error object is extended with .prev field and > e:set_prev(err) method. .prev field returns previous error if any exist. > e:set_prev(err) method expects err to be error object or nil and sets > err as previous error of e. For instance: > > e1 = box.error.new({code = 111, reason = "cause"}) > e2 = box.error.new({code = 111, reason = "cause of cause"}) > > e1:set_prev(e2) > assert(e1.prev == e2) -- true > > Cycles are not allowed for error lists: > > e2:set_prev(e1) > - error: 'builtin/error.lua: Cycles are not allowed' > > Nil is valid input to :set_prev() method: > > e1:set_prev(nil) > assert(e1.prev == nil) -- true > > Note that error can be 'previous' only to the one error at once: > > e1:set_prev(e2) > e3:set_prev(e2) > assert(e1.prev == nil) -- true > assert(e3.prev == e2) -- true > > Setting previous error does not erase its own previous members: > > -- e1 -> e2 -> e3 -> e4 > e1:set_prev(e2) > e2:set_prev(e3) > e3:set_prev(e4) > e2:set_prev(e5) > -- Now there are two lists: e1->e2->e5 and e3->e4 > assert(e1.prev == e2) -- true > assert(e2.prev == e5) -- true > assert(e3.prev == e4) -- true > > Alternatively: > > e1:set_prev(e2) > e2:set_prev(e3) > e3:set_prev(e4) > e5:set_prev(e3) > -- Now there are two lists: e1->e2 and e5->e3->e4 > assert(e1.prev == e2) -- true > assert(e2.prev == nil) -- true > assert(e5.prev == e3) -- true > assert(e3.prev == e4) -- true > > Stacked diagnostics is also supported by IProto protocol. Now responses > containing errors always (even there's only one error to be returned) 1. there's -> if there's. > include new IProto key: IPROTO_ERROR_STACK (0x51). So, body corresponding to > error response now looks like: > MAP{IPROTO_ERROR : string, IPROTO_ERROR_STACK : ARRAY[MAP{ERROR_CODE : uint, ERROR_MESSAGE : string}, MAP{...}, ...]} > > where IPROTO_ERROR is 0x31 key, IPROTO_ERROR_STACK is 0x51, ERROR_CODE > is 0x01 and ERROR_MESSAGE is 0x02. > Instances of older versions (without support of stacked errors in > protocol) simply ignore unknown keys and still rely only on IPROTO_ERROR > key. > > Closes #1148 > --- > src/box/error.cc | 17 +++++ > src/box/error.h | 16 +++++ > src/box/iproto_constants.h | 6 ++ > src/box/lua/net_box.lua | 32 ++++++++- > src/box/xrow.c | 78 +++++++++++++++++++-- > test/box-py/iproto.result | 6 +- > test/box-py/iproto.test.py | 6 +- > test/box/iproto.result | 166 +++++++++++++++++++++++++++++++++++++++++++++ > test/box/iproto.test.lua | 73 ++++++++++++++++++++ > test/box/net.box.result | 65 ++++++++++++++++++ > test/box/net.box.test.lua | 25 +++++++ > 11 files changed, 475 insertions(+), 15 deletions(-) > create mode 100644 test/box/iproto.result > create mode 100644 test/box/iproto.test.lua > > diff --git a/src/box/error.h b/src/box/error.h > index 42043ef80..626701f27 100644 > --- a/src/box/error.h > +++ b/src/box/error.h > @@ -137,6 +137,22 @@ struct error * > box_error_construct(const char *file, unsigned line, uint32_t code, > const char *fmt, ...); > > +/** > + * Add error to the diagnostic area. In contrast to box_error_set() > + * it does not replace previous error being set, but rather link > + * them into list. > + * > + * \param code IPROTO error code (enum \link box_error_code \endlink) > + * \param format (const char * ) - printf()-like format string > + * \param ... - format arguments > + * \returns -1 for convention use > + * > + * \sa enum box_error_code > + */ > +int > +box_error_add(const char *file, unsigned line, uint32_t code, > + const char *fmt, ...); 2. Lets keep it out of the public C API for now. We can add it later, when somebody asks. > + > /** > * A backward-compatible API define. > */ > diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h > index b66c05c06..a77660018 100644 > --- a/src/box/iproto_constants.h > +++ b/src/box/iproto_constants.h> @@ -149,6 +150,11 @@ enum iproto_ballot_key { > IPROTO_BALLOT_IS_LOADING = 0x04, > }; > > +enum iproto_error_key { > + IPROTO_ERROR_CODE = 0x01, > + IPROTO_ERROR_MESSAGE = 0x02, 3. I would use normal decimal numbers, and start from 0. There is no any reason why should it be hex and start from 1. Up to you. > +}; > + > #define bit(c) (1ULL< > #define IPROTO_HEAD_BMAP (bit(REQUEST_TYPE) | bit(SYNC) | bit(REPLICA_ID) |\ > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua > index b4811edfa..9a619e3d4 100644 > --- a/src/box/lua/net_box.lua > +++ b/src/box/lua/net_box.lua > @@ -277,8 +280,24 @@ local function create_transport(host, port, user, password, callback, > -- > function request_index:result() > if self.errno then > - return nil, box.error.new({code = self.errno, > - reason = self.response}) > + if type(self.response) == 'table' then > + -- Decode and fill in error stack. > + local prev = nil > + for i = #self.response, 1, -1 do 4. Why do you start from the end? Seems like you could easily do the same with the direct iteration. Your way is not worse, but it raises unnecessary questions. > + local error = self.response[i] > + local code = error[IPROTO_ERROR_CODE] > + local msg = error[IPROTO_ERROR_MESSAGE] > + assert(type(code) == 'number') > + assert(type(msg) == 'string') > + local new_err = box.error.new({code = code, reason = msg}) > + new_err:set_prev(prev) > + prev = new_err > + end > + return nil, prev > + else > + return nil, box.error.new({code = self.errno, > + reason = self.response}) > + end > elseif not self.id then > return self.response > elseif not worker_fiber then > diff --git a/src/box/xrow.c b/src/box/xrow.c > index 3f1c90c87..b8c78cbc5 100644 > --- a/src/box/xrow.c > +++ b/src/box/xrow.c > @@ -1072,6 +1085,48 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len, > return 0; > } > > +static int > +iproto_decode_error_stack(const char **pos) > +{ > + char *reason = tt_static_buf(); > + static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\ > + "expected to be larger than error message max length"); > + /* > + * Erase previously set diag errors. It is required since > + * box_error_add() does not replace previous errors. > + */ > + box_error_clear(); > + uint32_t stack_sz = mp_decode_array(pos); > + for (uint32_t i = 0; i < stack_sz; i++) { > + uint32_t code = 0; > + if (mp_typeof(**pos) != MP_MAP) > + return -1; > + uint32_t map_sz = mp_decode_map(pos); 5. Before calling any decode() you need to call a corresponding check(). Otherwise a truncated packet can crash Tarantool. Check other xrow decoders and net.box.test.lua for corruption tests. > + for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) { > + if (mp_typeof(**pos) != MP_UINT) > + return -1; > + uint8_t key = mp_decode_uint(pos); > + if (key == IPROTO_ERROR_CODE) { > + if (mp_typeof(**pos) != MP_UINT) > + return -1; > + code = mp_decode_uint(pos); > + } else if (key == IPROTO_ERROR_MESSAGE) { > + if (mp_typeof(**pos) != MP_STR) > + return -1; > + uint32_t len; > + const char *str = mp_decode_str(pos, &len); > + snprintf(reason, TT_STATIC_BUF_LEN, "%.*s", > + len, str); 6. Specifically for this we have tt_cstr() function. Lets use it here. > + } else { > + mp_next(pos); > + continue; > + } > + box_error_add(__FILE__, __LINE__, code, reason); 7. Someday we should send file and line in iproto too. Not related to your patch tho. > + } > + } > + return 0; > +} > + > void > xrow_decode_error(struct xrow_header *row) > { > @@ -1098,15 +1153,26 @@ xrow_decode_error(struct xrow_header *row) > continue; > } > uint8_t key = mp_decode_uint(&pos); > - if (key != IPROTO_ERROR || mp_typeof(*pos) != MP_STR) { > - mp_next(&pos); /* value */ > + if (key == IPROTO_ERROR && mp_typeof(*pos) == MP_STR) { > + /* > + * Obsolete way of sending error responses. > + * To be deprecated but still should be supported > + * to not break backward compatibility. > + */ > + uint32_t len; > + const char *str = mp_decode_str(&pos, &len); > + snprintf(error, sizeof(error), "%.*s", len, str); > + box_error_set(__FILE__, __LINE__, code, error); > + } else if (key == IPROTO_ERROR_STACK && > + mp_typeof(*pos) == MP_ARRAY) { 8. If we got an error stack, but it is not an array, this looks like a broken packet. > + if (iproto_decode_error_stack(&pos) != 0) > + continue; > + } else { > + mp_next(&pos); > continue; > } > - > - uint32_t len; > - const char *str = mp_decode_str(&pos, &len); > - snprintf(error, sizeof(error), "%.*s", len, str); > } > diff --git a/test/box/iproto.result b/test/box/iproto.result > new file mode 100644 > index 000000000..28b8bf140 > --- /dev/null > +++ b/test/box/iproto.result > @@ -0,0 +1,166 @@ > +test_run = require('test_run').new() > +--- > +... > +net_box = require('net.box') > +--- > +... > +urilib = require('uri') > +--- > +... > +msgpack = require('msgpack') > +--- > +... > +IPROTO_REQUEST_TYPE = 0x00 > +--- > +... > +IPROTO_INSERT = 0x02 > +--- > +... > +IPROTO_SYNC = 0x01 > +--- > +... > +IPROTO_SPACE_ID = 0x10 > +--- > +... > +IPROTO_TUPLE = 0x21 > +--- > +... > +IPROTO_ERROR = 0x31 > +--- > +... > +IPROTO_ERROR_STACK = 0x51 > +--- > +... > +IPROTO_ERROR_CODE = 0x01 > +--- > +... > +IPROTO_ERROR_MESSAGE = 0x02 > +--- > +... > +IPROTO_OK = 0x00 > +--- > +... > +IPROTO_SCHEMA_VERSION = 0x05 > +--- > +... > +IPROTO_STATUS_KEY = 0x00 > +--- > +... > +-- gh-1148: test capabilities of stacked diagnostics bypassing net.box. > +-- > +test_run:cmd("setopt delimiter ';'") > +--- > +- true > +... > +lua_func = [[function(tuple) local json = require('json') return json.encode(tuple) end]] > +test_run:cmd("setopt delimiter ''"); 9. Why do you need a custom delimiter for one line? The same in the net.box test. > diff --git a/test/box/net.box.result b/test/box/net.box.result > index e3dabf7d9..c5d5d3743 100644 > --- a/test/box/net.box.result > +++ b/test/box/net.box.result > @@ -3902,6 +3902,71 @@ sock:close() > --- > - true > ... > +-- gh-1148: test stacked diagnostics. > +-- > +test_run:cmd("push filter \"file: .*\" to \"file: \"") > +--- > +- true > +... > +test_run:cmd("setopt delimiter ';'") > +--- > +- true > +... > +lua_code = [[function(tuple) local json = require('json') return json.encode(tuple) end]] > +test_run:cmd("setopt delimiter ''"); > +--- > +... > +box.schema.func.create('f1', {body = lua_code, is_deterministic = true, is_sandboxed = true}) > +--- > +... > +s = box.schema.space.create('s') > +--- > +... > +pk = s:create_index('pk') > +--- > +... > +idx = s:create_index('idx', {func = box.func.f1.id, parts = {{1, 'string'}}}) > +--- > +... > +box.schema.user.grant('guest', 'read,write,execute', 'universe') > +--- > +... > +c = net.connect(box.cfg.listen) > +--- > +... > +f = function(...) return c.space.s:insert(...) end > +--- > +... > +_, e = pcall(f, {1}) > +--- > +... > +assert(e ~= nil) > +--- > +- true > +... > +e:unpack().message 10. You don't need to call unpack() to get a message. Just write e.message.