Re: [Tarantool-patches] [PATCH v2] core: handle fiber cancellation for fiber.cond

[Sergey Ostanevich <sergos@tarantool.org>]

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@tarantool.org wrote:
> > From: Sergey Ostanevich <sergos@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@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
> >