From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 C9A4D469710 for ; Thu, 26 Nov 2020 00:33:02 +0300 (MSK) References: <20201031162911.61876-1-sergos@tarantool.org> From: Vladislav Shpilevoy Message-ID: <12e5f150-8e08-1004-ad8a-c6bd1a04fd5f@tarantool.org> Date: Wed, 25 Nov 2020 22:32:59 +0100 MIME-Version: 1.0 In-Reply-To: <20201031162911.61876-1-sergos@tarantool.org> Content-Type: text/plain; charset=utf-8 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: alexander.turenko@tarantool.org Hi! Thanks for the fixes! Technically the patch is good! See 3 non-technical comments below. > diff --git a/src/box/relay.cc b/src/box/relay.cc > index b68b45e00..a7bc2c6f7 100644 > --- a/src/box/relay.cc > +++ b/src/box/relay.cc > @@ -780,6 +771,16 @@ relay_subscribe_f(va_list ap) > cbus_endpoint_destroy(&relay->endpoint, cbus_process); > > relay_exit(relay); > + > + /* > + * Log the error that caused the relay to break the loop. > + * Don't clear the error for status reporting. > + */ > + assert(!diag_is_empty(&relay->diag)); > + diag_set_error(diag_get(), diag_last_error(&relay->diag)); > + diag_log(); > + say_crit("exiting the relay loop"); > + 1. I suggest you to move this to a separate commit, before you change fiber cond behaviour. Because it seems it is not related to conds really. And even if it would be related, still the issue looks like a separate bug. > return -1; > } > 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..ca4ca2c90 > --- /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, 'expected result is false') > +test:ok(tostring(result.err) == 'fiber is cancelled', 'fiber cancellation should be reported') 2. I think you are also supposed to call os.exit with test:check() like other tap tests do. Otherwise it probably always ends with 0 code, and won't work properly when we will make tap tests non-diff based. > diff --git a/test/box/gh-4834-netbox-fiber-cancel.result b/test/box/gh-4834-netbox-fiber-cancel.result > new file mode 100644 > index 000000000..cb631995c > --- /dev/null > +++ b/test/box/gh-4834-netbox-fiber-cancel.result > @@ -0,0 +1,62 @@ > +-- 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 3. I noticed you never finish the fiber. So it hangs there forever or until a restart. Better not leave any test artifacts and finish it completely. Just save the channel to a global variable and put something into it in the end.