From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9523D469719 for ; Sat, 31 Oct 2020 19:29:20 +0300 (MSK) From: sergos@tarantool.org Date: Sat, 31 Oct 2020 19:29:11 +0300 Message-Id: <20201031162911.61876-1-sergos@tarantool.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH v2] core: handle fiber cancellation for fiber.cond List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org, alexander.turenko@tarantool.org From: Sergey Ostanevich 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 @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