Re: [Tarantool-patches] [PATCH v2 3/5] error: Increase the number of fields transmitted through IPROTO

[lvasiliev <lvasiliev@tarantool.org>]

Hi! Thanks for the review.

On 14.04.2020 4:12, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Sorry, some of my comments may contain typos, because it is
> very late and I can't continue polishing it today.
> 
> See 4 comments below, review fixes in the end of the email,
> and on a new branch in a separate commit.
> 
> On 10/04/2020 10:10, Leonid Vasiliev wrote:
>> Has been added IPROTO_ERROR_TRACEBACK and IPROTO_ERROR_CUSTOM_TYPE
>> fields to IPROTO_ERROR_STACK for transfering through network
>> a backtrace and custom type.
> 
> 1. The new IProto codes are part of the public API as well, and should
> be documented.
Ok
> 
>>
>> Needed for #4398
>> ---
>>   src/box/iproto_constants.h |  2 ++
>>   src/box/lua/error.cc       | 23 +++++++++++++++++++++++
>>   src/box/lua/net_box.lua    | 21 ++++++++++++++++++++-
>>   src/box/xrow.c             | 19 ++++++++++++++++++-
>>   4 files changed, 63 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
>> index 1bcce1f..27dee7a 100644
>> --- a/src/box/lua/error.cc
>> +++ b/src/box/lua/error.cc
>> @@ -236,6 +236,25 @@ luaT_error_custom_type(lua_State *L)
>>   }
>>   
>>   static int
>> +luaT_error_set_lua_traceback(lua_State *L)
> 
> 2. This is not necessary as well as box.error.custom_type was not
> needed. Can be done via FFI.
Ok
> 
>> +{
>> +	if (lua_gettop(L) < 2)
>> +		return luaL_error(L, "Usage: box.error.set_lua_traceback"\
>> +				     "(error, traceback)");
>> +
>> +	struct error *e = luaL_checkerror(L, 1);
>> +
>> +	if (lua_type(L, 2) == LUA_TSTRING) {
>> +		error_set_lua_traceback(e, lua_tostring(L, 2));
>> +	} else {
>> +		return luaL_error(L, "Usage: box.error.set_lua_traceback"\
>> +				     "(error, traceback)");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
>> index 07fa54c..1e0cd7a 100644
>> --- a/src/box/lua/net_box.lua
>> +++ b/src/box/lua/net_box.lua
>> @@ -287,7 +289,24 @@ local function create_transport(host, port, user, password, callback,
>>                       local error = self.response[i]
>>                       local code = error[IPROTO_ERROR_CODE]
>>                       local msg = error[IPROTO_ERROR_MESSAGE]
>> -                    local new_err = box.error.new({code = code, reason = msg})
>> +                    local custom_type = error[IPROTO_ERROR_CUSTOM_TYPE]
>> +                    local traceback = error[IPROTO_ERROR_TRACEBACK]
After we decided don't concatenate the traceback, I decided that it 
would be inconsistent to transmit the traceback in the case of "throw" 
over the network, since when creating a new error, the "trace" is set 
new and it always has been. Furthermore, an error can change the type. 
So, I delete IPROTO_ERROR_TRACEBACK.If you consider it wrong decision, I 
can return IPROTO_ERROR_TRACEBACK.
>> +
>> +                    local new_err
>> +                    if custom_type then
>> +                        new_err = box.error.new({type = custom_type,
>> +                                                 reason = msg,
>> +                                                 traceback = false})
>> +                    else
>> +                        new_err = box.error.new({code = code,
>> +                                                 reason = msg,
>> +                                                 traceback = false})
> 
> 3. You can write all arguments in one table, box.error.new() should
> handle that fine.
> 
>> +                    end
>> +
>> +                    if traceback then
>> +                        box.error.set_lua_traceback(new_err, traceback)
>> +                    end
>> +
>>                       new_err:set_prev(prev)
>>                       prev = new_err
>>                   end
>> diff --git a/src/box/xrow.c b/src/box/xrow.c
>> index be026a4..cd88e49 100644
>> --- a/src/box/xrow.c
>> +++ b/src/box/xrow.c
>> @@ -494,11 +494,28 @@ mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
>>   	mpstream_encode_uint(stream, IPROTO_ERROR_STACK);
>>   	mpstream_encode_array(stream, err_cnt);
>>   	for (const struct error *it = error; it != NULL; it = it->cause) {
>> -		mpstream_encode_map(stream, 2);
>> +		/* Code and message are necessary fields */
>> +		uint32_t map_size = 2;
>> +		const char *custom_type = NULL;
>> +		if (it->lua_traceback)
>> +			++map_size;
>> +		if (strcmp(box_error_type(it), "CustomError") == 0) {
>> +			++map_size;
>> +			custom_type = box_custom_error_type(it);
>> +		}
>> +		mpstream_encode_map(stream, map_size);
>>   		mpstream_encode_uint(stream, IPROTO_ERROR_CODE);
>>   		mpstream_encode_uint(stream, box_error_code(it));
>>   		mpstream_encode_uint(stream, IPROTO_ERROR_MESSAGE);
>>   		mpstream_encode_str(stream, it->errmsg);
>> +		if (it->lua_traceback) {
>> +			mpstream_encode_uint(stream, IPROTO_ERROR_TRACEBACK);
>> +			mpstream_encode_str(stream, it->lua_traceback);
>> +		}
>> +		if (custom_type) {
>> +			mpstream_encode_uint(stream, IPROTO_ERROR_CUSTOM_TYPE);
>> +			mpstream_encode_str(stream, custom_type);
>> +		}
>>   	}
>>   }
> 
> 4. No any test. I wonder how did you check that it actually works?
That's my mistake. After our discussion for testing I used the test 
added in the last commit
> After I glanced at the last patch, I realized that looks like you
> implemented everything in a single huge commit, and then split it
> up, correct?
It's kind of true. After our discussion the patchset had to be heavily 
redesigned and this was done in one commit, which has been split after)
> 
> Consider my review fixes below and on a new branch in a
> separate commit.
> 
I applied your patch and added you to Co-authored.
> Branch is:
> 
>      lvasiliev/gh-4398-expose-error-module-4-review
> 
> ====================
> 
> ====================
>      Review fixes
>      
>      New commit message proposal:
>      
>      error: send traceback and custom type in IProto
>      
>      Error traceback and custom type features were added to the public
>      Lua API in the previous commits. This one makes the new attributes
>      being sent in IProto.
>      
>      @TarantoolBot document
>      Title: New error object attributes in IProto
>      
>      Error objects in IProto already have 2 fields:
>      `IPROTO_ERROR_CODE = 0x01` and `IPROTO_ERROR_MESSAGE = 0x02`.
>      
>      Now there are 2 more:
>      
>      `IPROTO_ERROR_TRACEBACK = 0x03` and
>      `IPROTO_ERROR_CUSTOM_TYPE = 0x04`.
>      
>      Both are optional, have MP_STR type, and speak for themselves.
>      Traceback is whatever the error object managed to collect from
>      the caller's stack when was pushed on it. It is visible in Lua via
>      `err_object.traceback`. Custom error type is another error object
>      attribute. This is what a user specifies in
>      `box.error.new({type = <custom_type>})` or
>      `box.error.new(<custom_type>)`.
> 
> diff --git a/extra/exports b/extra/exports
> index a5ebe0884..967e994c9 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -241,6 +241,7 @@ box_error_last
>   box_error_clear
>   box_error_set
>   error_set_prev
> +error_set_traceback
>   box_latch_new
>   box_latch_delete
>   box_latch_lock
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 1e0cd7aba..5c07bb07c 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -10,6 +10,11 @@ local urilib   = require('uri')
>   local internal = require('net.box.lib')
>   local trigger  = require('internal.trigger')
>   
> +ffi.cdef[[
> +void
> +error_set_traceback(struct error *e, const char *traceback);
> +]]
> +
>   local band              = bit.band
>   local max               = math.max
>   local fiber_clock       = fiber.clock
> @@ -287,26 +292,16 @@ local function create_transport(host, port, user, password, callback,
>                   local prev = nil
>                   for i = #self.response, 1, -1 do
>                       local error = self.response[i]
> -                    local code = error[IPROTO_ERROR_CODE]
> -                    local msg = error[IPROTO_ERROR_MESSAGE]
> -                    local custom_type = error[IPROTO_ERROR_CUSTOM_TYPE]
>                       local traceback = error[IPROTO_ERROR_TRACEBACK]
> -
> -                    local new_err
> -                    if custom_type then
> -                        new_err = box.error.new({type = custom_type,
> -                                                 reason = msg,
> -                                                 traceback = false})
> -                    else
> -                        new_err = box.error.new({code = code,
> -                                                 reason = msg,
> -                                                 traceback = false})
> +                    local new_err = box.error.new({
> +                        type = error[IPROTO_ERROR_CUSTOM_TYPE],
> +                        code = error[IPROTO_ERROR_CODE],
> +                        reason = error[IPROTO_ERROR_MESSAGE],
> +                        traceback = false
> +                    })
> +                    if traceback ~= nil then
> +                        ffi.C.error_set_traceback(new_err, traceback)
>                       end
> -
> -                    if traceback then
> -                        box.error.set_lua_traceback(new_err, traceback)
> -                    end
> -
>                       new_err:set_prev(prev)
>                       prev = new_err
>                   end
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index 04775d8ce..a9a6a6c75 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -494,7 +494,6 @@ mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
>   	mpstream_encode_uint(stream, IPROTO_ERROR_STACK);
>   	mpstream_encode_array(stream, err_cnt);
>   	for (const struct error *it = error; it != NULL; it = it->cause) {
> -		/* Code and message are necessary fields */
>   		uint32_t map_size = 2;
>   		const char *custom_type = box_error_custom_type(it);
>   		map_size += (it->traceback != NULL);
> diff --git a/test/box/error.result b/test/box/error.result
> index b717a4ff4..6db8813b4 100644
> --- a/test/box/error.result
> +++ b/test/box/error.result
> @@ -949,3 +949,59 @@ e = box.error.new({type = string.rep('a', 128)})
>    | ---
>    | - 63
>    | ...
> +
> +--
> +-- Check how traceback and custom error type are passed through
> +-- IProto.
> +--
> +netbox = require('net.box')
> + | ---
> + | ...
> +box.schema.user.grant('guest', 'super')
> + | ---
> + | ...
> +c = netbox.connect(box.cfg.listen)
> + | ---
> + | ...
> +
> +_, e = pcall(c.call, c, 't3', {true, true})
> + | ---
> + | ...
> +check_trace(e:unpack().traceback)
> + | ---
> + | - true
> + | ...
> +
> +_, e = pcall(c.call, c, 'box.error', {                                          \
> +    {code = 123, type = 'TestType', traceback = true, reason = 'Test reason'}   \
> +})
> + | ---
> + | ...
> +e = e:unpack()
> + | ---
> + | ...
> +e.traceback ~= nil or e.traceback
> + | ---
> + | - true
> + | ...
> +e.traceback = nil
> + | ---
> + | ...
> +e.trace = nil
> + | ---
> + | ...
> +e
> + | ---
> + | - code: 123
> + |   base_type: CustomError
> + |   type: TestType
> + |   custom_type: TestType
> + |   message: Test reason
> + | ...
> +
> +c:close()
> + | ---
> + | ...
> +box.schema.user.revoke('guest', 'super')
> + | ---
> + | ...
> diff --git a/test/box/error.test.lua b/test/box/error.test.lua
> index fe4051899..0821fa0a8 100644
> --- a/test/box/error.test.lua
> +++ b/test/box/error.test.lua
> @@ -278,3 +278,26 @@ e:unpack()
>   -- Try too long type name.
>   e = box.error.new({type = string.rep('a', 128)})
>   #e.type
> +
> +--
> +-- Check how traceback and custom error type are passed through
> +-- IProto.
> +--
> +netbox = require('net.box')
> +box.schema.user.grant('guest', 'super')
> +c = netbox.connect(box.cfg.listen)
> +
> +_, e = pcall(c.call, c, 't3', {true, true})
> +check_trace(e:unpack().traceback)
> +
> +_, e = pcall(c.call, c, 'box.error', {                                          \
> +    {code = 123, type = 'TestType', traceback = true, reason = 'Test reason'}   \
> +})
> +e = e:unpack()
> +e.traceback ~= nil or e.traceback
> +e.traceback = nil
> +e.trace = nil
> +e
> +
> +c:close()
> +box.schema.user.revoke('guest', 'super')
>