[Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area
Nikita Pettik
korablev at tarantool.org
Fri Apr 3 04:54:16 MSK 2020
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)
More information about the Tarantool-patches
mailing list