[Tarantool-patches] [PATCH v2] core: handle fiber cancellation for fiber.cond
Sergey Ostanevich
sergos at tarantool.org
Thu Nov 5 23:42:27 MSK 2020
Hi!
On 04 ноя 13:00, Leonid Vasiliev wrote:
> Hi! Thank you for the patch.
> See some comments below:
>
> 1) The patch changes undocumented behavior, AFAIU.
> So, I have a question:"Do you plan to backport the patch to
> tarantool 1.10?". If the answer is "Yes" - I'm comfortable with the
> changes. But if the answer is "No" - I will object, because in this case
> both behaviors must be supported in all modules.
That's a very good point, Leonid! I didn't met any use of the '0' or '1'
result in C or in Lua for the case of fiber cancellation. Although it
might be not true about many projects in Tarantool ecosystem and -
what's more scary - in user's code in the multitude of installations.
Also, I have to admit that introduction of two types of behavior for
different versions of Tarantool will get things even worse.
I think we have to re-iterate the discussion of the change in the
ticket first.
Sergos
>
> 2) I think changing the behavior in C doesn't cause much of a problem,
> because before when you wait without timeout, you don't need to check
> the return value (it's always 0). But in Lua it will cause the problems,
> because now throws an error if cancelled and all wait calls should be
> wrapped to pcall.
>
> On 31.10.2020 19:29, sergos at tarantool.org wrote:
> > From: Sergey Ostanevich <sergos at tarantool.org>
> >
> > 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
>
> 3) behaviour -> behavior.
>
> > 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 <olegrok at tarantool.org>
> >
> > @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().
>
> 4) I don't think it's a good decision adding a comparison with
> fiber.channel():get() to the documentation. Up to you.
>
> 5) Document the changes in module.h.
>
> > ---
>
> 6) Add @ChangeLog.
>
> >
> > Github: https://gitlab.com/tarantool/tarantool/-/commits/sergos/gh-5013-fiber-cond
> > Issue: https://github.com/tarantool/tarantool/issues/5013
> >
> > 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);
> > 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;
> > return 0;
> > }
> > diff --git a/test/app-tap/gh-5013-fiber-cancel.test.lua b/test/app-tap/gh-5013-fiber-cancel.test.lua
> > new file mode 100755
> > index 000000000..ae805c5bf
> > --- /dev/null
> > +++ b/test/app-tap/gh-5013-fiber-cancel.test.lua
> > @@ -0,0 +1,23 @@
> > +#!/usr/bin/env tarantool
> > +
> > +local tap = require('tap')
> > +local fiber = require('fiber')
> > +local test = tap.test("gh-5013-fiber-cancel")
> > +
> > +test:plan(2)
> > +
> > +local result = {}
> > +
> > +function test_f()
> > + local cond = fiber.cond()
> > + local res, err = pcall(cond.wait, cond)
> > + result.res = res
> > + result.err = err
> > +end
> > +
> > +local f = fiber.create(test_f)
> > +f:cancel()
> > +fiber.yield()
> > +
> > +test:ok(result.res == false, tostring(result.res))
> > +test:ok(tostring(result.err) == 'fiber is cancelled', tostring(result.err))
> > diff --git a/test/box/net.box_fiber_cancel_gh-4834.result b/test/box/net.box_fiber_cancel_gh-4834.result
> > new file mode 100644
> > index 000000000..eab0a5e4d
> > --- /dev/null
> > +++ b/test/box/net.box_fiber_cancel_gh-4834.result
> > @@ -0,0 +1,65 @@
> > +-- test-run result file version 2
> > +remote = require 'net.box'
> > + | ---
> > + | ...
> > +fiber = require 'fiber'
> > + | ---
> > + | ...
> > +test_run = require('test_run').new()
> > + | ---
> > + | ...
> > +
> > +-- #4834: Cancelling fiber doesn't interrupt netbox operations
> > +function infinite_call() fiber.channel(1):get() end
> > + | ---
> > + | ...
> > +box.schema.func.create('infinite_call')
> > + | ---
> > + | ...
> > +box.schema.user.grant('guest', 'execute', 'function', 'infinite_call')
> > + | ---
> > + | ...
> > +
> > +error_msg = nil
> > + | ---
> > + | ...
> > +test_run:cmd("setopt delimiter ';'")
> > + | ---
> > + | - true
> > + | ...
> > +function gh4834()
> > + local cn = remote.connect(box.cfg.listen)
> > + local f = fiber.new(function()
> > + _, error_msg = pcall(cn.call, cn, 'infinite_call')
> > + end)
> > + f:set_joinable(true)
> > + fiber.yield()
> > + f:cancel()
> > + f:join()
> > + cn:close()
> > +end;
> > + | ---
> > + | ...
> > +test_run:cmd("setopt delimiter ''");
> > + | ---
> > + | - true
> > + | ...
> > +gh4834()
> > + | ---
> > + | ...
> > +error_msg
> > + | ---
> > + | - fiber is cancelled
> > + | ...
> > +box.schema.func.drop('infinite_call')
> > + | ---
> > + | ...
> > +infinite_call = nil
> > + | ---
> > + | ...
> > +channel = nil
> > + | ---
> > + | ...
> > +error_msg = nil
> > + | ---
> > + | ...
> > diff --git a/test/box/net.box_fiber_cancel_gh-4834.test.lua b/test/box/net.box_fiber_cancel_gh-4834.test.lua
> > new file mode 100644
> > index 000000000..06fb3ceac
> > --- /dev/null
> > +++ b/test/box/net.box_fiber_cancel_gh-4834.test.lua
> > @@ -0,0 +1,29 @@
> > +remote = require 'net.box'
> > +fiber = require 'fiber'
> > +test_run = require('test_run').new()
> > +
> > +-- #4834: Cancelling fiber doesn't interrupt netbox operations
> > +function infinite_call() fiber.channel(1):get() end
> > +box.schema.func.create('infinite_call')
> > +box.schema.user.grant('guest', 'execute', 'function', 'infinite_call')
> > +
> > +error_msg = nil
> > +test_run:cmd("setopt delimiter ';'")
> > +function gh4834()
> > + local cn = remote.connect(box.cfg.listen)
> > + local f = fiber.new(function()
> > + _, error_msg = pcall(cn.call, cn, 'infinite_call')
> > + end)
> > + f:set_joinable(true)
> > + fiber.yield()
> > + f:cancel()
> > + f:join()
> > + cn:close()
> > +end;
> > +test_run:cmd("setopt delimiter ''");
> > +gh4834()
> > +error_msg
> > +box.schema.func.drop('infinite_call')
> > +infinite_call = nil
> > +channel = nil
> > +error_msg = nil
> >
More information about the Tarantool-patches
mailing list