From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 061004696C3 for ; Fri, 3 Apr 2020 01:20:35 +0300 (MSK) References: <20200401160914.GC26447@tarantool.org> <50ab4dbe-0e2e-9d65-6087-4d01feb94b8e@tarantool.org> <20200402174232.GA31861@tarantool.org> From: Vladislav Shpilevoy Message-ID: <24e2df72-400f-11f9-25a2-eabf871e6cb5@tarantool.org> Date: Fri, 3 Apr 2020 00:20:34 +0200 MIME-Version: 1.0 In-Reply-To: <20200402174232.GA31861@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the fixes! >> Also a small comment about cycle detection in your patch. Here I >> would strongly-strongly recommend to avoid it when it is clearly >> not necessary. > > To be honest, I don't understand why do you care about this opimization so > much. Linking errors into list is unlikely to be hot execution path, so nobody > should bother with such minor improvement here. I am really afraid, that our solution team will find a way to shoot in their legs. For example, by doing some weird banking application producing error stacks with 50 errors and more, or they will add many errors when will forward them in the network from node to node (they actually want this, and are going to do that when we introduce custom errors), and will waste notable time on cycle checking. And probably will even hit the cycle error, and will complain. This is what I am afraid of. I am trying to save every CPU cycle. > Instead, keeping code clear, > simple and straightforward (your diff contains more branches and looks a bit > more complicated to me) must be the main goal. Applied diff below only due to > your recommendation. > > > Still, I haven't changed my mind. I didn't apply diff below, sorry. > I'll leave it to the second reviewer. Not sure there will be a second reviewer. From what I understand, the release is begin of next week. I am ok, lets leave it then as is. Sorry, I still couldn't finish with this commit. Made another amendment. See diff below, and on top of this commit on the branch. What I did is I made diag_add_error() assert, when the diag is empty. And when we are trying to add not a topmost error. To understand why see that case. Assume, we have a place which does diag_set() somewhere deep. And we have code calling this place and doing diag_add(). While diag_set() is there, we can't end up in infinitely growing stack if that error hits again and again. Diag_set() will drop the old stack. Then the diag_set() place is removed/changed in a way, that it does not set a diag, or also makes diag_add(). We will not notice this in the tests, and the stack will have a chance of growing infinitely. So I added an assertion to catch such changes right away. I hope you agree. I added one another tiny commit on top of the last patch to make box_error_add() ok with that. This is a public function, and in there it is ok to both add and set. Also I added a test on what happens when you box.error.set() middle of an error stack. ==================== diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h index 276bf2362..10d59d240 100644 --- a/src/lib/core/diag.h +++ b/src/lib/core/diag.h @@ -254,11 +254,12 @@ static inline void diag_add_error(struct diag *diag, struct error *e) { assert(e != NULL); + assert(diag->last != NULL); + assert(e->effect == NULL); + assert(diag->last->effect == NULL); error_ref(e); - error_unlink_effect(e); e->cause = diag->last; - if (diag->last != NULL) - diag->last->effect = e; + diag->last->effect = e; diag->last = e; } diff --git a/test/box/error.result b/test/box/error.result index a4e8a8d8d..3d07f6e64 100644 --- a/test/box/error.result +++ b/test/box/error.result @@ -789,3 +789,22 @@ assert(e1.prev == e2) | --- | - true | ... + +-- Set middle of an error stack into the diagnostics area. +e1:set_prev(e2) + | --- + | ... +e2:set_prev(e3) + | --- + | ... +box.error.set(e2) + | --- + | ... +assert(e1.prev == nil) + | --- + | - true + | ... +assert(e2.prev == e3) + | --- + | - true + | ... diff --git a/test/box/error.test.lua b/test/box/error.test.lua index f03fb4d36..ed7eb7565 100644 --- a/test/box/error.test.lua +++ b/test/box/error.test.lua @@ -214,3 +214,10 @@ assert(e2.code == 111) box.error.set(e1) box.error.clear() assert(e1.prev == e2) + +-- Set middle of an error stack into the diagnostics area. +e1:set_prev(e2) +e2:set_prev(e3) +box.error.set(e2) +assert(e1.prev == nil) +assert(e2.prev == e3)