Tarantool development patches archive
 help / color / mirror / Atom feed
From: "Alexander V. Tikhonov" <avtikhon@tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] lua: avoid panic if HOOK_GC is not active
Date: Wed, 23 Dec 2020 13:23:42 +0300	[thread overview]
Message-ID: <20201223102342.GA77688@hpalx> (raw)
In-Reply-To: <20201222130858.16812-1-skaplun@tarantool.org>

Hi Sergey, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/233455482

On Tue, Dec 22, 2020 at 04:08:58PM +0300, Sergey Kaplun via Tarantool-patches wrote:
> Panic at fiber.yield() occures inside any active hook.
> 
> This is the regression from 96dbc49d097a96af5273cce2b5663db5917f4ea9
> ('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)) {
>  		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
> 

  parent reply	other threads:[~2020-12-23 10:23 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-22 13:08 Sergey Kaplun
2020-12-22 16:05 ` Igor Munkin
2020-12-22 16:09   ` Igor Munkin
2020-12-22 17:51   ` Sergey Kaplun
2020-12-22 17:55     ` Sergey Kaplun
2020-12-22 16:37 ` Sergey Ostanevich
2020-12-22 17:47   ` Sergey Kaplun
2020-12-22 18:01 ` Sergey Kaplun
2020-12-23  9:47   ` Sergey Ostanevich
2020-12-23 10:23 ` Alexander V. Tikhonov [this message]
2020-12-23 13:05 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201223102342.GA77688@hpalx \
    --to=avtikhon@tarantool.org \
    --cc=skaplun@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] lua: avoid panic if HOOK_GC is not active' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox