Re: [Tarantool-patches] [PATCH] lua: avoid panic if HOOK_GC is not active

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the patch!

Sergos

> On 22 Dec 2020, at 16:08, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Panic at fiber.yield() occures inside any active hook.
> 
> This is the regression from 96dbc49d097a96af5273cce2b5663db5917f4ea9
  ^^^^^^^^^^ It is a     ^^^ caused by

> ('lua: prohibit fiber yield when GC hook is active').
> 
> This patch fixes false positive panic in cases when VM is not running
> a GC hook.
> 
> Reported-by: Michael Filonenko <filonenko.mikhail@gmail.com>
> Follows up #4727
> Closes #5649
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-5649-fix-for-debug-hook
> Issue: https://github.com/tarantool/tarantool/issues/5649
> CI: https://gitlab.com/tarantool/tarantool/-/pipelines/233249505
> 
> src/lua/utils.c                               |  2 +-
> .../gh-5649-yield-in-debug-hook.test.lua      | 25 +++++++++++++++++++
> 2 files changed, 26 insertions(+), 1 deletion(-)
> create mode 100755 test/app-tap/gh-5649-yield-in-debug-hook.test.lua
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 23fbdd4ad..b5a6ca5b7 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -1375,7 +1375,7 @@ void cord_on_yield(void)
> 	 * earlier. As a result fiber switch is prohibited when
> 	 * GC hook is active and the platform is forced to stop.
> 	 */
> -	if (unlikely(g->hookmask & (HOOK_ACTIVE|HOOK_GC))) {
> +	if (unlikely(g->hookmask & HOOK_GC)) {

It can happens the GC hook is not active? Should we have a second test for HOOK_ACTIVE?

> 		struct lua_State *L = fiber()->storage.lua.stack;
> 		assert(L != NULL);
> 		lua_pushfstring(L, "fiber %d is switched while running GC"
> diff --git a/test/app-tap/gh-5649-yield-in-debug-hook.test.lua b/test/app-tap/gh-5649-yield-in-debug-hook.test.lua
> new file mode 100755
> index 000000000..9c1cca51f
> --- /dev/null
> +++ b/test/app-tap/gh-5649-yield-in-debug-hook.test.lua
> @@ -0,0 +1,25 @@
> +#!/usr/bin/env tarantool
> +
> +local fiber = require('fiber')
> +local tap = require('tap')
> +local test = tap.test('yield-in-debug-hook')
> +
> +test:plan(2)
> +
> +-- Test that HOOK_ACTIVE is not enough to panic and
> +-- fiber still can use general hooks at switches.
> +fiber.create(function()
> +  local old_hook, mask, count = debug.gethook()
> +  debug.sethook(function()
> +    fiber.yield()
> +  end, 'c')
> +  -- All ok if panic doesn't occure.
> +  -- Yield before hook is set back.
> +  debug.sethook(old_hook, mask, count)
> +  test:ok(true)
> +end)
> +-- Return to second fiber.
> +fiber.yield()
> +test:ok(true)
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.28.0
>