From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 4C61F4765E0 for ; Wed, 30 Dec 2020 12:33:03 +0300 (MSK) Date: Wed, 30 Dec 2020 12:32:16 +0300 From: Sergey Kaplun Message-ID: <20201230093216.GP14702@root> References: <061599ac978b162d42ce80ac65106ba67c6d5fcb.1609278043.git.skaplun@tarantool.org> <20201230090630.GW5396@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201230090630.GW5396@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in memory profiler List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Igor, Thanks for the review! I've updated commit message to the following as you purposed: | core: fix resources leak in memory profiler | | When memory profiler fails to start with PROFILE_ERRRUN status both | stream and ctx are not released. At the same time when memory profiler | fails to start with the PROFILE_ERRIO status both stream and ctx are | released twice. Both cases occur due to invalid return status checking. | | To fix the leakage on_stop callback is called manually inside | the profiler when error on start occurs. Checks in | misc.memprof.start() are omitted. | | Follows up tarantool/tarantool#5442 Branch is force-pushed. On 30.12.20, Igor Munkin wrote: > Sergey, > > Thanks for the patch! LGTM except the wording in commit message > (consider the comments below). > > Side note: I want to notice that with on_start callback things would be > clearer. Let's return to this again later. > > On 30.12.20, Sergey Kaplun wrote: > > When the profiler is failing to start with error different from > > Typo: s/is failing/fails/. > Typo: s/with error/with the error/. > > > PROFILE_ERRIO neither a file stream is closed nor ctx is freed > > in case of incorrect return status checking. > > Strictly saying there are two problems: > * Possible leakage for PROFILE_ERRRUN > * Double free for PROFILE_ERRIO > > So I propose the following wording: > > | When memory profiler fails to start with PROFILE_ERRRUN status both > | stream and ctx are not released. At the same time when memory profiler > | fails to start with the PROFILE_ERRIO status both stream and ctx are > | released twice. Both cases occur due to invalid return status checking. > > > > > To avoid this behaviour on_stop callback is called manually inside > > Minor: s/To avoid this behaviour/To fix the leakage/. > > > the profiler when error on start is occurring. Checks in > > Typo: s/is occuring/occurs/. > > > misc.memprof.start() are omitted. > > > > Follows up tarantool/tarantool#5442 > > --- > > > > * How patch was checked: > > Before patch you can occur the error like: > > | $ src/luajit -e ' > > | local f, msg, errno = misc.memprof.start("/tmp/tmp_memprofile.bin") > > | misc.memprof.start("/tmp/tmp_memprofile.bin") print(f,msg,errno) > > | ' > > | true nil nil > > | luajit: lj_state.c:178: close_state: Assertion `g->gc.total == sizeof(GG_State)' failed. > > This patch fixes it. > > > > * Why this assertion is not failed in tests (we have the test with same > > functionality)? > > This assertion failed inside close_state. Tarantool in some reason > > doesn't call lua_close on stop. It's weird to me. I'll try to find an > > explanation and will create a ticket. > > > > * Why I don't create a test case. > > The best idea is to do something like this and waiting for OOM: > > | for _ = 1, 10000 do > > | misc.memprof.start("/tmp/tmp_memprofile.bin") > > | end > > But it's disgusting, so as I've discussed with Igor offline test case > > will be ommited. > > Side note: The test aims to hit LUA_ERRMEM and need to be run for a long > time. The exact reason we didn't face this failure is the omitted > in Tarantool. Yep, I've found this pretty comment inside : | #if 0 | /* | * This doesn't work reliably since things | * are too interconnected. | */ | tarantool_lua_free(); | session_free(); | user_cache_free(); | fiber_free(); | memory_free(); | random_free(); | #endif > > > > > src/lib_misc.c | 4 ---- > > src/lj_memprof.c | 8 ++++++-- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > > > > > > -- > > 2.28.0 > > > > -- > Best regards, > IM -- Best regards, Sergey Kaplun