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 15:45, Alexander Turenko wrote:
> On Thu, Apr 16, 2020 at 03:27:35PM +0300, lvasiliev wrote:
>>
>>
>> On 16.04.2020 12:30, Alexander Turenko wrote:
>>> 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.
>>>
>> Now __serialize is used for transfer old style error too.
> 
> As we discussed, a serializer should not see at __serialize of a type it
> knows natively. Let's keep __serialize for REPL only.
> 
Discussed with Alexander. Will be moved to a separate task. Now we
continue to use __serialize to transfer the error through net.box in the 
old style.
>>>>
>>>>>>>>
>>>>>>>>         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.
>> No, the error without traceback is not 'partial'. If
>> global error_is_traceback_enabled is false - all errors haven't a
>> traceback. If error creates with traceback=false, it hasn't a traceback.
> 
> So we anyway need to docuemnt a rule when an error will contain the
> traceback and the way you implement it does not add much complexity.
> 
> I can buy this point.
>  >>>
>>> 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'.
>>>
>> You are hyperbolizing. It can be regarded as "Technical debt".
>> If you insist, I can remove this patch.
> 
> I didn't review the patchset, just looked at the question, so I cannot
> insist here.
> 
> Personally I have questions around traceback (see [1]), so I would
> prefer safe way: concentrate on marshaling, postpone traceback.
> 
> However I don't feel myself in power to decide.
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016043.html
>
We omit traceback and implement it later (after discussion with
Alexander Turenko).