Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Ostanevich <sergos@tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in memory profiler
Date: Wed, 30 Dec 2020 12:53:55 +0300	[thread overview]
Message-ID: <A04A23B6-C68B-4A48-9D5F-E43CE5D8245D@tarantool.org> (raw)
In-Reply-To: <20201230093216.GP14702@root>

[-- Attachment #1: Type: text/plain, Size: 4066 bytes --]

LGTM.

Sergos

> On 30 Dec 2020, at 12:32, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> 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
>> <lua_close> in Tarantool.
> 
> Yep, I've found this pretty comment inside <src/main.cc <http://main.cc/>>:
> 
> | #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(-)
>>> 
>> 
>> <snipped>
>> 
>>> 
>>> -- 
>>> 2.28.0
>>> 
>> 
>> -- 
>> Best regards,
>> IM
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 42292 bytes --]

  reply	other threads:[~2020-12-30  9:53 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-29 22:22 [Tarantool-patches] [PATCH luajit 0/3] LuaJIT memory profiler bug fixes Sergey Kaplun
2020-12-29 22:22 ` [Tarantool-patches] [PATCH luajit 1/3] misc: fix build with disabled memory profiler Sergey Kaplun
2020-12-30  8:49   ` Igor Munkin
2020-12-30  8:52     ` Sergey Kaplun
2020-12-30  9:42       ` Sergey Ostanevich
2020-12-29 22:22 ` [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in " Sergey Kaplun
2020-12-30  9:06   ` Igor Munkin
2020-12-30  9:31     ` Sergey Ostanevich
2020-12-30  9:33       ` Sergey Kaplun
2020-12-30  9:32     ` Sergey Kaplun
2020-12-30  9:53       ` Sergey Ostanevich [this message]
2020-12-29 22:22 ` [Tarantool-patches] [PATCH luajit 3/3] core: remove excess assertion inside memprof Sergey Kaplun
2020-12-30  9:39   ` Igor Munkin
2020-12-30  9:50     ` Sergey Kaplun
2020-12-30 10:50       ` Sergey Ostanevich
2020-12-30 11:06         ` Sergey Kaplun
2020-12-30  8:24 ` [Tarantool-patches] [PATCH luajit 0/3] LuaJIT memory profiler bug fixes Alexander V. Tikhonov
2020-12-30 11:20 ` Igor Munkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=A04A23B6-C68B-4A48-9D5F-E43CE5D8245D@tarantool.org \
    --to=sergos@tarantool.org \
    --cc=skaplun@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in memory profiler' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox