[tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API

[Vladislav Shpilevoy]

Thanks for the answers!

There is a bug:

e1 = box.error.new({code = 123, reason = 'test'})
e2 = box.error.new({code = 123, reason = 'test2'})
e3 = box.error.new({code = 123, reason = 'test3'})

e2:wrap(e1)
e3:wrap(e2)
e1:wrap(e3)
e1:unpack()

This test leads to an infinite loop.

On 06/08/2019 09:56, Kirill Shcherbatov wrote:
> I'll make review fixes and I'll send a corresponding letter later.Now I'd like to discuss a few unclear moments.
> 
>>> err1 =
>>> err1:wrap(err) - add err error object as a reason for err1 object
>>>                  this call modifies err1 object and doesn't
>>>                  modify err object.
>>
>>> err1_parent =
>>> err1:unwrap()  - remove the most recent error in error object,
>>>                  return it's parent. the call has no effect when
>>>                  there is no parent in given error object.
>> 3. If it modifies err1, then why do I need to assign a result to
>> err1? The same below.
> 
> Let's start discussing this question with second :unwrap method.
> At first, some Lua variables point to error object (sic: not diag area);
> Therefore we need a way to return an unwrapped parent object to user.
> The implemented error:unwrap() method modifies an original error
> object(removing it's parent) and returns it's parent object.
> next_err = err:unpack()

'unpack'? It does not return a next error, it returns an array of
errors now. What am I missing?

If you are talking about 'unwrap', then ok, why do I need to
modify the error object at all? As I understand, 'unwrap' just
returns the reason object. How does it require to modify the
original error? Also, looks like I can't unwrap an error stack
more than once because of that - I need to build the stack
back, if I want to have several hooks processing the stack.

Currently I don't have that problem with 'unpack' - I can call it
multiple times on one error object. But I can't call 'unwrap'
multiple times.

> To make API consistent, I also return an error object in
> error:wrap(reason) method. This is the only reason for :wrap.
> We may get rid of it, if it is your strong opinion.

No, for me it is already clear, that unwrap should return an
error, and consistency is a good point. Lets keep 'wrap'
returning an error too. It is worth mentioning in the doc
request.

>>> +/**
>>> + * Wrap reason error object into given error.
>>> + * This API replaces box.error.last() value with an updated
>>> + * error object.
>>
>> 4. Why do you need to change the global error? And why is not
>> it mentioned in the docbot request?
> In many details my motivation is similar with (3.)th block:
> to make my API consistent.

Consistent with what? We've never had a similar function.
The only function, setting an error, was box_error_set. All
other functions were just returning error meta.

>  
> It is really important to enforce something taking a reference to parent
> error before unref(ing) it for self (for :unwrap) object. We would like to return reason for
> user, right? But self object make have the last reference to it. The delete method
> mustn't be called.

Not sure if I understood the sentences. What is a problem with
references? It is Lua, it manages references for you.

> Partially :wrap and :unwrap operations are constructors that introduce
> a new error.

Why 'partially'? What is missing?

> So changing box.error.last() seems for me reasonable.

In fact, there is another problem which you are trying to solve
with hacking behaviour of wrap and unwrap functions. The problem
is that neither C nor Lua API provide a way to add an error object
to the current one. They are missing 'diag_add'. You substituted
diag_add with wrap() changing the global diagnostics area. Although
perhaps it is ok. I don't think it would be better for users to
call box.error.add(box.error.wrap(reason)), so lets keep as is now.

TLDR: never mind.

>> 7. Unwrap does not allow to unwrap a leaf error.
>> But there is no API to determine if the error is
>> leaf. So a user can't determine when to stop calling
>> unwrap.> 
>> I am talking about C public API which you have changed
>> here. A user can't check error->reason != NULL before
>> calling box_error_unwrap.
> I don't mind: I've had a draft with such implementation.
> Let's do it so.
> 
>>> +err2 = nil
>>> + | ---
>>> + | ...
>>> +collectgarbage()
>>> + | ---
>>> + | - 0
>>> + | ...
>>
>> 10. Nit: you could nullify all the errors at once, and call
>> collectgarbage.
>> What do you mean?
> I consciously clean up the errors and call the garbage collector in these places.
> If you put an extra printf in error_unref/destructor you'll see why this is important.

Not sure if it is a good way to help with understanding tests. Please,
describe here the problem, in a comment.

> (also see your 4th question - this is a coverage tests for this problem)

I don't understand the problem in 4th point. Lua keeps references for you
until any singe ref is gone. Error object can't be deleted until there is
a reference on it.

>>> +
>>> +s:drop()
>>> + | ---
>>> + | ...
>> 11. In the RFC you said, that IProto returns a list of error. Where
>> it is?
> I haven't implemented this yet. Kostya said that we make do it later.
> 

Is there an issue about that?