Tarantool development patches archive
 help / color / mirror / Atom feed
From: lvasiliev <lvasiliev@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 3/5] error: Increase the number of fields transmitted through IPROTO
Date: Wed, 15 Apr 2020 12:25:53 +0300	[thread overview]
Message-ID: <2a1c9210-d6d7-fa5f-7906-f6623139a28c@tarantool.org> (raw)
In-Reply-To: <dade5463-268a-e00b-4515-5ae0858c1ba4@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')
> 

  reply	other threads:[~2020-04-15  9:25 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-10  8:10 [Tarantool-patches] [PATCH v2 0/5] Extending error functionality Leonid Vasiliev
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 1/5] error: Add a Lua backtrace to error Leonid Vasiliev
2020-04-14  1:11   ` Vladislav Shpilevoy
2020-04-15  9:25     ` lvasiliev
2020-04-16  0:00       ` Vladislav Shpilevoy
2020-04-16  1:11         ` Alexander Turenko
2020-04-16  8:58           ` lvasiliev
2020-04-16  9:30             ` Alexander Turenko
2020-04-16 12:27               ` lvasiliev
2020-04-16 12:45                 ` Alexander Turenko
2020-04-16 17:48                   ` lvasiliev
2020-04-16  8:40         ` lvasiliev
2020-04-16  9:04           ` lvasiliev
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 2/5] error: Add the custom error type Leonid Vasiliev
2020-04-14  1:11   ` Vladislav Shpilevoy
2020-04-15  9:25     ` lvasiliev
2020-04-16  0:02       ` Vladislav Shpilevoy
2020-04-16  9:18         ` lvasiliev
2020-04-16 21:03           ` Vladislav Shpilevoy
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 3/5] error: Increase the number of fields transmitted through IPROTO Leonid Vasiliev
2020-04-14  1:12   ` Vladislav Shpilevoy
2020-04-15  9:25     ` lvasiliev [this message]
2020-04-16  0:02       ` Vladislav Shpilevoy
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 4/5] iproto: Add session settings for IPROTO Leonid Vasiliev
2020-04-14  1:12   ` Vladislav Shpilevoy
2020-04-15  9:26     ` lvasiliev
2020-04-16  0:06       ` Vladislav Shpilevoy
2020-04-16  9:36         ` lvasiliev
2020-04-16 21:04           ` Vladislav Shpilevoy
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 5/5] iproto: Update error MsgPack encoding Leonid Vasiliev
2020-04-14  1:12   ` Vladislav Shpilevoy
2020-04-15  9:25     ` lvasiliev
2020-04-16  0:11       ` Vladislav Shpilevoy
2020-04-16 10:04         ` lvasiliev
2020-04-16 21:06           ` Vladislav Shpilevoy
2020-04-14  1:10 ` [Tarantool-patches] [PATCH v2 0/5] Extending error functionality Vladislav Shpilevoy
2020-04-15  9:48   ` lvasiliev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2a1c9210-d6d7-fa5f-7906-f6623139a28c@tarantool.org \
    --to=lvasiliev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 3/5] error: Increase the number of fields transmitted through IPROTO' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox