[Tarantool-patches] [PATCH v2 3/5] error: Increase the number of fields transmitted through IPROTO
lvasiliev
lvasiliev at tarantool.org
Wed Apr 15 12:25:53 MSK 2020
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')
>
More information about the Tarantool-patches
mailing list