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

[Vladislav Shpilevoy <v.shpilevoy@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 = <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')