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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

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.

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
>