Re: [Tarantool-patches] [PATCH v2] core: handle fiber cancellation for fiber.cond

[Vladislav Shpilevoy <v.shpilevoy@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.