Re: [Tarantool-patches] [PATCH] lua: prohibit fiber yield when GC hook is active

[Igor Munkin <imun@tarantool.org>]

Vlad,

Thanks for your review!

On 04.10.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 02.10.2020 16:39, Igor Munkin wrote:
> > While running GC hook (i.e. __gc  metamethod) garbage collector engine
> > is "stopped": the memory penalty threshold is set to LJ_MAX_MEM and
> > incremental GC step is not triggered as a result. Ergo, yielding the
> > execution at the finalizer body leads to further running platform with
> > disabled LuaJIT GC. It is not re-enabled until the yielded fiber doesn't
> > get the execution back.
> > 
> > This changeset extends <cord_on_yield> routine with the check whether GC
> > hook is active. If the switch-over occurs in scope of __gc metamethod
> > the platform is forced to stop its execution with EXIT_FAILURE and calls
> > panic routine before the exit.
> > 
> > Relates to #4518
> > Follows up #4727
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > Vlad introduced the internal interface and local internal background
> > fiber in scope of 8443bd9 ("fiber: introduce schedule_task() internal
> > function") to postpone any yielding finalization (e.g. 3d5b4da ("fio:
> > close unused descriptors automatically") and f073834 ("swim: use
> > fiber._internal.schedule_task() for GC")). After this patch is merged we
> > need to update docs and provide users a correct scenario to detect and
> > fix yielding finalizers.
> 
> What is the scenario to fix? If you propose to expose schedule_task(),
> I would better avoid it. We need a properly designed pool of Lua fibers
> with stable API to be able to expose it. Until we did it, users should
> do their own 'schedule_task()' or a similar alternative.

Well, I don't quite get your point regarding fiber *pool* API, but I
agree that users should not use internal API and totally OK with the
idea to provide a recipe with ad-hoc <schedule_task> implementation. I
believe we should at least share with users the right (even "official"
or "approved" if you wish) way to handle the issue.

> 
> The patch LGTM.

Added your tag:
| Reviewed-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

-- 
Best regards,
IM