From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (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 1F56F4696C4 for ; Wed, 15 Apr 2020 12:25:54 +0300 (MSK) References: <36240df4750932601c23b3b3fe4fe11e1034dfb1.1586505741.git.lvasiliev@tarantool.org> From: lvasiliev Message-ID: <2a1c9210-d6d7-fa5f-7906-f6623139a28c@tarantool.org> Date: Wed, 15 Apr 2020 12:25:53 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 3/5] error: Increase the number of fields transmitted through IPROTO List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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 = })` or > `box.error.new()`. > > 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') >