From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B948D2582A for ; Tue, 6 Aug 2019 16:47:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ys-xvufexgVV for ; Tue, 6 Aug 2019 16:47:59 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DDE3E25827 for ; Tue, 6 Aug 2019 16:47:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 3/3] box: extend ffi error object API References: <47d9072e60bdc563c7466a4e51db1a61bc71a610.1564657285.git.kshcherbatov@tarantool.org> <55d56797-f937-4088-fdba-7d9dfef51ae6@tarantool.org> From: Vladislav Shpilevoy Message-ID: <50e7c5e4-ec32-893b-ba47-cf8664ddb029@tarantool.org> Date: Tue, 6 Aug 2019 22:50:22 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov 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?