From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 B82C245C304 for ; Wed, 2 Dec 2020 13:58:35 +0300 (MSK) Date: Wed, 2 Dec 2020 13:58:33 +0300 From: "Alexander V. Tikhonov" Message-ID: <20201202105833.GA213381@hpalx> References: <20201031162911.61876-1-sergos@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201031162911.61876-1-sergos@tarantool.org> Subject: Re: [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: sergos@tarantool.org Cc: tarantool-patches@dev.tarantool.org 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@tarantool.org wrote: > 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 >