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

[Tarantool-patches] [PATCH luajit] 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 profiler output file and then exits.
Due to early exit, output file handle stay opened. This patch
fixes condition and file handle is closed even a number of samples
is equal to 0.

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

Part of tarantool/tarantool#11055
---
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 | 30 +++++++++++++++++++
 .../script.lua                                | 14 +++++++++
 test/tarantool-tests/utils/tools.lua          |  4 +++
 4 files changed, 49 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..b50b5fce
--- /dev/null
+++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
@@ -0,0 +1,30 @@
+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
+-- close a file handle for profile output file.
+-- See also: https://github.com/luajIT/luajIT/issues/1304
+
+local p_filename = '/tmp/profile'
+
+-- <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)
+
+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..77335a08
--- /dev/null
+++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
@@ -0,0 +1,14 @@
+local p_options = 'ri1'
+local p_filename = assert(arg[1], 'filename argument is missing')
+
+require('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.
+require('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_filename) == 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] Always close profiler output file.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On 10.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

Nit: s/`prof_finish`/`prof_finish()`/
Feel free to ignore.

> `No samples collected` to profiler output file and then exits.

Typo: s/profiler/a profiler/
Typo: s/exits/exited/

> Due to early exit, output file handle stay opened. This patch

Typo: s/output/the output/
Typo: s/opened/open/

> fixes condition and file handle is closed even a number of samples

Typo: s/condition/the condition/
Typo: s/file/the file/
Typo: s/even a number/even if the number/

> is equal to 0.
> 
> Sergey Bronnikov:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#11055
> ---
> 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 | 30 +++++++++++++++++++
>  .../script.lua                                | 14 +++++++++
>  test/tarantool-tests/utils/tools.lua          |  4 +++
>  4 files changed, 49 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..b50b5fce
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
> @@ -0,0 +1,30 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1304-close-profile-dump-with-0-samples'):skipcond({
> +  ['Test requires /proc filesystem'] = jit.os == 'OSX',

I suppose we may get off this skipcond, see the last comment.

> +})
> +local utils = require('utils')
> +
> +test:plan(1)
> +
> +-- Test file to demonstrate LuaJIT incorrect behaviour with missed
> +-- close a file handle for profile output file.

Typo? s/close/closing/
Typo: s/profile/the profile/

> +-- See also: https://github.com/luajIT/luajIT/issues/1304
> +
> +local p_filename = '/tmp/profile'

It's better to use for the profile name generation the following
function:
| local profilename = require("utils").tools.profilename

> +
> +-- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
> +-- with the given environment, launch options and CLI arguments.
> +local script = utils.exec.makecmd(arg)

I don't get it. Why do we need the separate script for it?

> +-- 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)
> +
> +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.

> +
> +-- 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..77335a08
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
> @@ -0,0 +1,14 @@
> +local p_options = 'ri1'

It is better to use a much bigger interval (`i99999` for example) to be
sure that there will be no samples collected. Also, the `r` option isn't
required.

> +local p_filename = assert(arg[1], 'filename argument is missing')
> +
> +require('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.
> +require('jit.p').stop()

I suppose we may require this module only once.

> +
> +-- Make sure LuaJIT profiler is close a file handle.
> +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.

Also, in this case we can run this test on OSX.

> +assert(ls_output:find(p_filename) == 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
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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

Hello, Sergey,

thanks for review! Please see my comments.

Updated version was force-pushed to the branch.

Sergey

On 11.02.2025 17:53, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On 10.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
> Nit: s/`prof_finish`/`prof_finish()`/
> Feel free to ignore.
What for? I'm already written it is a function. Ignored.
>> `No samples collected` to profiler output file and then exits.
> Typo: s/profiler/a profiler/
Fixed.
> Typo: s/exits/exited/
Fixed.
>
>> Due to early exit, output file handle stay opened. This patch
> Typo: s/output/the output/
> Typo: s/opened/open/
Fixed.
>
>> fixes condition and file handle is closed even a number of samples
> Typo: s/condition/the condition/
> Typo: s/file/the file/
> Typo: s/even a number/even if the number/
Fixed.
>
>> is equal to 0.
>>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Part of tarantool/tarantool#11055
>> ---
>> 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 | 30 +++++++++++++++++++
>>   .../script.lua                                | 14 +++++++++
>>   test/tarantool-tests/utils/tools.lua          |  4 +++
>>   4 files changed, 49 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..b50b5fce
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples.test.lua
>> @@ -0,0 +1,30 @@
>> +local tap = require('tap')
>> +local test = tap.test('lj-1304-close-profile-dump-with-0-samples'):skipcond({
>> +  ['Test requires /proc filesystem'] = jit.os == 'OSX',
> I suppose we may get off this skipcond, see the last comment.
>
>> +})
>> +local utils = require('utils')
>> +
>> +test:plan(1)
>> +
>> +-- Test file to demonstrate LuaJIT incorrect behaviour with missed
>> +-- close a file handle for profile output file.
> Typo? s/close/closing/
> Typo: s/profile/the profile/

Fixed:

@@ -7,10 +7,10 @@ local utils = require('utils')
  test:plan(1)

  -- Test file to demonstrate LuaJIT incorrect behaviour with missed
--- close a file handle for profile output file.
+-- closing a file handle for the profile output file.
  -- See also: https://github.com/luajIT/luajIT/issues/1304

-local p_filename = '/tmp/profile'
+local p_filename = utils.tools.profilename

  -- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
  -- with the given environment, launch options and CLI arguments.

>
>> +-- See also:https://github.com/luajIT/luajIT/issues/1304
>> +
>> +local p_filename = '/tmp/profile'
> It's better to use for the profile name generation the following
> function:
> | local profilename = require("utils").tools.profilename

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
@@ -10,7 +10,7 @@ test:plan(1)
  -- close a file handle for profile output file.
  -- See also: https://github.com/luajIT/luajIT/issues/1304

-local p_filename = '/tmp/profile'
+local p_filename = utils.tools.profilename

  -- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
  -- with the given environment, launch options and CLI arguments.
>
>> +
>> +-- <makecmd> runs %testname%/script.lua by <LUAJIT_TEST_BINARY>
>> +-- with the given environment, launch options and CLI arguments.
>> +local script = utils.exec.makecmd(arg)
> 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.

>
>> +-- 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)
>> +
>> +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]".

>
>> +
>> +-- 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..77335a08
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-1304-close-profile-dump-with-0-samples/script.lua
>> @@ -0,0 +1,14 @@
>> +local p_options = 'ri1'
> It is better to use a much bigger interval (`i99999` for example) to be
> sure that there will be no samples collected. Also, the `r` option isn't
> required.

Fixed:

--- 
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
@@ -1,6 +1,8 @@
  local jit_p = require('jit.p')

-local p_options = 'ri1'
+-- 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')

  jit_p.start(p_options, p_filename)

>> +local p_filename = assert(arg[1], 'filename argument is missing')
>> +
>> +require('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.
>> +require('jit.p').stop()
> I suppose we may require this module only once.

Fixed:

--- 
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
@@ -1,13 +1,15 @@
+local jit_p = require('jit.p')
+
  local p_options = 'ri1'
  local p_filename = assert(arg[1], 'filename argument is missing')

-require('jit.p').start(p_options, p_filename)
+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.
-require('jit.p').stop()
+jit_p.stop()

  -- Make sure LuaJIT profiler is close a file handle.
  local ls_output = io.popen('ls -l /proc/$$/fd'):read('a*')

>
>> +
>> +-- Make sure LuaJIT profiler is close a file handle.
>> +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.

>
> Also, in this case we can run this test on OSX.
Testing on MacOS doesn't add more value, because the problem is 
platform-independent.
>
>> +assert(ls_output:find(p_filename) == 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
>>

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