From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 8C718469719 for ; Wed, 4 Nov 2020 13:01:07 +0300 (MSK) References: <20201031162911.61876-1-sergos@tarantool.org> From: Leonid Vasiliev Message-ID: <7e2cf16f-f443-5a4b-d658-8d4e3ecd6f74@tarantool.org> Date: Wed, 4 Nov 2020 13:00:33 +0300 MIME-Version: 1.0 In-Reply-To: <20201031162911.61876-1-sergos@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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, tarantool-patches@dev.tarantool.org Cc: v.shpilevoy@tarantool.org, alexander.turenko@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 > > 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 > > @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 >