Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area

[Nikita Pettik <korablev@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)