From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 E3ACD4696C3 for ; Fri, 3 Apr 2020 04:54:16 +0300 (MSK) Date: Fri, 3 Apr 2020 01:54:16 +0000 From: Nikita Pettik Message-ID: <20200403015416.GB946@tarantool.org> References: <20200401160914.GC26447@tarantool.org> <50ab4dbe-0e2e-9d65-6087-4d01feb94b8e@tarantool.org> <20200402174232.GA31861@tarantool.org> <24e2df72-400f-11f9-25a2-eabf871e6cb5@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <24e2df72-400f-11f9-25a2-eabf871e6cb5@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 03 Apr 00:20, Vladislav Shpilevoy wrote: > 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. There's still 56 open issues assigned to 2.4 milestone. I doubt that all of them will be closed till Monday... > 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. Ok, as far as I understand, to ensure that diag_set() is always called before diag_add(). > 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. Agree. > 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); I added a few comments, check them out: diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h index 10d59d240..9c238f5a2 100644 --- a/src/lib/core/diag.h +++ b/src/lib/core/diag.h @@ -254,7 +254,12 @@ static inline void diag_add_error(struct diag *diag, struct error *e) { assert(e != NULL); + /* diag_set_error() should be called before. */ assert(diag->last != NULL); + /* + * e should be the bottom of its own stack. + * Otherwise some errors may be lost. + */ assert(e->effect == NULL); assert(diag->last->effect == NULL); error_ref(e); > + 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)