Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: lvasiliev <lvasiliev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org,
	Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v2 1/5] error: Add a Lua backtrace to error
Date: Thu, 16 Apr 2020 15:45:48 +0300	[thread overview]
Message-ID: <20200416124548.66rhbezr7ve6vx7o@tkn_work_nb> (raw)
In-Reply-To: <ab329357-c6df-0057-407e-2964bde8c638@tarantool.org>

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.

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

  reply	other threads:[~2020-04-16 12:45 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-10  8:10 [Tarantool-patches] [PATCH v2 0/5] Extending error functionality Leonid Vasiliev
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 1/5] error: Add a Lua backtrace to error Leonid Vasiliev
2020-04-14  1:11   ` Vladislav Shpilevoy
2020-04-15  9:25     ` lvasiliev
2020-04-16  0:00       ` Vladislav Shpilevoy
2020-04-16  1:11         ` Alexander Turenko
2020-04-16  8:58           ` lvasiliev
2020-04-16  9:30             ` Alexander Turenko
2020-04-16 12:27               ` lvasiliev
2020-04-16 12:45                 ` Alexander Turenko [this message]
2020-04-16 17:48                   ` lvasiliev
2020-04-16  8:40         ` lvasiliev
2020-04-16  9:04           ` lvasiliev
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 2/5] error: Add the custom error type Leonid Vasiliev
2020-04-14  1:11   ` Vladislav Shpilevoy
2020-04-15  9:25     ` lvasiliev
2020-04-16  0:02       ` Vladislav Shpilevoy
2020-04-16  9:18         ` lvasiliev
2020-04-16 21:03           ` Vladislav Shpilevoy
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 3/5] error: Increase the number of fields transmitted through IPROTO Leonid Vasiliev
2020-04-14  1:12   ` Vladislav Shpilevoy
2020-04-15  9:25     ` lvasiliev
2020-04-16  0:02       ` Vladislav Shpilevoy
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 4/5] iproto: Add session settings for IPROTO Leonid Vasiliev
2020-04-14  1:12   ` Vladislav Shpilevoy
2020-04-15  9:26     ` lvasiliev
2020-04-16  0:06       ` Vladislav Shpilevoy
2020-04-16  9:36         ` lvasiliev
2020-04-16 21:04           ` Vladislav Shpilevoy
2020-04-10  8:10 ` [Tarantool-patches] [PATCH v2 5/5] iproto: Update error MsgPack encoding Leonid Vasiliev
2020-04-14  1:12   ` Vladislav Shpilevoy
2020-04-15  9:25     ` lvasiliev
2020-04-16  0:11       ` Vladislav Shpilevoy
2020-04-16 10:04         ` lvasiliev
2020-04-16 21:06           ` Vladislav Shpilevoy
2020-04-14  1:10 ` [Tarantool-patches] [PATCH v2 0/5] Extending error functionality Vladislav Shpilevoy
2020-04-15  9:48   ` lvasiliev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200416124548.66rhbezr7ve6vx7o@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=lvasiliev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 1/5] error: Add a Lua backtrace to error' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox