Re: [Tarantool-patches] [PATCH luajit 2/3] core: fix resources leak in memory profiler

[Igor Munkin <imun@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
<lua_close> in Tarantool.

> 
>  src/lib_misc.c   | 4 ----
>  src/lj_memprof.c | 8 ++++++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 

<snipped>

>  
> -- 
> 2.28.0
> 

-- 
Best regards,
IM