[Tarantool-patches] [PATCH v2] core: handle fiber cancellation for fiber.cond
Alexander V. Tikhonov
avtikhon at tarantool.org
Wed Dec 2 13:58:33 MSK 2020
Hi, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.
[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/222853269
On Sat, Oct 31, 2020 at 07:29:11PM +0300, 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
> 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().
> ---
>
> 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
> --
> 2.17.1
>
More information about the Tarantool-patches
mailing list