From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (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 C45EF4765E0 for ; Wed, 30 Dec 2020 12:34:43 +0300 (MSK) Date: Wed, 30 Dec 2020 12:33:56 +0300 From: Sergey Kaplun Message-ID: <20201230093356.GQ14702@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: 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: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Hi! On 30.12.20, Sergey Ostanevich wrote: > Serge, can you point me to a branch please? https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-misc-memprof-fixes > > > On 30 Dec 2020, at 12:06, 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. > > > >> > >> 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