From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E9B384765E0 for ; Wed, 30 Dec 2020 12:06:36 +0300 (MSK) Date: Wed, 30 Dec 2020 12:06:30 +0300 From: Igor Munkin Message-ID: <20201230090630.GW5396@tarantool.org> References: <061599ac978b162d42ce80ac65106ba67c6d5fcb.1609278043.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <061599ac978b162d42ce80ac65106ba67c6d5fcb.1609278043.git.skaplun@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: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org 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