Re: [Tarantool-patches] [PATCH v2 1/5] error: Add a Lua backtrace to error

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

> On 14.04.2020 4:11, 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.
>>
>> First of all there are some major issues with the
>> patch, which IMO should be clarified before it is pushed.
>>
>> 1) Why do we need the traceback in scope of 4398? It has
>> nothing to do with marshaling through IProto, nor with
>> custom error types.
> This is part of the errors.lua functional, which was attached as an example. After that it was also one of the functionality requested by Nazarov.

The original ticket does not contain any mentioning of a traceback:
https://github.com/tarantool/tarantool/issues/4398#issue-475632900

I agree that it was requested later, in comments, but still it
looks pretty separate from this, don't you agree?

>> 2) What to do with stacked errors? Currently only the first
>> error in the stack gets a traceback, because luaT_pusherror() is
>> called only on the top error in the stack. Consider this test:
>>
>>      box.cfg{}
>>      lua_code = [[function(tuple)
>>                      local json = require('json')
>>                      return json.encode(tuple)
>>                   end]]
>>      box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true, is_sandboxed = true})
>>      s = box.schema.space.create('withdata')
>>      pk = s:create_index('pk')
>>      idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}})
>>
>>      function test_func() return pcall(s.insert, s, {1}) end
>>      box.error.cfg{traceback_enable = true}
>>      ok, err = test_func()
>>
>>
>>      tarantool> err:unpack()
>>      ---
>>      - traceback: "stack traceback:\n\t[C]: at 0x010014d1b0\n\t[C]: in function 'test_func'\n\t[string
>>          \"ok, err = test_func()\"]:1: in main chunk\n\t[C]: in function 'pcall'\n\tbuiltin/box/console.lua:382:
>>          in function 'eval'\n\tbuiltin/box/console.lua:676: in function 'repl'\n\tbuiltin/box/console.lua:725:
>>          in function <builtin/box/console.lua:713>"
>>      ... <snipped>
>>
>>      tarantool> err.prev:unpack()
>>      ---
>>      - type: LuajitError
>>        message: '[string "return function(tuple)..."]:2: attempt to call global ''require''
>>          (a nil value)'
>>      ... <snipped>
>>
>> The second error does not have a traceback at all.
> (I added Turenko to To)
> I have two variants:
> - Leave as is and to document such behavior
> - Add the same traceback to all errors in the stack
> Alexander what do you think?

There is a third option - leave traceback out of this patchset for
a next release. Because they are clearly underdesigned. But on the
other hand it is not something critical. After all, we just can say,
that a traceback can be present, or can be not, and its content is
just a string, which can't be assumed to have any special format.

That would allow us to add/remove them and change their format anytime.

>> diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
>> index 4432bc89b..a616e87a4 100644
>> --- a/src/box/lua/error.cc
>> +++ b/src/box/lua/error.cc
>> @@ -112,9 +112,11 @@ raise:
>>       }
>>         struct error *err = box_error_new(file, line, code, "%s", reason);
>> -    if (tb_parsed)
>> -        err->traceback_mode = tb_mode;
>> -
>> +    /*
>> +     * Explicit traceback option overrides the global setting.
>> +     */
>> +    if (err != NULL && is_traceback_specified)
> 1) box_error_new don't return NULL

True, then 'err != NULL' check can be dropped.

>>   diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
>> index 1caa75ee9..833454bac 100644
>> --- a/src/lib/core/diag.c
>> +++ b/src/lib/core/diag.c
>> @@ -128,27 +125,14 @@ error_vformat_msg(struct error *e, const char *format, va_list ap)
>> -void
>> -error_set_traceback_supplementation(bool traceback_mode)
>> +error_set_traceback(struct error *e, const char *traceback)
>>   {
>> -    global_traceback_mode = traceback_mode;
>> +    assert(e->traceback == NULL);
> 2) Do I understand correctly that asserts only work on debug? Will this approach not be dangerous by memory leaks on release?

It should not be. Because the method is not public, and
in private methods it is never called with non-zero e->traceback.

>> +    e->traceback = strdup(traceback);
>> +    /*
>> +     * Don't try to set it again. Traceback can be NULL in case of OOM, so
>> +     * it is not a reliable source of information whether need to collect a
>> +     * traceback.
>> +     */
>> +    e->is_traceback_enabled = false;
>>   }
>> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
>> index f38009c54..e918d3089 100644
>> --- a/src/lib/core/diag.h
>> +++ b/src/lib/core/diag.h
>> @@ -42,6 +42,13 @@
>>   extern "C" {
>>   #endif /* defined(__cplusplus) */
>>   +/**
>> + * Flag to turn on/off traceback automatic collection. Currently
>> + * it works only for Lua. Stack is collected at the moment of
>> + * error object push onto the stack.
>> + */
>> +extern bool error_is_traceback_enabled;
> 3) Why a global variable is more preferable than static?

Static variables are also global. I just removed the setter, since
it is trivial.

>>   diff --git a/src/lua/error.lua b/src/lua/error.lua
>> index 46d28667f..535110588 100644
>> --- a/src/lua/error.lua
>> +++ b/src/lua/error.lua
>> @@ -26,8 +26,8 @@ struct error {
>>       char _errmsg[DIAG_ERRMSG_MAX];
>>       struct error *_cause;
>>       struct error *_effect;
>> -    char *lua_traceback;
>> -    bool traceback_mode;
>> +    char *traceback;
> 4) Replaced to err_traceback. It overlaps the traceback getter.
>> +    bool is_traceback_enabled;

Better replace it with _traceback. Similar to other members.