<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi! Thanks for changes. See two comments below.<br>
</p>
<div class="moz-cite-prefix">On 31/10/2020 19:29,
<a class="moz-txt-link-abbreviated" href="mailto:sergos@tarantool.org">sergos@tarantool.org</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20201031162911.61876-1-sergos@tarantool.org">
<pre class="moz-quote-pre" wrap="">From: Sergey Ostanevich <a class="moz-txt-link-rfc2396E" href="mailto:sergos@tarantool.org"><sergos@tarantool.org></a>
Hi!
Thanks to Oleg Babin's comment I found there's no need to update any lua
interfaces, since the reason was in C implementation. Also, there is one
place the change is played, so after I fixed it I got complete testing
pass.
Force-pushed branch, v2 patch attached.
Before this patch fiber.cond():wait() just returns for cancelled
fiber. In contrast fiber.channel():get() threw "fiber is
canceled" error.
This patch unify behaviour of channels and condvars and also fixes
related net.box module problem - it was impossible to interrupt
net.box call with fiber.cancel because it used fiber.cond under
the hood. Test cases for both bugs are added.
Closes #4834
Closes #5013
Co-authored-by: Oleg Babin <a class="moz-txt-link-rfc2396E" href="mailto:olegrok@tarantool.org"><olegrok@tarantool.org></a>
@TarantoolBot document
Title: fiber.cond():wait() throws if fiber is cancelled
Currently fiber.cond():wait() throws an error if waiting fiber is
cancelled like in case with fiber.channel():get().
---
Github: <a class="moz-txt-link-freetext" href="https://gitlab.com/tarantool/tarantool/-/commits/sergos/gh-5013-fiber-cond">https://gitlab.com/tarantool/tarantool/-/commits/sergos/gh-5013-fiber-cond</a>
Issue: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/issues/5013">https://github.com/tarantool/tarantool/issues/5013</a>
src/box/box.cc | 6 +-
src/lib/core/fiber_cond.c | 1 +
test/app-tap/gh-5013-fiber-cancel.test.lua | 23 +++++++
test/box/net.box_fiber_cancel_gh-4834.result | 65 +++++++++++++++++++
.../box/net.box_fiber_cancel_gh-4834.test.lua | 29 +++++++++
5 files changed, 120 insertions(+), 4 deletions(-)
create mode 100755 test/app-tap/gh-5013-fiber-cancel.test.lua
create mode 100644 test/box/net.box_fiber_cancel_gh-4834.result
create mode 100644 test/box/net.box_fiber_cancel_gh-4834.test.lua
diff --git a/src/box/box.cc b/src/box/box.cc
index 18568df3b..bfa1051f9 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -305,10 +305,8 @@ box_wait_ro(bool ro, double timeout)
{
double deadline = ev_monotonic_now(loop()) + timeout;
while (is_box_configured == false || box_is_ro() != ro) {
- if (fiber_cond_wait_deadline(&ro_cond, deadline) != 0)
- return -1;
- if (fiber_is_cancelled()) {
- diag_set(FiberIsCancelled);
+ if (fiber_cond_wait_deadline(&ro_cond, deadline) != 0) {
+ if (fiber_is_cancelled()) diag_set(FiberIsCancelled);</pre>
</blockquote>
<p>Here you use spaces instead of tabs.<br>
</p>
<blockquote type="cite"
cite="mid:20201031162911.61876-1-sergos@tarantool.org">
<pre class="moz-quote-pre" wrap="">
return -1;
}
}
diff --git a/src/lib/core/fiber_cond.c b/src/lib/core/fiber_cond.c
index 904a350d9..b0645069e 100644
--- a/src/lib/core/fiber_cond.c
+++ b/src/lib/core/fiber_cond.c
@@ -108,6 +108,7 @@ fiber_cond_wait_timeout(struct fiber_cond *c, double timeout)
diag_set(TimedOut);
return -1;
}
+ if (fiber_is_cancelled()) return -1;</pre>
</blockquote>
<p>It's qute strange to return -1 here but don't set a reason to
diagnostic area. Look how it is done for channels</p>
<p>(<a class="moz-txt-link-freetext" href="https://github.com/tarantool/tarantool/blob/42c64d06d5d1a3ec937b3c596af083a672a68ad8/src/lib/core/fiber_channel.c#L180">https://github.com/tarantool/tarantool/blob/42c64d06d5d1a3ec937b3c596af083a672a68ad8/src/lib/core/fiber_channel.c#L180</a>).</p>
<p>There is some inconsistency without it.</p>
<p>I've looked a bit deeper at the failure I reported before. Seems
the problem is in "cbus_unpair" function.</p>
<p>The problem appears only if FiberIsCancelled is setted to diag
area in "fiber_cond_wait" function.</p>
<p>This is where my expertise ends, as I'm not familiar with "cbus".
However I have some minds how it could be eliminated.</p>
<p>Let's declare cbus_unpair fiber as is not cancellable and stop
report is_cancellable flag for non-cancellable fibers. See some
PoC below:</p>
<p><br>
</p>
<p>diff --git a/src/lib/core/cbus.c b/src/lib/core/cbus.c<br>
index 5d91fb948..4167c756a 100644<br>
--- a/src/lib/core/cbus.c<br>
+++ b/src/lib/core/cbus.c<br>
@@ -630,6 +630,7 @@ cbus_unpair(struct cpipe *dest_pipe, struct
cpipe *src_pipe,<br>
msg.unpair_arg = unpair_arg;<br>
msg.src_pipe = src_pipe;<br>
msg.complete = false;<br>
+ fiber_set_cancellable(false);<br>
fiber_cond_create(&msg.cond);<br>
<br>
cpipe_push(dest_pipe, &msg.cmsg);<br>
@@ -643,6 +644,7 @@ cbus_unpair(struct cpipe *dest_pipe, struct
cpipe *src_pipe,<br>
fiber_cond_wait(&msg.cond);<br>
}<br>
<br>
+ fiber_set_cancellable(true);<br>
cpipe_destroy(dest_pipe);<br>
}<br>
<br>
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c<br>
index 483ae3ce1..8100c9da6 100644<br>
--- a/src/lib/core/fiber.c<br>
+++ b/src/lib/core/fiber.c<br>
@@ -553,6 +553,9 @@ fiber_set_cancellable(bool yesno)<br>
bool<br>
fiber_is_cancelled(void)<br>
{<br>
+ if ((fiber()->flags & FIBER_IS_CANCELLABLE) == 0) {<br>
+ return false;<br>
+ }<br>
return fiber()->flags & FIBER_IS_CANCELLED;<br>
}</p>
<p><br>
</p>
<p>To be honest I've not checked such change carefully and also have
segfault at replication/gc.test.lua for "memtx" engine.</p>
<p>Finally, feel free to ignore this comment I hope Vlad or Sasha
can give you more accurate and correct advices.<br>
<span style="color: rgb(36, 41, 46); font-family: SFMono-Regular, Consolas, "Liberation Mono", Menlo, monospace; font-size: 12px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; white-space: pre; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(255, 255, 255); text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;"></span></p>
<p>
</p>
<blockquote type="cite"
cite="mid:20201031162911.61876-1-sergos@tarantool.org">
</blockquote>
</body>
</html>