Re: [Tarantool-patches] [PATCH 1/2] fiber: make wakeup in Lua and C nop on self

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

24.04.2021 02:15, Vladislav Shpilevoy пишет:
> fiber.wakeup() in Lua and fiber_wakeup() in C could lead to a
> crash or undefined behaviour when called on the currently running
> fiber.
>
> In particular, if after wakeup was started a new fiber in a
> blocking way (fiber.create() and fiber_start()) it would crash in
> debug build, and lead to unknown results in release.
>
> If after wakeup was made a sleep with non-zero timeout or an
> infinite yield (fiber_yield()), the fiber woke up in the same
> event loop iteration regardless of any timeout or other wakeups.
> It was a spurious wakeup, which is not expected in most of the
> places internally.
>
> The patch makes the wakeup nop on the current fiber making it safe
> to use anywhere.
>
> Closes #5292
> Closes #6043
>
> @TarantoolBot document
> Title: fiber.wakeup() in Lua and fiber_wakeup() in C are nop on self
> In Lua `fiber.wakeup()` being called on the current fiber does not
> do anything, safe to use. The same for `fiber_wakeup()` in C.
> ---

Thanks for the patch!
LGTM, generally, with one test-related comment.


...

> diff --git a/test/app-tap/gh-6043-fiber-wakeup-self.test.lua b/test/app-tap/gh-6043-fiber-wakeup-self.test.lua
> new file mode 100755
> index 000000000..082c1228d
> --- /dev/null
> +++ b/test/app-tap/gh-6043-fiber-wakeup-self.test.lua
> @@ -0,0 +1,50 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local fiber = require('fiber')
> +
> +--
> +-- gh-6043: fiber.wakeup() on self could lead to a crash if followed by
> +-- fiber.create(). Also it could lead to spurious wakeup if followed by a sleep
> +-- with non-zero timeout, or by, for example, an infinite yield such as WAL
> +-- write.
> +--
> +
> +local function test_wakeup_self_and_call(test)
> +    test:plan(3)
> +
> +    local self = fiber.self()
> +    fiber.wakeup(self)
> +    local count = 0
> +    local f = fiber.create(function()
> +        count = count + 1
> +        fiber.yield()
> +        count = count + 1
> +    end)
> +    test:is(count, 1, 'created a fiber')
> +    fiber.yield()
> +    test:is(count, 2, 'it yielded')
> +    test:is(f:status(), 'dead', 'and ended')
> +end
> +
> +local function test_wakeup_self_and_wal_write(test)
> +    test:plan(1)
> +
> +    local s = box.schema.create_space('test')
> +    s:create_index('pk')
> +
> +    fiber.wakeup(fiber.self())
> +    local lsn = box.info.lsn
> +    s:replace{1}
> +    test:is(box.info.lsn, lsn + 1, 'written to WAL')
> +    s:drop()
> +end

Again, shouldn't everything related to space creation, WAL writes and so on
reside in box-tap ? I may be wrong, just asking.

> +
> +box.cfg{}
> +
> +local test = tap.test('gh-6043-fiber-wakeup-self')
> +test:plan(2)
> +test:test('wakeup self + call', test_wakeup_self_and_call)
> +test:test('wakeup self + wal write', test_wakeup_self_and_wal_write)
> +
> +os.exit(test:check() and 0 or 1)
> diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
> index 9c1a23bdd..a1e3ded87 100644
> --- a/test/unit/fiber.cc
> +++ b/test/unit/fiber.cc
> @@ -138,8 +138,7 @@ fiber_join_test()
>   	fiber_set_joinable(fiber, true);
>   	fiber_wakeup(fiber);
>   	/** Let the fiber schedule */
> -	fiber_wakeup(fiber());
> -	fiber_yield();
> +	fiber_reschedule();
>   	note("by this time the fiber should be dead already");
>   	fiber_cancel(fiber);
>   	fiber_join(fiber);
> @@ -201,12 +200,42 @@ fiber_name_test()
>   	footer();
>   }
>   
> +static void
> +fiber_wakeup_self_test()
> +{
> +	header();
> +
> +	struct fiber *f = fiber();
> +
> +	fiber_wakeup(f);
> +	double duration = 0.001;
> +	uint64_t t1 = fiber_clock64();
> +	fiber_sleep(duration);
> +	uint64_t t2 = fiber_clock64();
> +	/*
> +	 * It was a real sleep, not 0 duration. Wakeup is nop on the running
> +	 * fiber.
> +	 */
> +	assert(t2 - t1 >= duration);
> +
> +	/*
> +	 * Wakeup + start of a new fiber. This is different from yield but
> +	 * works without crashes too.
> +	 */
> +	struct fiber *newf = fiber_new_xc("nop", noop_f);
> +	fiber_wakeup(f);
> +	fiber_start(newf);
> +
> +	footer();
> +}
> +
>   static int
>   main_f(va_list ap)
>   {
>   	fiber_name_test();
>   	fiber_join_test();
>   	fiber_stack_test();
> +	fiber_wakeup_self_test();
>   	ev_break(loop(), EVBREAK_ALL);
>   	return 0;
>   }
> diff --git a/test/unit/fiber.result b/test/unit/fiber.result
> index a61e0a2b8..36bf2a599 100644
> --- a/test/unit/fiber.result
> +++ b/test/unit/fiber.result
> @@ -17,3 +17,5 @@ SystemError Failed to allocate 42 bytes in allocator for exception: Cannot alloc
>   # normal-stack fiber not crashed
>   # big-stack fiber not crashed
>   	*** fiber_stack_test: done ***
> +	*** fiber_wakeup_self_test ***
> +	*** fiber_wakeup_self_test: done ***
> diff --git a/test/unit/fiber_stress.cc b/test/unit/fiber_stress.cc
> index 44ab12487..aa892d0a0 100644
> --- a/test/unit/fiber_stress.cc
> +++ b/test/unit/fiber_stress.cc
> @@ -9,10 +9,8 @@ enum {
>   static int
>   yield_f(va_list ap)
>   {
> -	for (int i = 0; i < ITERATIONS; i++) {
> -		fiber_wakeup(fiber());
> -		fiber_yield();
> -	}
> +	for (int i = 0; i < ITERATIONS; i++)
> +		fiber_reschedule();
>   	return 0;
>   }
>   

-- 
Serge Petrenko