From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 998BB4765E0 for ; Tue, 22 Dec 2020 20:51:54 +0300 (MSK) Date: Tue, 22 Dec 2020 20:51:09 +0300 From: Sergey Kaplun Message-ID: <20201222175109.GB9101@root> References: <20201222130858.16812-1-skaplun@tarantool.org> <20201222160548.GW5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201222160548.GW5396@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: Igor Munkin Cc: Michael Filonenko , tarantool-patches@dev.tarantool.org Hi, Igor! Thanks for the review! On 22.12.20, Igor Munkin wrote: > 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 Yes, it's better. Thanks! > > 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. Look nice for me. Applied :) > > > > > 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. > Fixed. > > 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 +++++++++++++++++++ > > +-- Return to second fiber. > > Minor: s/to second/to the second/. Fixed! Thanks! > > > -- > Best regards, > IM I've updated commit message and comments in code considering your and Igor comments. Branch is force-pushed. New commit message is the follows: | lua: avoid panic if HOOK_GC is not an active hook | | Platform panic occurs when fiber.yield() is used within any active | (i.e. being executed now) hook. | | It is a regression caused by 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. | | Relates to #4518 | Closes #5649 | | Reported-by: Michael Filonenko -- Best regards, Sergey Kaplun