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

[lvasiliev <lvasiliev@tarantool.org>]

Hi! Thanks you for the feedback.

On 16.04.2020 3:00, Vladislav Shpilevoy wrote:
> 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?
At the beginning of my work on the task I said that it has three 
separate tasks:
1) Add Custom type.
2) Add traceback.
3) Marshalling through net.box.
But when I tried to send it for review separated it has not of any 
interest to reviewers, because the task didn't seem to be completed (as 
I understand). Or I could not correctly explain my point of view.
So, I agree that this looks like one of the separate parts of the task.
> 
>>> 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.
If I understand the problem correctly, now we will not have an error 
traceback for some errors (in the case of the stack diagnostic), if we 
delete the traceback patch, it will not be at all. Perhaps, it is more 
user-friendly to leave a traceback for at least part of the errors. If 
string is the normal format for a traceback (and I think it is), then it 
seems to me good to leave it as is and finalize in the future with some 
users feedbacks. But, if string is a bad data format for traceback then 
patch must be deleted from the patchset.
> 
>>> 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.
Ok, I understand you point of view.
> 
>>> +    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.
Hmm, ok.
> 
>>>    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.
Ok, I will replace it in the new version of the patchset if the 
traceback patch will be saved. But I can replace only traceback and 
traceback_mode(is_traceback_enabled), because other is "Unneccessary diff".
>