Re: [Tarantool-patches] [PATCH 7/7] iproto: support error stacked diagnostic area

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

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

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

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

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

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

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

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

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

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

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

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

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

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