From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 C753B4765E0 for ; Tue, 22 Dec 2020 19:05:54 +0300 (MSK) Date: Tue, 22 Dec 2020 19:05:48 +0300 From: Igor Munkin Message-ID: <20201222160548.GW5396@tarantool.org> References: <20201222130858.16812-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 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: Michael Filonenko , tarantool-patches@dev.tarantool.org Sergey, Thanks for the patch! LGTM except a couple of nits below. Minor: Strictly saying there is no panic if GC hook is not active (literally any Lua code except the finalizers), but panic occurs when the active hook is not HOOK_GC. So I propose to tune the commit subject the following way: | lua: avoid panic if HOOK_GC is not an active hook On 22.12.20, Sergey Kaplun wrote: > Panic at fiber.yield() occures inside any active hook. Minor: I rearrange this line a bit, feel free to ignore. | Platform panic occurs when fiber.yield() is used within any active | (i.e. being executed now) 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 Minor: I guess this patch doesn't follow #4727 but rather #4518 (considering the commit message for the patch you mentioned above). So I propose to use either "Relates to #4518" or "Follows up #4518" instead. I personally prefer the first one, since it occured #4518 relates to totally another issue. > 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/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. Minor: s/to second/to the second/. > +fiber.yield() > +test:ok(true) > + > +os.exit(test:check() and 0 or 1) > -- > 2.28.0 > -- Best regards, IM