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

[lvasiliev <lvasiliev@tarantool.org>]

Hi! Thanks for the feedback.

On 16.04.2020 4:11, Alexander Turenko wrote:
>>>> 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>
> 
> BTW, can we call :split('\n') for .traceback field in at least
> __serialize? The cited output is hard to read. Alternative: place two
> newlines in row somewhere to force yaml serializer to use multiline
> string format.

Traceback is absent in __serialize, because it will change the error 
view for old client. If the client matches result with some pattern it, 
will be broken.

>>>>
>>>>       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?
> 
> The first approach look inconsistent. A user may want to get a cause of
> a topmost error and pass it somewhere. The function, where the error
> will be processed (say, serialized), don't know whether a traceback
> should be grabbed from some other error object (and how to find it).
> 
Not quite, you either have a traceback or not. Don't try to get it from 
another error.
>>
>> 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.
> 
> Are there other problems with traceback?
> 
> BTW, how traceback is linked with .trace?
> 
> Generally I'm for decomposing problems. Leonid, you may just keep the
> feature at top patches of your series. If it'll look ready, it will
> land. Otherwise, only first part will land.
> 
> I think that it is okay to implement marshalling over net.box first.
> 
> WBR, Alexander Turenko.
> 
As I said here 3 separate parts (Custom type, traceback and marshalling) 
but I don’t want to shuffle the patchset (patches have some dependencies 
from previous). I suggest just making a decision whether we will add a 
traceback or not. If not, I will simply remove this patch.