From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 76A0D4696C3 for ; Thu, 16 Apr 2020 20:48:59 +0300 (MSK) References: <831ae724-0d73-916b-0fad-dd52ce967664@tarantool.org> <20200416011118.3jbhcdtqpqr7lwit@tkn_work_nb> <20200416093037.vgtdm6xrto7g6qxb@tkn_work_nb> <20200416124548.66rhbezr7ve6vx7o@tkn_work_nb> From: lvasiliev Message-ID: Date: Thu, 16 Apr 2020 20:48:58 +0300 MIME-Version: 1.0 In-Reply-To: <20200416124548.66rhbezr7ve6vx7o@tkn_work_nb> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 1/5] error: Add a Lua backtrace to error List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy 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 " >>>>>>>>      ... >>>>> >>>>> 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)' >>>>>>>>      ... >>>>>>>> >>>>>>>> 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).