From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp30.i.mail.ru (smtp30.i.mail.ru [94.100.177.90]) (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 3E415469719 for ; Thu, 5 Nov 2020 23:42:29 +0300 (MSK) Date: Thu, 5 Nov 2020 23:42:27 +0300 From: Sergey Ostanevich Message-ID: <20201105204227.GE517@tarantool.org> References: <20201031162911.61876-1-sergos@tarantool.org> <7e2cf16f-f443-5a4b-d658-8d4e3ecd6f74@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <7e2cf16f-f443-5a4b-d658-8d4e3ecd6f74@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: Leonid Vasiliev Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, alexander.turenko@tarantool.org Hi! On 04 ноя 13:00, Leonid Vasiliev wrote: > 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. That's a very good point, Leonid! I didn't met any use of the '0' or '1' result in C or in Lua for the case of fiber cancellation. Although it might be not true about many projects in Tarantool ecosystem and - what's more scary - in user's code in the multitude of installations. Also, I have to admit that introduction of two types of behavior for different versions of Tarantool will get things even worse. I think we have to re-iterate the discussion of the change in the ticket first. Sergos > > 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 > >