From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 6A9FC4696C4 for ; Tue, 14 Apr 2020 04:12:04 +0300 (MSK) References: <36240df4750932601c23b3b3fe4fe11e1034dfb1.1586505741.git.lvasiliev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 14 Apr 2020 03:12:02 +0200 MIME-Version: 1.0 In-Reply-To: <36240df4750932601c23b3b3fe4fe11e1034dfb1.1586505741.git.lvasiliev@tarantool.org> Content-Type: text/plain; charset=utf-8 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: Leonid Vasiliev Cc: tarantool-patches@dev.tarantool.org 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. > > 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. > +{ > + 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] > + > + 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? 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? Consider my review fixes below and on a new branch in a separate commit. 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')