Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Nikita Pettik <korablev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area
Date: Fri, 3 Apr 2020 00:20:34 +0200	[thread overview]
Message-ID: <24e2df72-400f-11f9-25a2-eabf871e6cb5@tarantool.org> (raw)
In-Reply-To: <20200402174232.GA31861@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)

  reply	other threads:[~2020-04-02 22:20 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-25  1:42 [Tarantool-patches] [PATCH v2 00/10] Stacked diagnostics Nikita Pettik
2020-03-25  1:42 ` [Tarantool-patches] [PATCH v2 01/10] box: rfc for stacked diagnostic area Nikita Pettik
2020-03-25  8:27   ` Konstantin Osipov
2020-03-25 14:08     ` Nikita Pettik
2020-03-25  1:42 ` [Tarantool-patches] [PATCH v2 02/10] box: rename diag_add_error to diag_set_error Nikita Pettik
2020-03-25  8:27   ` Konstantin Osipov
2020-03-26  0:22   ` Vladislav Shpilevoy
2020-03-26 12:31     ` Nikita Pettik
2020-03-25  1:42 ` [Tarantool-patches] [PATCH v2 03/10] test: move box.error tests to box/error.test.lua Nikita Pettik
2020-03-25  8:28   ` Konstantin Osipov
2020-03-26  0:22   ` Vladislav Shpilevoy
2020-03-26 12:31     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 04/10] box/error: introduce box.error.set() method Nikita Pettik
2020-03-25  8:33   ` Konstantin Osipov
2020-03-25 17:41     ` Nikita Pettik
2020-03-26  0:22   ` Vladislav Shpilevoy
2020-03-26 12:31     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 05/10] box/error: don't set error created via box.error.new to diag Nikita Pettik
2020-03-26 16:50   ` Konstantin Osipov
2020-03-26 17:59     ` Nikita Pettik
2020-03-26 18:06       ` Nikita Pettik
2020-03-26 18:07       ` Alexander Turenko
2020-03-27  0:19   ` Vladislav Shpilevoy
2020-03-27 13:09     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area Nikita Pettik
2020-03-26 16:54   ` Konstantin Osipov
2020-03-26 18:03     ` Nikita Pettik
2020-03-26 18:08       ` Konstantin Osipov
2020-03-28 18:40   ` Vladislav Shpilevoy
2020-04-01 16:09     ` Nikita Pettik
2020-04-02  0:29       ` Vladislav Shpilevoy
2020-04-02 17:42         ` Nikita Pettik
2020-04-02 22:20           ` Vladislav Shpilevoy [this message]
2020-04-03  1:54             ` Nikita Pettik
2020-04-03 23:17               ` Vladislav Shpilevoy
2020-03-28 18:59   ` Vladislav Shpilevoy
2020-03-31 17:44     ` Nikita Pettik
2020-04-02  0:29       ` Vladislav Shpilevoy
2020-04-02 14:16         ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 07/10] box: use stacked diagnostic area for functional indexes Nikita Pettik
2020-03-30 23:24   ` Vladislav Shpilevoy
2020-04-01 15:53     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 08/10] box/error: clarify purpose of reference counting in struct error Nikita Pettik
2020-03-30 23:24   ` Vladislav Shpilevoy
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 09/10] iproto: refactor error encoding with mpstream Nikita Pettik
2020-03-30 23:24   ` Vladislav Shpilevoy
2020-04-01 15:54     ` Nikita Pettik
2020-03-25  1:43 ` [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area Nikita Pettik
2020-03-30 23:24   ` Vladislav Shpilevoy
2020-04-01 16:26     ` Nikita Pettik
2020-04-01 22:24       ` Nikita Pettik
2020-04-02  0:29         ` Vladislav Shpilevoy
2020-04-02 14:01           ` Nikita Pettik
2020-04-02 22:20             ` Vladislav Shpilevoy
2020-04-03  2:16               ` Nikita Pettik
2020-04-03 23:17                 ` Vladislav Shpilevoy
2020-04-06 11:07                   ` Nikita Pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=24e2df72-400f-11f9-25a2-eabf871e6cb5@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox