[Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Bronnikov via Tarantool-patches]

From: Mike Pall <mike>

Reported by Guilherme Batalheiro.

(cherry picked from commit fca66335d131669cf017420af6963a7565babb58)

Before the patch, a function `prof_finish` wrote a string
`No samples collected` to a profiler output file and then exited.
Due to early exit, thethe  output file handle stay open. This patch
fixes the condition and the file handle is closed even if the
number of samples is equal to 0.

Sergey Bronnikov:
* added the description and the test for the problem

Part of tarantool/tarantool#11055
---
Changes v2:
- Added fixes according to comments by Sergey Kaplun.

Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-close-file-profiler

Related issues:
- https://github.com/luajIT/luajIT/issues/1304
- https://github.com/tarantool/tarantool/issues/11055

 src/jit/p.lua                                 |  4 +--
 ...close-profile-dump-with-0-samples.test.lua | 31 +++++++++++++++++++
 .../script.lua                                | 19 ++++++++++++
 test/tarantool-tests/utils/tools.lua          |  4 +++
 4 files changed, 55 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
 create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua

diff --git a/src/jit/p.lua b/src/jit/p.lua
index 4569d69e..89b49584 100644
--- a/src/jit/p.lua
+++ b/src/jit/p.lua
@@ -228,9 +228,7 @@ local function prof_finish()
     local samples = prof_samples
     if samples == 0 then
       if prof_raw ~= true then out:write("[No samples collected]\n") end
-      return
-    end
-    if prof_ann then
+    elseif prof_ann then
       prof_annotate(prof_count1, samples)
     else
       prof_top(prof_count1, prof_count2, samples, "")
diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
new file mode 100644
index 00000000..614ecc69
--- /dev/null
+++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
@@ -0,0 +1,31 @@
+local tap = require('tap')
+local test = tap.test('lj-1304-close-profile-dump-with-0-samples'):skipcond({
+  ['Test requires /proc filesystem'] = jit.os == 'OSX',
+})
+local utils = require('utils')
+
+test:plan(1)
+
+-- Test file to demonstrate LuaJIT incorrect behaviour with missed
+-- closing a file handle for the profile output file.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1304
+
+local p_postfix = 'sysprofdata'
+local p_filename = utils.tools.profilename(p_postfix)
+
+-- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
+-- with the given environment, launch options and CLI arguments.
+local script = utils.exec.makecmd(arg)
+-- Execute a Lua script with start and stop LuaJIT profiler,
+-- it is expected no samples found by profiler. The script's
+-- output is suppressed, it is not interested.
+local _ = script(p_filename, p_postfix)
+
+local p_content = io.open(p_filename):read('a*')
+test:is(utils.tools.trim(p_content), '[No samples collected]',
+        'profile dump has no samples')
+
+-- Teardown.
+os.remove(p_filename)
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
new file mode 100644
index 00000000..864958f5
--- /dev/null
+++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
@@ -0,0 +1,19 @@
+local jit_p = require('jit.p')
+
+-- Using a bigger interval to make sure that there will be no
+-- samples collected.
+local p_options = 'i99999'
+local p_filename = assert(arg[1], 'filename argument is missing')
+local p_postfix = assert(arg[2], 'postfix argument is missing')
+
+jit_p.start(p_options, p_filename)
+
+-- No code to generate profiling samples.
+
+-- Stop profiler to execute `jit/p.lua:prof_fmt()`. With zero
+-- samples it triggers early return without closing the file.
+jit_p.stop()
+
+-- Make sure LuaJIT profiler is close a file handle.
+local ls_output = io.popen('ls -l /proc/$$/fd'):read('a*')
+assert(ls_output:find(p_postfix) == nil, 'file is open')
diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
index 33fcae78..9cb65daf 100644
--- a/test/tarantool-tests/utils/tools.lua
+++ b/test/tarantool-tests/utils/tools.lua
@@ -21,4 +21,8 @@ function M.read_file(path)
   return content
 end
 
+function M.trim(str)
+  return (str:gsub('^%s*(.-)%s*$', '%1'))
+end
+
 return M
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes and clarification!
Please consider by answers below.

On 12.02.25, Sergey Bronnikov wrote:
> From: Mike Pall <mike>
> 
> Reported by Guilherme Batalheiro.
> 
> (cherry picked from commit fca66335d131669cf017420af6963a7565babb58)
> 
> Before the patch, a function `prof_finish` wrote a string
> `No samples collected` to a profiler output file and then exited.
> Due to early exit, thethe  output file handle stay open. This patch

Typo: s/thethe  /the /

> fixes the condition and the file handle is closed even if the
> number of samples is equal to 0.
> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#11055
> ---
> Changes v2:
> - Added fixes according to comments by Sergey Kaplun.
> 
> Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-close-file-profiler
> 
> Related issues:
> - https://github.com/luajIT/luajIT/issues/1304
> - https://github.com/tarantool/tarantool/issues/11055
> 
>  src/jit/p.lua                                 |  4 +--
>  ...close-profile-dump-with-0-samples.test.lua | 31 +++++++++++++++++++
>  .../script.lua                                | 19 ++++++++++++
>  test/tarantool-tests/utils/tools.lua          |  4 +++
>  4 files changed, 55 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
>  create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
> 
> diff --git a/src/jit/p.lua b/src/jit/p.lua
> index 4569d69e..89b49584 100644
> --- a/src/jit/p.lua
> +++ b/src/jit/p.lua

<snipped>

> diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
> new file mode 100644
> index 00000000..614ecc69
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
> @@ -0,0 +1,31 @@

<snipped>

> +local script = utils.exec.makecmd(arg)
> +-- Execute a Lua script with start and stop LuaJIT profiler,
> +-- it is expected no samples found by profiler. The script's
> +-- output is suppressed, it is not interested.
> +local _ = script(p_filename, p_postfix)

> # > I don't get it. Why do we need the separate script for it?
> # prof_finish() is executed first time on calling `jit.p.stop()`
> #
> # and executed second time when GC finalizer is calling [1] and therefore
> # message "'[No samples collected]'" appears
> #
> # two times in the output file.
> #
> # 1. https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290
> #
> # We can inline script.lua to a test body and call garbage collector manually,
> # but I propose to leave it as is because  executing as a separate process is
> # more real use case.

Now it is clear to me, thanks!

This is another face of the bug that was fixed by this commit.
Nice catch!

In the original issue, automated parsing shows empty file content.
I suppose it was done with something like the following:

| LUA_PATH="src/?.lua;;" src/luajit -e '
| local jp = require"jit.p"
| local filename = "/tmp/j.p"
|
| collectgarbage("stop")
| jp.start("i9999999", filename)
| jp.stop()
|
| local f = io.open(filename, "r")
| print(("<%s>"):format(f:read("*all")))
| f.close()
| collectgarbage("restart")
| os.remove(filename)
| '

Before the patch, I get the following output:

| <>

After the patch, I get the following output:

| <[No samples collected]
| >

I guess this is the use case for guys from the NeoVim:
They run the profiler inside the application but have no samples
collected. The inspector may read the file from the different process,
but there is no need for it. And, yes, in the test we have to manually
stop and restart the GC, but only for the stability of the reproducer.
It is not a part of a use case itself.

Your test case may be simplified as the following (in terms of my
script):
| LUA_PATH="src/?.lua;;" src/luajit -e '
| local jp = require"jit.p"
| local filename = "/tmp/j.p"
|
| collectgarbage("stop")
| jp.start("i9999999", filename)
| jp.stop()
|
| -- Unload the module and clean the local.
| package.loaded["jit.p"] = nil
| jp = nil
| collectgarbage("collect")
|
| local f = io.open(filename, "r")
| print(("<%s>"):format(f:read("*all")))
| f.close()
| collectgarbage("restart")
| os.remove(filename)
| '
| <[No samples collected]
| [No samples collected]
| >

Since, IMHO, there is no need to check the </proc/$$/fd> content (see
the rationale below), we may use this test case in the original file
without the helper script.

So, let's check both cases (the original one and that found by you) in
the test.

> +
> +local p_content = io.open(p_filename):read('a*')
> +test:is(utils.tools.trim(p_content), '[No samples collected]',
> +        'profile dump has no samples')

> # > Minor: I suppose simple `test:like()` will be enough. In that case there
> # > is no need to use the newly introduced `trim()` function here.
> #
> # The check checks that exactly one "[No samples collected]" is in a file.
> #
> # Before the patch, a file contains two "[No samples collected]".

OK, then let's use the comparison with '[No samples collected]\n', since
we want exact matching. That way we will be sure that there are no
garbage whitespaces, and there is no need to see what exactly (strip
only \n or all whitespaces, etc.) `trim()` helper does.

> +
> +-- Teardown.
> +os.remove(p_filename)
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
> new file mode 100644
> index 00000000..864958f5
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua

<snipped>

> +local ls_output = io.popen('ls -l /proc/$$/fd'):read('a*')

> # > Minor: I am not sure that we really need this check. The descriptor will
> # > be closed when it will be garbage collected, IINM, so there is no leak,
> # > actually. I suggest dropping it and checking only the content of the
> # > file -- this is the main issue regarding `jit.p` solved by that commit.
> #
> # This is for checking that `prof_finish` behaves correctly.
> #
> # fd leak is a resource leak and there is a separate CWE for this -
> #
> # https://cwe.mitre.org/data/definitions/775.html
> #
> # On my machine max number of file descriptors is 126693 per process:
> #
> # [0] ~ $ ulimit -n
> # 126693
> #
> # You will get file descriptor exhaustion if you will run
> #
> # `require('jit.p').start()` 126693 times with different output files.

I still believe that this is not a leak.
Since we have 1 storage for file descriptor (`out` in <jit/p.lua>) all
previous objects stored to this upvalue become dead and will be
collected by the GC.

On my machine, I have only 1024 fds available for the process by
default:
| $ ulimit -n
| 1024

So we can easily check the fds leakage:
| LUA_PATH="src/?.lua;;" src/luajit -e '
| local jp = require"jit.p"
| local filename = "/tmp/j.p"
|
| -- collectgarbage("stop")
| for i = 1, 2000 do
|     jp.start("i9999999", filename)
|     jp.stop()
|     if i % 100 == 0 then print(i) end
| end
|
| os.remove(filename)
| '
| 100
| ...
| 2000

Prints numbers up to 2000 successfully (i.e., there is no leakage of
descriptors).
Plus, we have a bunch of errors like the following:
| ERROR in finalizer: src/jit/p.lua:230: attempt to use a closed file

This happens on `lua_close()` since the udata with the `io` handle is
finalized before the `newproxy()`. Thus, the finalizer for the proxy
can't be used `io` in its finalizer.

Obviously, if we uncomment the `collectgarbage("stop")` line, we get the
corresponding error:

| 1000
| src/luajit: src/jit/p.lua:300: /tmp/j.p: Too many open files

But, anyway, at the `lua_close()` all descriptors will be closed.

The similar behaviour we have for the pure Lua:
| lua -e '
| local filename = "/tmp/j.p"
| collectgarbage("stop")
| for i = 1, 2000 do
|     assert(io.open(filename, "w"))
|     if i % 100 == 0 then print(i) end
| end
|
| os.remove(filename)
| '
| ..
| 1000
| lua: (command line):7: /tmp/j.p: Too many open files

Thus, there is no need for the to check </proc/$$/fd> content. Hence, we
don't need to run this inside the separate script.

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review! Updates applied and force-pushed.

Sergey

On 13.02.2025 18:31, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the fixes and clarification!
> Please consider by answers below.
>
> On 12.02.25, Sergey Bronnikov wrote:
>> From: Mike Pall <mike>
>>
>> Reported by Guilherme Batalheiro.
>>
>> (cherry picked from commit fca66335d131669cf017420af6963a7565babb58)
>>
>> Before the patch, a function `prof_finish` wrote a string
>> `No samples collected` to a profiler output file and then exited.
>> Due to early exit, thethe  output file handle stay open. This patch
> Typo: s/thethe  /the /
Fixed.
>
>> fixes the condition and the file handle is closed even if the
>> number of samples is equal to 0.
>>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#11055
>> ---
>> Changes v2:
>> - Added fixes according to comments by Sergey Kaplun.
>>
>> Branch:https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-close-file-profiler
>>
>> Related issues:
>> -https://github.com/luajIT/luajIT/issues/1304
>> -https://github.com/tarantool/tarantool/issues/11055
>>
>>   src/jit/p.lua                                 |  4 +--
>>   ...close-profile-dump-with-0-samples.test.lua | 31 +++++++++++++++++++
>>   .../script.lua                                | 19 ++++++++++++
>>   test/tarantool-tests/utils/tools.lua          |  4 +++
>>   4 files changed, 55 insertions(+), 3 deletions(-)
>>   create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
>>   create mode 100644 test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
>>
>> diff --git a/src/jit/p.lua b/src/jit/p.lua
>> index 4569d69e..89b49584 100644
>> --- a/src/jit/p.lua
>> +++ b/src/jit/p.lua
> <snipped>
>
>> diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
>> new file mode 100644
>> index 00000000..614ecc69
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
>> @@ -0,0 +1,31 @@
> <snipped>
>
>> +local script = utils.exec.makecmd(arg)
>> +-- Execute a Lua script with start and stop LuaJIT profiler,
>> +-- it is expected no samples found by profiler. The script's
>> +-- output is suppressed, it is not interested.
>> +local _ = script(p_filename, p_postfix)
>> # > I don't get it. Why do we need the separate script for it?
>> # prof_finish() is executed first time on calling `jit.p.stop()`
>> #
>> # and executed second time when GC finalizer is calling [1] and therefore
>> # message "'[No samples collected]'" appears
>> #
>> # two times in the output file.
>> #
>> # 1.https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290
>> #
>> # We can inline script.lua to a test body and call garbage collector manually,
>> # but I propose to leave it as is because  executing as a separate process is
>> # more real use case.
> Now it is clear to me, thanks!
>
> This is another face of the bug that was fixed by this commit.
> Nice catch!
>
> In the original issue, automated parsing shows empty file content.
> I suppose it was done with something like the following:
>
> | LUA_PATH="src/?.lua;;" src/luajit -e '
> | local jp = require"jit.p"
> | local filename = "/tmp/j.p"
> |
> | collectgarbage("stop")
> | jp.start("i9999999", filename)
> | jp.stop()
> |
> | local f = io.open(filename, "r")
> | print(("<%s>"):format(f:read("*all")))
> | f.close()
> | collectgarbage("restart")
> | os.remove(filename)
> | '
>
> Before the patch, I get the following output:
>
> | <>
>
> After the patch, I get the following output:
>
> | <[No samples collected]
> | >
ok, replaced testcase
>
> I guess this is the use case for guys from the NeoVim:
> They run the profiler inside the application but have no samples
> collected. The inspector may read the file from the different process,
> but there is no need for it. And, yes, in the test we have to manually
> stop and restart the GC, but only for the stability of the reproducer.
> It is not a part of a use case itself.
>
> Your test case may be simplified as the following (in terms of my
> script):
> | LUA_PATH="src/?.lua;;" src/luajit -e '
> | local jp = require"jit.p"
> | local filename = "/tmp/j.p"
> |
> | collectgarbage("stop")
> | jp.start("i9999999", filename)
> | jp.stop()
> |
> | -- Unload the module and clean the local.
> | package.loaded["jit.p"] = nil
> | jp = nil
> | collectgarbage("collect")
> |
> | local f = io.open(filename, "r")
> | print(("<%s>"):format(f:read("*all")))
> | f.close()
> | collectgarbage("restart")
> | os.remove(filename)
> | '
> | <[No samples collected]
> | [No samples collected]
> | >
>
> Since, IMHO, there is no need to check the </proc/$$/fd> content (see
> the rationale below), we may use this test case in the original file
> without the helper script.
>
> So, let's check both cases (the original one and that found by you) in
> the test.

What is a rationale for this?

This is a helper script for LuaJIT profiler and according to our 
backporting rules

test must cover a backported patch. This rule is fulfilled by my test.

>
>> +
>> +local p_content = io.open(p_filename):read('a*')
>> +test:is(utils.tools.trim(p_content), '[No samples collected]',
>> +        'profile dump has no samples')
>> # > Minor: I suppose simple `test:like()` will be enough. In that case there
>> # > is no need to use the newly introduced `trim()` function here.
>> #
>> # The check checks that exactly one "[No samples collected]" is in a file.
>> #
>> # Before the patch, a file contains two "[No samples collected]".
> OK, then let's use the comparison with '[No samples collected]\n', since
> we want exact matching. That way we will be sure that there are no
> garbage whitespaces, and there is no need to see what exactly (strip
> only \n or all whitespaces, etc.) `trim()` helper does.

It looks reasonable, fixed:

--- 
a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
+++ 
b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
@@ -22,8 +22,7 @@ local script = utils.exec.makecmd(arg)
  local _ = script(p_filename, p_postfix)

  local p_content = io.open(p_filename):read('a*')
-test:is(utils.tools.trim(p_content), '[No samples collected]',
-        'profile dump has no samples')
+test:is(p_content, '[No samples collected]\n', 'profile dump has no 
samples')

  -- Teardown.
  os.remove(p_filename)

>
>> +
>> +-- Teardown.
>> +os.remove(p_filename)
>> +
>> +test:done(true)
>> diff --git a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
>> new file mode 100644
>> index 00000000..864958f5
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
> <snipped>
>
>> +local ls_output = io.popen('ls -l /proc/$$/fd'):read('a*')
>> # > Minor: I am not sure that we really need this check. The descriptor will
>> # > be closed when it will be garbage collected, IINM, so there is no leak,
>> # > actually. I suggest dropping it and checking only the content of the
>> # > file -- this is the main issue regarding `jit.p` solved by that commit.
>> #
>> # This is for checking that `prof_finish` behaves correctly.
>> #
>> # fd leak is a resource leak and there is a separate CWE for this -
>> #
>> #https://cwe.mitre.org/data/definitions/775.html
>> #
>> # On my machine max number of file descriptors is 126693 per process:
>> #
>> # [0] ~ $ ulimit -n
>> # 126693
>> #
>> # You will get file descriptor exhaustion if you will run
>> #
>> # `require('jit.p').start()` 126693 times with different output files.
> I still believe that this is not a leak.
> Since we have 1 storage for file descriptor (`out` in <jit/p.lua>) all
> previous objects stored to this upvalue become dead and will be
> collected by the GC.
Right, I've missed this.
>
> On my machine, I have only 1024 fds available for the process by
> default:
> | $ ulimit -n
> | 1024
>
> So we can easily check the fds leakage:
> | LUA_PATH="src/?.lua;;" src/luajit -e '
> | local jp = require"jit.p"
> | local filename = "/tmp/j.p"
> |
> | -- collectgarbage("stop")
> | for i = 1, 2000 do
> |     jp.start("i9999999", filename)
> |     jp.stop()
> |     if i % 100 == 0 then print(i) end
> | end
> |
> | os.remove(filename)
> | '
> | 100
> | ...
> | 2000
>
> Prints numbers up to 2000 successfully (i.e., there is no leakage of
> descriptors).
> Plus, we have a bunch of errors like the following:
> | ERROR in finalizer: src/jit/p.lua:230: attempt to use a closed file
>
> This happens on `lua_close()` since the udata with the `io` handle is
> finalized before the `newproxy()`. Thus, the finalizer for the proxy
> can't be used `io` in its finalizer.
>
> Obviously, if we uncomment the `collectgarbage("stop")` line, we get the
> corresponding error:
>
> | 1000
> | src/luajit: src/jit/p.lua:300: /tmp/j.p: Too many open files
>
> But, anyway, at the `lua_close()` all descriptors will be closed.
>
> The similar behaviour we have for the pure Lua:
> | lua -e '
> | local filename = "/tmp/j.p"
> | collectgarbage("stop")
> | for i = 1, 2000 do
> |     assert(io.open(filename, "w"))
> |     if i % 100 == 0 then print(i) end
> | end
> |
> | os.remove(filename)
> | '
> | ..
> | 1000
> | lua: (command line):7: /tmp/j.p: Too many open files
>
> Thus, there is no need for the to check </proc/$$/fd> content. Hence, we
> don't need to run this inside the separate script.
>
> <snipped>
>
>> -- 
>> 2.34.1
>>

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

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey,
Thanks for the fixes!
See my answers below.

On 25.02.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review! Updates applied and force-pushed.
> 
> Sergey
> 
> On 13.02.2025 18:31, Sergey Kaplun via Tarantool-patches wrote:
> > Hi, Sergey!
> > Thanks for the fixes and clarification!
> > Please consider by answers below.
> >
> > On 12.02.25, Sergey Bronnikov wrote:

<snipped>

> >
> >> +local script = utils.exec.makecmd(arg)
> >> +-- Execute a Lua script with start and stop LuaJIT profiler,
> >> +-- it is expected no samples found by profiler. The script's
> >> +-- output is suppressed, it is not interested.
> >> +local _ = script(p_filename, p_postfix)
> >> # > I don't get it. Why do we need the separate script for it?
> >> # prof_finish() is executed first time on calling `jit.p.stop()`
> >> #
> >> # and executed second time when GC finalizer is calling [1] and therefore
> >> # message "'[No samples collected]'" appears
> >> #
> >> # two times in the output file.
> >> #
> >> # 1.https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290
> >> #
> >> # We can inline script.lua to a test body and call garbage collector manually,
> >> # but I propose to leave it as is because  executing as a separate process is
> >> # more real use case.
> > Now it is clear to me, thanks!
> >
> > This is another face of the bug that was fixed by this commit.
> > Nice catch!
> >
> > In the original issue, automated parsing shows empty file content.
> > I suppose it was done with something like the following:
> >
> > | LUA_PATH="src/?.lua;;" src/luajit -e '
> > | local jp = require"jit.p"
> > | local filename = "/tmp/j.p"
> > |
> > | collectgarbage("stop")
> > | jp.start("i9999999", filename)
> > | jp.stop()
> > |
> > | local f = io.open(filename, "r")
> > | print(("<%s>"):format(f:read("*all")))
> > | f.close()
> > | collectgarbage("restart")
> > | os.remove(filename)
> > | '
> >
> > Before the patch, I get the following output:
> >
> > | <>
> >
> > After the patch, I get the following output:
> >
> > | <[No samples collected]
> > | >
> ok, replaced testcase

> +-- See also: https://github.com/LuaJIT/LuaJIT/issues/1304

Nit: Missed dot at the end of the sentence.

<snipped>

> +
> + jit_p.start('i9999999', filename)

Please add the comment before the call that we set the interval of the
profiling to a huge enough value to be sure that there are no samples
collected.

> + jit_p.stop()
> +
> + local f = io.open(filename, 'r')
> + local p_content = io.open(filename):read('a*')
> + test:is(p_content, '[No samples collected]\n', 'profile dump has no samples')
> + f.close()

Typo: s/f./f:/

> +
> + -- Teardown.
> + collectgarbage('restart')
> + os.remove(filename)

> >
> > I guess this is the use case for guys from the NeoVim:
> > They run the profiler inside the application but have no samples
> > collected. The inspector may read the file from the different process,
> > but there is no need for it. And, yes, in the test we have to manually
> > stop and restart the GC, but only for the stability of the reproducer.
> > It is not a part of a use case itself.
> >
> > Your test case may be simplified as the following (in terms of my
> > script):
> > | LUA_PATH="src/?.lua;;" src/luajit -e '
> > | local jp = require"jit.p"
> > | local filename = "/tmp/j.p"
> > |
> > | collectgarbage("stop")
> > | jp.start("i9999999", filename)
> > | jp.stop()
> > |
> > | -- Unload the module and clean the local.
> > | package.loaded["jit.p"] = nil
> > | jp = nil
> > | collectgarbage("collect")
> > |
> > | local f = io.open(filename, "r")
> > | print(("<%s>"):format(f:read("*all")))
> > | f.close()
> > | collectgarbage("restart")
> > | os.remove(filename)
> > | '
> > | <[No samples collected]
> > | [No samples collected]
> > | >
> >
> > Since, IMHO, there is no need to check the </proc/$$/fd> content (see
> > the rationale below), we may use this test case in the original file
> > without the helper script.
> >
> > So, let's check both cases (the original one and that found by you) in
> > the test.
> 
> What is a rationale for this?

As I said before, this is an additional possible demonstration of the
bug. I see nothing bad to add it as well, since it simulates a little
bit different workload -- flushing the output before we finish the
process or if we unload the module. I am not insisting on it, though.

> 
> This is a helper script for LuaJIT profiler and according to our 
> backporting rules
> 
> test must cover a backported patch. This rule is fulfilled by my test.

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,

thanks for review!

Changes applied and force-pushed.

Sergey

On 26.02.2025 14:58, Sergey Kaplun wrote:
> Hi, Sergey,
> Thanks for the fixes!
> See my answers below.
>
> On 25.02.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for review! Updates applied and force-pushed.
>>
>> Sergey
>>
>> On 13.02.2025 18:31, Sergey Kaplun via Tarantool-patches wrote:
>>> Hi, Sergey!
>>> Thanks for the fixes and clarification!
>>> Please consider by answers below.
>>>
>>> On 12.02.25, Sergey Bronnikov wrote:
> <snipped>
>
>>>> +local script = utils.exec.makecmd(arg)
>>>> +-- Execute a Lua script with start and stop LuaJIT profiler,
>>>> +-- it is expected no samples found by profiler. The script's
>>>> +-- output is suppressed, it is not interested.
>>>> +local _ = script(p_filename, p_postfix)
>>>> # > I don't get it. Why do we need the separate script for it?
>>>> # prof_finish() is executed first time on calling `jit.p.stop()`
>>>> #
>>>> # and executed second time when GC finalizer is calling [1] and therefore
>>>> # message "'[No samples collected]'" appears
>>>> #
>>>> # two times in the output file.
>>>> #
>>>> # 1.https://github.com/tarantool/luajit/blob/80360fb2e570ce8d54ff59ccf0bcd5dfb6b98527/src/jit/p.lua#L290
>>>> #
>>>> # We can inline script.lua to a test body and call garbage collector manually,
>>>> # but I propose to leave it as is because  executing as a separate process is
>>>> # more real use case.
>>> Now it is clear to me, thanks!
>>>
>>> This is another face of the bug that was fixed by this commit.
>>> Nice catch!
>>>
>>> In the original issue, automated parsing shows empty file content.
>>> I suppose it was done with something like the following:
>>>
>>> | LUA_PATH="src/?.lua;;" src/luajit -e '
>>> | local jp = require"jit.p"
>>> | local filename = "/tmp/j.p"
>>> |
>>> | collectgarbage("stop")
>>> | jp.start("i9999999", filename)
>>> | jp.stop()
>>> |
>>> | local f = io.open(filename, "r")
>>> | print(("<%s>"):format(f:read("*all")))
>>> | f.close()
>>> | collectgarbage("restart")
>>> | os.remove(filename)
>>> | '
>>>
>>> Before the patch, I get the following output:
>>>
>>> | <>
>>>
>>> After the patch, I get the following output:
>>>
>>> | <[No samples collected]
>>> | >
>> ok, replaced testcase
>> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1304
> Nit: Missed dot at the end of the sentence.

Fixed, thanks!

(BTW currently 150 tests with links finished with dot, and 35 are not.)

> <snipped>
>
>> +
>> + jit_p.start('i9999999', filename)
> Please add the comment before the call that we set the interval of the
> profiling to a huge enough value to be sure that there are no samples
> collected.
Added.
>
>> + jit_p.stop()
>> +
>> + local f = io.open(filename, 'r')
>> + local p_content = io.open(filename):read('a*')
>> +test:is(p_content, '[No samples collected]\n', 'profile dump has no samples')
>> + f.close()
> Typo: s/f./f:/
>
Fixed.
>> +
>> + -- Teardown.
>> + collectgarbage('restart')
>> + os.remove(filename)
>>> I guess this is the use case for guys from the NeoVim:
>>> They run the profiler inside the application but have no samples
>>> collected. The inspector may read the file from the different process,
>>> but there is no need for it. And, yes, in the test we have to manually
>>> stop and restart the GC, but only for the stability of the reproducer.
>>> It is not a part of a use case itself.
>>>
>>> Your test case may be simplified as the following (in terms of my
>>> script):
>>> | LUA_PATH="src/?.lua;;" src/luajit -e '
>>> | local jp = require"jit.p"
>>> | local filename = "/tmp/j.p"
>>> |
>>> | collectgarbage("stop")
>>> | jp.start("i9999999", filename)
>>> | jp.stop()
>>> |
>>> | -- Unload the module and clean the local.
>>> | package.loaded["jit.p"] = nil
>>> | jp = nil
>>> | collectgarbage("collect")
>>> |
>>> | local f = io.open(filename, "r")
>>> | print(("<%s>"):format(f:read("*all")))
>>> | f.close()
>>> | collectgarbage("restart")
>>> | os.remove(filename)
>>> | '
>>> | <[No samples collected]
>>> | [No samples collected]
>>> | >
>>>
>>> Since, IMHO, there is no need to check the </proc/$$/fd> content (see
>>> the rationale below), we may use this test case in the original file
>>> without the helper script.
>>>
>>> So, let's check both cases (the original one and that found by you) in
>>> the test.
>> What is a rationale for this?
> As I said before, this is an additional possible demonstration of the
> bug. I see nothing bad to add it as well, since it simulates a little
> bit different workload -- flushing the output before we finish the
> process or if we unload the module. I am not insisting on it, though.

How often users unload profiler's module? For me, this case looks artificial

and the final goal for us is not a demonstration, but covering a code 
touched by the patch,

it is done by proposed test.

(I'm really confused that on review I learn more and more new 
requirements to the patches

like adding all available testcases that cover a patch or that code 
should follow C89 or

that using etc.)

The bug fixed by the patch is quite minor, it will not break production 
and will not kill humans etc.

I believe a single testcase and time that we both spend on doing 
backport, test and two iterations

of review is more than enough for this patch. Moreover, jit.p is not 
used by Tarantool users,

we provide our own profilers to them.


>> This is a helper script for LuaJIT profiler and according to our
>> backporting rules
>>
>> test must cover a backported patch. This rule is fulfilled by my test.
> <snipped>
>

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

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes, LGTM.

On 27.02.25, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> thanks for review!
> 
> Changes applied and force-pushed.
> 
> Sergey
> 
> On 26.02.2025 14:58, Sergey Kaplun wrote:

<snipped>

> >> What is a rationale for this?
> > As I said before, this is an additional possible demonstration of the
> > bug. I see nothing bad to add it as well, since it simulates a little
> > bit different workload -- flushing the output before we finish the
> > process or if we unload the module. I am not insisting on it, though.
> 
> How often users unload profiler's module? For me, this case looks artificial

The following module [1] is used to reload all Lua modules (including
`jit.p`). It was used for any Lua package update or code modification.

[1]: https://github.com/moonlibs/package-reload

Also, it is not only about unloading the module but also about finishing
the process too (as you first discovered). This looks like a real use
case. This variant of the test is just simpler than creating a child
process and checking its result files.

> and the final goal for us is not a demonstration, but covering a code 
> touched by the patch,
> 
> it is done by proposed test.
> 
> (I'm really confused that on review I learn more and more new 
> requirements to the patches

It is not a requirement, just a suggestion. See the last line of my
comment. I'm OK with reproducing the issue only since it is not the
vital part of the code.

> 
> like adding all available testcases that cover a patch or that code 
> should follow C89 or
> 
> that using etc.)
> 
> The bug fixed by the patch is quite minor, it will not break production 
> and will not kill humans etc.
> 
> I believe a single testcase and time that we both spend on doing 
> backport, test and two iterations
> 
> of review is more than enough for this patch. Moreover, jit.p is not 
> used by Tarantool users,
> 
> we provide our own profilers to them.

So, just ignore it then.

> 
> 
> >> This is a helper script for LuaJIT profiler and according to our
> >> backporting rules
> >>
> >> test must cover a backported patch. This rule is fulfilled by my test.
> > <snipped>
> >

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey!

The second testcase has been added and

changes force-pushed to the branch.

Sergey

On 04.03.2025 18:40, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes, LGTM.
>
> On 27.02.25, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for review!
>>
>> Changes applied and force-pushed.
>>
>> Sergey
>>
>> On 26.02.2025 14:58, Sergey Kaplun wrote:
> <snipped>
>
>>>> What is a rationale for this?
>>> As I said before, this is an additional possible demonstration of the
>>> bug. I see nothing bad to add it as well, since it simulates a little
>>> bit different workload -- flushing the output before we finish the
>>> process or if we unload the module. I am not insisting on it, though.
>> How often users unload profiler's module? For me, this case looks artificial
> The following module [1] is used to reload all Lua modules (including
> `jit.p`). It was used for any Lua package update or code modification.
>
> [1]:https://github.com/moonlibs/package-reload
>
> Also, it is not only about unloading the module but also about finishing
> the process too (as you first discovered). This looks like a real use
> case. This variant of the test is just simpler than creating a child
> process and checking its result files.
>
>> and the final goal for us is not a demonstration, but covering a code
>> touched by the patch,
>>
>> it is done by proposed test.
>>
>> (I'm really confused that on review I learn more and more new
>> requirements to the patches
> It is not a requirement, just a suggestion. See the last line of my
> comment. I'm OK with reproducing the issue only since it is not the
> vital part of the code.
>
>> like adding all available testcases that cover a patch or that code
>> should follow C89 or
>>
>> that using etc.)
>>
>> The bug fixed by the patch is quite minor, it will not break production
>> and will not kill humans etc.
>>
>> I believe a single testcase and time that we both spend on doing
>> backport, test and two iterations
>>
>> of review is more than enough for this patch. Moreover, jit.p is not
>> used by Tarantool users,
>>
>> we provide our own profilers to them.
> So, just ignore it then.
>
>>
>>>> This is a helper script for LuaJIT profiler and according to our
>>>> backporting rules
>>>>
>>>> test must cover a backported patch. This rule is fulfilled by my test.
>>> <snipped>
>>>

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

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for adding the testcases!
LGTM with a last comment below.

> +
> +local function close_profile_dump_with_0_samples_with_unload(subtest)
> +  local jit_p = require('jit.p')
> +
> +  subtest:plan(1)
> +
> +  collectgarbage('stop')
> +  jit_p.start(jit_p_options, filename)
> +  jit_p.stop()
> +
> +  -- Unload the module and clean the local.
> +  package.loaded['jit.p'] = nil
> +  jit_p = nil -- luacheck: no unused
> +  collectgarbage('collect')
> +
> +  local f = io.open(filename, 'r')
> +  local p_content = f:read('a*')
> +  subtest:is(p_content, '[No samples collected]\n',
> +             'profile dump has no samples')
> +  f.close()

Typo: s/f./f:/

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Bronnikov via Tarantool-patches]

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


On 05.03.2025 17:58, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for adding the testcases!
> LGTM with a last comment below.
>
>> +
>> +local function close_profile_dump_with_0_samples_with_unload(subtest)
>> +  local jit_p = require('jit.p')
>> +
>> +subtest:plan(1)
>> +
>> +  collectgarbage('stop')
>> +  jit_p.start(jit_p_options, filename)
>> +  jit_p.stop()
>> +
>> +  -- Unload the module and clean the local.
>> +  package.loaded['jit.p'] = nil
>> +  jit_p = nil -- luacheck: no unused
>> +  collectgarbage('collect')
>> +
>> +  local f = io.open(filename, 'r')
>> +  local p_content =f:read('a*')
>> +subtest:is(p_content, '[No samples collected]\n',
>> +             'profile dump has no samples')
>> +  f.close()
> Typo: s/f./f:/

Thanks!


--- 
a/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
+++ 
b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
@@ -51,7 +51,7 @@ local function 
close_profile_dump_with_0_samples_with_unload(subtest)
    local p_content = f:read('a*')
subtest:is(p_content, '[No samples collected]\n',
               'profile dump has no samples')
-  f.close()
+ f:close()

    -- Teardown.
    collectgarbage('restart')

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

Re: [Tarantool-patches] [PATCH luajit][v2] Always close profiler output file.

[Sergey Kaplun via Tarantool-patches]

Sergey,

I've applied the patch into all long-term branches in tarantool/luajit
and bumped a new version in master [1], release/3.3 [2], release/3.2 [3]
and release/2.11 [4].

[1]: https://github.com/tarantool/tarantool/pull/11235
[2]: https://github.com/tarantool/tarantool/pull/11236
[3]: https://github.com/tarantool/tarantool/pull/11237
[4]: https://github.com/tarantool/tarantool/pull/11238


-- 
Best regards,
Sergey Kaplun