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 >> >> 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 > > >> 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  -- runs %testname%/script.lua by  -- 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  -- runs %testname%/script.lua by  -- with the given environment, launch options and CLI arguments. > >> + >> +-- runs %testname%/script.lua by >> +-- 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 >>