From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp42.i.mail.ru (smtp42.i.mail.ru [94.100.177.102]) (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 540C04765E0 for ; Wed, 23 Dec 2020 13:23:45 +0300 (MSK) Date: Wed, 23 Dec 2020 13:23:42 +0300 From: "Alexander V. Tikhonov" Message-ID: <20201223102342.GA77688@hpalx> References: <20201222130858.16812-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201222130858.16812-1-skaplun@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] lua: avoid panic if HOOK_GC is not active List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Kaplun Cc: tarantool-patches@dev.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 > 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 >