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

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Thu, Apr 16, 2020 at 11:58:03AM +0300, lvasiliev wrote:
> 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.

It is better to keep __serialize on track with newly added fields. I
would not bother with possibility that someone may call __serialize
manually or capture console output to compare against a sample.
Extending a map should be okay from backward-compatibility point of
view.

> 
> > > > > 
> > > > >       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.

You propose to introduce some kind of 'full' and 'partial' errors. It is
hard to document, because there is no rationale for this. When something
is introduced, it should be for the sake of something.

A kind of 'the API is complex, but, I see, it is highly flexible' or
'here I should take care of this peculiar, but OTOH some cases may be
processed much faster due to this'.