From: Sergey Kaplun <skaplun@tarantool.org> To: Igor Munkin <imun@tarantool.org> Cc: Michael Filonenko <filonenko.mikhail@gmail.com>, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] lua: avoid panic if HOOK_GC is not active Date: Tue, 22 Dec 2020 20:51:09 +0300 [thread overview] Message-ID: <20201222175109.GB9101@root> (raw) In-Reply-To: <20201222160548.GW5396@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 <filonenko.mikhail@gmail.com> > > 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 +++++++++++++++++++ <snipped> > > +-- Return to second fiber. > > Minor: s/to second/to the second/. Fixed! Thanks! > <snipped> > > -- > 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 <filonenko.mikhail@gmail.com> -- Best regards, Sergey Kaplun
next prev parent reply other threads:[~2020-12-22 17:51 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 [this message] 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 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=20201222175109.GB9101@root \ --to=skaplun@tarantool.org \ --cc=filonenko.mikhail@gmail.com \ --cc=imun@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