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

[Vladislav Shpilevoy <v.shpilevoy@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)