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

[Kirill Shcherbatov]

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

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.

>> +/**
>> + * 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.
 
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. Partially :wrap and :unwrap operations are constructors that introduce
a new error. So changing box.error.last() seems for me reasonable.

> 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.
(also see your 4th question - this is a coverage tests for this problem)

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