From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 09B71469719 for ; Sun, 4 Oct 2020 22:01:40 +0300 (MSK) Date: Sun, 4 Oct 2020 21:51:05 +0300 From: Igor Munkin Message-ID: <20201004185105.GK18920@tarantool.org> References: <61072ba98da2e4098b0c35cad83ffcdcbc537c8e.1601644669.git.imun@tarantool.org> <97a28e52-3d64-b0a8-7dfb-d8d0fd30c409@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <97a28e52-3d64-b0a8-7dfb-d8d0fd30c409@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] lua: prohibit fiber yield when GC hook is active List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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 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 > > --- > > > > 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 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 -- Best regards, IM