* [Tarantool-patches] [PATCH luajit] Improve CLI signal handling on POSIX. @ 2026-10-21 17:42 Evgeniy Temirgaleev via Tarantool-patches 2026-10-21 23:23 ` [Tarantool-patches] [PATCH luajit v2] " Evgeniy Temirgaleev via Tarantool-patches 2026-10-26 12:59 ` [Tarantool-patches] [PATCH luajit] " Sergey Bronnikov via Tarantool-patches 0 siblings, 2 replies; 4+ messages in thread From: Evgeniy Temirgaleev via Tarantool-patches @ 2026-10-21 17:42 UTC (permalink / raw) To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches From: Mike Pall <mike> (cherry picked from commit 423ac2144b0447c925596f4d93e073ce501ed4e1) An obsolete signal() function in glibc implemented as a wrapper for POSIX sigaction() call with BSD semantic. In particular, it provides an SA_RESTART flag for SIGINT handler [1-2]. The last makes certain system calls, e.g. read() or wait(), restartable across signals [3]. The patch switches from signal() to sigaction() for POSIX systems. 1. https://man7.org/linux/man-pages/man2/signal.2.html 2. https://ftp.gnu.org/gnu/glibc/glibc-2.44.tar.xz (sysdeps/posix/signal.c, line 44) 3. https://man7.org/linux/man-pages/man2/sigaction.2.html Temir Galeev: * added the description and the test for the problem Part of tarantool/tarantool#13442 --- Branch: https://github.com/tarantool/luajit/tree/tmr_g/lj-noticket-improve-cli-sig-handling-2 src/luajit.c | 23 +++++-- ...ket-fix-cli-posix-sigint-handling.test.lua | 17 +++++ .../script.lua | 25 ++++++++ .../script.sh | 28 +++++++++ test/tarantool-tests/utils/exec.lua | 63 +++++++++++++++---- 5 files changed, 141 insertions(+), 15 deletions(-) create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh diff --git a/src/luajit.c b/src/luajit.c index d9d85361..d35bea54 100644 --- a/src/luajit.c +++ b/src/luajit.c @@ -35,6 +35,21 @@ #if !LJ_TARGET_CONSOLE #include <signal.h> + +#if LJ_TARGET_POSIX +/* Improve signal handling on POSIX. Try CTRL-C on: luajit -e 'io.read()' */ +static void signal_set(int sig, void (*h)(int)) +{ + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = h; + sigemptyset(&sa.sa_mask); + sigaction(sig, &sa, NULL); +} +#else +#define signal_set signal +#endif + #endif static lua_State *globalL = NULL; @@ -54,8 +69,8 @@ static void lstop(lua_State *L, lua_Debug *ar) static void laction(int i) { - signal(i, SIG_DFL); /* if another SIGINT happens before lstop, - terminate process (default action) */ + /* Terminate process if another SIGINT happens (double CTRL-C). */ + signal_set(i, SIG_DFL); lua_sethook(globalL, lstop, LUA_MASKCALL | LUA_MASKRET | LUA_MASKCOUNT, 1); } #endif @@ -129,11 +144,11 @@ static int docall(lua_State *L, int narg, int clear) lua_pushcfunction(L, traceback); /* push traceback function */ lua_insert(L, base); /* put it under chunk and args */ #if !LJ_TARGET_CONSOLE - signal(SIGINT, laction); + signal_set(SIGINT, laction); #endif status = lua_pcall(L, narg, (clear ? 0 : LUA_MULTRET), base); #if !LJ_TARGET_CONSOLE - signal(SIGINT, SIG_DFL); + signal_set(SIGINT, SIG_DFL); #endif lua_remove(L, base); /* remove traceback function */ /* force a complete garbage collection in case of errors */ diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua new file mode 100644 index 00000000..4ea316d8 --- /dev/null +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua @@ -0,0 +1,17 @@ +local tap = require('tap') +local test = tap.test('lj-noticket-fix-cli-posix-sigint-handling'):skipcond({ + ['Check LuaJIT CLI signal handling'] = _TARANTOOL, + ['Disabled on POSIX nonconformant OS'] = jit.os ~= 'BSD' and + jit.os ~= 'Linux' and + jit.os ~= 'OSX' and + jit.os ~= 'POSIX', +}) + +test:plan(1) +local utils = require('utils') +local script = utils.exec.makecmd_bash(arg, {t = test}) +local output = script( + utils.exec.luabin(arg), + utils.exec.test_data(arg, 'script.lua')) +test:is(output, 'OK') +test:done(true) diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua new file mode 100644 index 00000000..0e6f4078 --- /dev/null +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua @@ -0,0 +1,25 @@ +local ffi = require("ffi") + +ffi.cdef[[ + int fork(void); + unsigned int sleep(unsigned int seconds); + int waitpid(int pid, int *status, int options); +]] + +local pid = ffi.C.fork() + +if pid == 0 then + -- child. + ffi.C.sleep(10) +elseif pid > 0 then + -- parent. + local status = ffi.new("int[1]") + -- wait() system call: it restarts after signal is handled + -- if SA_RESTART flag is set for the handler in sigaction() + -- or if old signal() is used and it's OS specific implementation + -- is BSD-aware. + ffi.C.waitpid(pid, status, 0) +else + -- fork() failed. + os.exit(1) +end diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh new file mode 100644 index 00000000..7fa3c019 --- /dev/null +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh @@ -0,0 +1,28 @@ +LUA_BIN=$1 +SCRIPT_TO_RUN=$2 +$LUA_BIN $SCRIPT_TO_RUN 2>/dev/null & +pid=$! + +# Wait for proccess is started and running. +sleep 1.5 +if ! kill -0 "$pid" 2>/dev/null; then + echo 'ERR' + exit 1 +fi + +# Send SIGINT and wait for termination. +kill -INT "$pid" 2>/dev/null +sleep 1.5 + +# Terminated? OK, signal handled -- 'sigaction() behavior'. +if ! kill -0 "$pid" 2>/dev/null; then + wait "$pid" 2>/dev/null + echo 'OK' + exit 0 +fi + +# Not terminated -- 'signal() BSD-behavior'. +kill -KILL "$pid" 2>/dev/null +wait "$pid" 2>/dev/null +echo 'FAIL' +exit 0 diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua index 49f8ef06..f19405a2 100644 --- a/test/tarantool-tests/utils/exec.lua +++ b/test/tarantool-tests/utils/exec.lua @@ -43,33 +43,74 @@ local function makeenv(tabenv) return table.concat(flatenv, ' ') end --- <makecmd> creates a command that runs %testname%/script.lua by --- <LUAJIT_TEST_BINARY> with the given environment, launch options --- and CLI arguments. The function yields an object (i.e. table) --- with the aforementioned parameters. To launch the command just --- call the object. -function M.makecmd(arg, opts) +-- <makecmd_exec> creates a command that runs given binary with the +-- given environment, launch options and zero or more permanent +-- arguments. +-- The function yields an object (i.e. table) with the aforementioned +-- parameters. To launch the command just call the object. Additional +-- arguments which may vary from call to call are the `__call` args. +-- Pass tap.test() object as opt.t for diagnostic. +function M.makecmd_exec(bin, opts) + assert(type(bin) == 'string') + assert(opts == nil or type(opts) == 'table') + local perm_args = opts and opts.args or {} + assert(type(perm_args) == 'table') return setmetatable({ - LUABIN = M.luacmd(arg), - SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'), + BIN = bin, ENV = opts and makeenv(opts.env) or '', REDIRECT = opts and opts.redirect or '', + PERMARGS = (' %s'):rep(#perm_args):format(unpack(perm_args)), + t = opts and opts.t, }, { __call = function(self, ...) -- This line just makes the command for <io.popen> by the -- following steps: -- 1. Replace the placeholders with the corresponding values - -- given to the command constructor (e.g. script, env). + -- given to the command constructor (e.g. bin, env). -- 2. Join all CLI arguments given to the __call metamethod. -- 3. Concatenate the results of step 1 and step 2 to obtain -- the resulting command. - local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self) + local cmd = ('<ENV> <BIN> <REDIRECT> <PERMARGS>'):gsub('%<(%w+)>', self) .. (' %s'):rep(select('#', ...)):format(...) + if self.t ~= nil then self.t:diag('cmd: '..cmd) end -- Trim both leading and trailing whitespace from the output -- produced by the child process. - return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '') + local output = io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '') + if self.t ~= nil then self.t:diag('output: '..output) end + return output end }) end +-- Makes a file name for running test's data file in '%testname%' directory. +function M.test_data(arg, name) + return arg[0]:gsub('%.test%.lua$', '/'..name) +end + +-- <makecmd_bash> creates a command that runs %testname%/script.sh +-- with the given environment, launch options and CLI arguments. +function M.makecmd_bash(arg, opts) + local script = opts and opts.script or M.test_data(arg, 'script.sh') + return M.makecmd_exec('/bin/bash', { + env = opts and opts.env or nil, + redirect = opts and opts.redirect or nil, + args = {script}, + t = opts and opts.t or nil, + }) +end + +-- <makecmd> creates a command that runs %testname%/script.lua by +-- <LUAJIT_TEST_BINARY> with the given environment, launch options +-- and CLI arguments. +function M.makecmd(arg, opts) + local luabin = M.luacmd(arg) + local script = opts and opts.script or M.test_data(arg, 'script.lua') + return M.makecmd_exec(luabin, { + env = opts and opts.env or nil, + redirect = opts and opts.redirect or nil, + args = {script}, + t = opts and opts.t or nil, + }) +end + return M -- 2.49.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Tarantool-patches] [PATCH luajit v2] Improve CLI signal handling on POSIX. 2026-10-21 17:42 [Tarantool-patches] [PATCH luajit] Improve CLI signal handling on POSIX Evgeniy Temirgaleev via Tarantool-patches @ 2026-10-21 23:23 ` Evgeniy Temirgaleev via Tarantool-patches 2026-10-26 12:59 ` [Tarantool-patches] [PATCH luajit] " Sergey Bronnikov via Tarantool-patches 1 sibling, 0 replies; 4+ messages in thread From: Evgeniy Temirgaleev via Tarantool-patches @ 2026-10-21 23:23 UTC (permalink / raw) To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches Hi, all. Sorry, I missed some lint fixes. Fixes is force pushed to the branch: - lint; - Also tuned wait time in 'script.sh' from 1.5s to 3s. It seems, the valgrind/sanitaizers tests flaks. Please, see v2: --- From: Mike Pall <mike> (cherry picked from commit 423ac2144b0447c925596f4d93e073ce501ed4e1) An obsolete signal() function in glibc implemented as a wrapper for POSIX sigaction() call with BSD semantic. In particular, it provides an SA_RESTART flag for SIGINT handler [1-2]. The last makes certain system calls, e.g. read() or wait(), restartable across signals [3]. The patch switches from signal() to sigaction() for POSIX systems. 1. https://man7.org/linux/man-pages/man2/signal.2.html 2. https://ftp.gnu.org/gnu/glibc/glibc-2.44.tar.xz (sysdeps/posix/signal.c, line 44) 3. https://man7.org/linux/man-pages/man2/sigaction.2.html Temir Galeev: * added the description and the test for the problem Part of tarantool/tarantool#13442 --- Branch: https://github.com/tarantool/luajit/tree/tmr_g/lj-noticket-improve-cli-sig-handling-2 src/luajit.c | 23 +++++-- ...ket-fix-cli-posix-sigint-handling.test.lua | 17 +++++ .../script.lua | 25 +++++++ .../script.sh | 28 ++++++++ test/tarantool-tests/utils/exec.lua | 66 +++++++++++++++---- 5 files changed, 144 insertions(+), 15 deletions(-) create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh diff --git a/src/luajit.c b/src/luajit.c index d9d85361..d35bea54 100644 --- a/src/luajit.c +++ b/src/luajit.c @@ -35,6 +35,21 @@ #if !LJ_TARGET_CONSOLE #include <signal.h> + +#if LJ_TARGET_POSIX +/* Improve signal handling on POSIX. Try CTRL-C on: luajit -e 'io.read()' */ +static void signal_set(int sig, void (*h)(int)) +{ + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = h; + sigemptyset(&sa.sa_mask); + sigaction(sig, &sa, NULL); +} +#else +#define signal_set signal +#endif + #endif static lua_State *globalL = NULL; @@ -54,8 +69,8 @@ static void lstop(lua_State *L, lua_Debug *ar) static void laction(int i) { - signal(i, SIG_DFL); /* if another SIGINT happens before lstop, - terminate process (default action) */ + /* Terminate process if another SIGINT happens (double CTRL-C). */ + signal_set(i, SIG_DFL); lua_sethook(globalL, lstop, LUA_MASKCALL | LUA_MASKRET | LUA_MASKCOUNT, 1); } #endif @@ -129,11 +144,11 @@ static int docall(lua_State *L, int narg, int clear) lua_pushcfunction(L, traceback); /* push traceback function */ lua_insert(L, base); /* put it under chunk and args */ #if !LJ_TARGET_CONSOLE - signal(SIGINT, laction); + signal_set(SIGINT, laction); #endif status = lua_pcall(L, narg, (clear ? 0 : LUA_MULTRET), base); #if !LJ_TARGET_CONSOLE - signal(SIGINT, SIG_DFL); + signal_set(SIGINT, SIG_DFL); #endif lua_remove(L, base); /* remove traceback function */ /* force a complete garbage collection in case of errors */ diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua new file mode 100644 index 00000000..4ea316d8 --- /dev/null +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua @@ -0,0 +1,17 @@ +local tap = require('tap') +local test = tap.test('lj-noticket-fix-cli-posix-sigint-handling'):skipcond({ + ['Check LuaJIT CLI signal handling'] = _TARANTOOL, + ['Disabled on POSIX nonconformant OS'] = jit.os ~= 'BSD' and + jit.os ~= 'Linux' and + jit.os ~= 'OSX' and + jit.os ~= 'POSIX', +}) + +test:plan(1) +local utils = require('utils') +local script = utils.exec.makecmd_bash(arg, {t = test}) +local output = script( + utils.exec.luabin(arg), + utils.exec.test_data(arg, 'script.lua')) +test:is(output, 'OK') +test:done(true) diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua new file mode 100644 index 00000000..c8e6b70b --- /dev/null +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua @@ -0,0 +1,25 @@ +local ffi = require("ffi") + +ffi.cdef[[ + int fork(void); + unsigned int sleep(unsigned int seconds); + int waitpid(int pid, int *status, int options); +]] + +local pid = ffi.C.fork() + +if pid == 0 then + -- child. + ffi.C.sleep(10) +elseif pid > 0 then + -- parent. + local status = ffi.new("int[1]") + -- wait() system call: it restarts after signal is handled + -- if SA_RESTART flag is set for the handler in sigaction() + -- or if old signal() is used and it's OS specific + -- implementation is BSD-aware. + ffi.C.waitpid(pid, status, 0) +else + -- fork() failed. + os.exit(1) +end diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh new file mode 100644 index 00000000..17d8241a --- /dev/null +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh @@ -0,0 +1,28 @@ +LUA_BIN=$1 +SCRIPT_TO_RUN=$2 +$LUA_BIN $SCRIPT_TO_RUN 2>/dev/null & +pid=$! + +# Wait for process is started and running. +sleep 3 +if ! kill -0 "$pid" 2>/dev/null; then + echo 'ERR' + exit 1 +fi + +# Send SIGINT and wait for termination. +kill -INT "$pid" 2>/dev/null +sleep 3 + +# Terminated? OK, signal handled -- 'sigaction() behavior'. +if ! kill -0 "$pid" 2>/dev/null; then + wait "$pid" 2>/dev/null + echo 'OK' + exit 0 +fi + +# Not terminated -- 'signal() BSD-behavior'. +kill -KILL "$pid" 2>/dev/null +wait "$pid" 2>/dev/null +echo 'FAIL' +exit 0 diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua index 49f8ef06..80ca87fc 100644 --- a/test/tarantool-tests/utils/exec.lua +++ b/test/tarantool-tests/utils/exec.lua @@ -43,33 +43,77 @@ local function makeenv(tabenv) return table.concat(flatenv, ' ') end --- <makecmd> creates a command that runs %testname%/script.lua by --- <LUAJIT_TEST_BINARY> with the given environment, launch options --- and CLI arguments. The function yields an object (i.e. table) --- with the aforementioned parameters. To launch the command just --- call the object. -function M.makecmd(arg, opts) +-- <makecmd_exec> creates a command that runs given binary +-- with the given environment, launch options and zero or +-- more permanent arguments. +-- The function yields an object (i.e. table) with the +-- aforementioned parameters. To launch the command just call +-- the object. Additional arguments which may vary from call +-- to call are the `__call` args. +-- Pass tap.test() object as opt.t for diagnostic. +function M.makecmd_exec(bin, opts) + assert(type(bin) == 'string') + assert(opts == nil or type(opts) == 'table') + local perm_args = opts and opts.args or {} + assert(type(perm_args) == 'table') return setmetatable({ - LUABIN = M.luacmd(arg), - SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'), + BIN = bin, ENV = opts and makeenv(opts.env) or '', REDIRECT = opts and opts.redirect or '', + PERMARGS = (' %s'):rep(#perm_args):format(unpack(perm_args)), + t = opts and opts.t, }, { __call = function(self, ...) -- This line just makes the command for <io.popen> by the -- following steps: -- 1. Replace the placeholders with the corresponding values - -- given to the command constructor (e.g. script, env). + -- given to the command constructor (e.g. bin, env). -- 2. Join all CLI arguments given to the __call metamethod. -- 3. Concatenate the results of step 1 and step 2 to obtain -- the resulting command. - local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self) + local cmd = ('<ENV> <BIN> <REDIRECT> <PERMARGS>'):gsub('%<(%w+)>', self) .. (' %s'):rep(select('#', ...)):format(...) + if self.t ~= nil then self.t:diag('cmd: '..cmd) end -- Trim both leading and trailing whitespace from the output -- produced by the child process. - return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '') + local output = io.popen(cmd): + read('*all'):gsub('^%s+', ''):gsub('%s+$', '') + if self.t ~= nil then self.t:diag('output: '..output) end + return output end }) end +-- Makes a file name for running test's data file in +-- '%testname%' directory. +function M.test_data(arg, name) + return arg[0]:gsub('%.test%.lua$', '/'..name) +end + +-- <makecmd_bash> creates a command that runs %testname%/script.sh +-- with the given environment, launch options and CLI arguments. +function M.makecmd_bash(arg, opts) + local script = opts and opts.script or M.test_data(arg, 'script.sh') + return M.makecmd_exec('/bin/bash', { + env = opts and opts.env or nil, + redirect = opts and opts.redirect or nil, + args = {script}, + t = opts and opts.t or nil, + }) +end + +-- <makecmd> creates a command that runs %testname%/script.lua by +-- <LUAJIT_TEST_BINARY> with the given environment, launch options +-- and CLI arguments. +function M.makecmd(arg, opts) + local luabin = M.luacmd(arg) + local script = opts and opts.script or M.test_data(arg, 'script.lua') + return M.makecmd_exec(luabin, { + env = opts and opts.env or nil, + redirect = opts and opts.redirect or nil, + args = {script}, + t = opts and opts.t or nil, + }) +end + return M -- 2.49.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Improve CLI signal handling on POSIX. 2026-10-21 17:42 [Tarantool-patches] [PATCH luajit] Improve CLI signal handling on POSIX Evgeniy Temirgaleev via Tarantool-patches 2026-10-21 23:23 ` [Tarantool-patches] [PATCH luajit v2] " Evgeniy Temirgaleev via Tarantool-patches @ 2026-10-26 12:59 ` Sergey Bronnikov via Tarantool-patches 2026-11-02 0:02 ` Evgeniy Temirgaleev via Tarantool-patches 1 sibling, 1 reply; 4+ messages in thread From: Sergey Bronnikov via Tarantool-patches @ 2026-10-26 12:59 UTC (permalink / raw) To: Evgeniy Temirgaleev, Sergey Kaplun; +Cc: tarantool-patches [-- Attachment #1: Type: text/plain, Size: 15580 bytes --] Hi, Evgeniy, thanks for the patch! See my comments below. Sergey On 10/21/26 20:42, Evgeniy Temirgaleev wrote: > From: Mike Pall <mike> > > (cherry picked from commit 423ac2144b0447c925596f4d93e073ce501ed4e1) > > An obsolete signal() function in glibc implemented as a wrapper for s/implemented/is implemented/ > POSIX sigaction() call with BSD semantic. In particular, it provides > an SA_RESTART flag for SIGINT handler [1-2]. The last makes certain s/last/latter/ > system calls, e.g. read() or wait(), restartable across signals [3]. > > The patch switches from signal() to sigaction() for POSIX systems. > > 1.https://man7.org/linux/man-pages/man2/signal.2.html > 2.https://ftp.gnu.org/gnu/glibc/glibc-2.44.tar.xz (sysdeps/posix/signal.c, line 44) isn't better to replace it with a link to a file in repository? https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/signal.c;h=7a80338fd6270b2b0e713c32c98c66fb284bd196;hb=HEAD > 3.https://man7.org/linux/man-pages/man2/sigaction.2.html > > Temir Galeev: > * added the description and the test for the problem Please consider replacing the shell scripts and helpers with an implementation that uses FFI C and libc functions. The draft of the script.lua is below: local ffi = require('ffi') ffi.cdef[[ int pipe(int *fds); int getpid(void); long read(int fd, void *buf, size_t count); unsigned int alarm(unsigned int seconds); ]] local fds = ffi.new('int[2]') assert(ffi.C.pipe(fds) == 0) -- Fail instead of hanging forever if SIGINT does not break the call. ffi.C.alarm(5) -- The CLI installs the SIGINT handler before the script starts, so this -- line is the readiness marker for the test. io.write(ffi.C.getpid(), '\n') io.flush() -- Block in read(2) until SIGINT. Without SA_RESTART it returns EINTR and -- the CLI runs the Lua hook, which raises "interrupted!". ffi.C.read(fds[0], ffi.new('char[1]'), 1) and test.lua: test:plan(1) local ffi = require('ffi') ffi.cdef[[ int kill(int pid, int sig); ]] local SIGINT = 2 local utils = require('utils') local script = arg[0]:gsub('%.test%.lua$', '/script.lua') local cmd = ('exec %s %s 2>&1'):format(utils.exec.luacmd(arg), script) local proc = io.popen(cmd) -- The child prints its PID after the CLI has installed the SIGINT -- handler, so it is a readiness marker. Skip possible noise lines. local pid for _ = 1, 10 do local line = proc:read('*l') if line == nil then break end pid = line:match('^%d+$') and tonumber(line) if pid ~= nil then break end end assert(pid ~= nil, 'the child process must report its PID') ffi.C.kill(pid, SIGINT) local output = proc:read('*a') proc:close() test:like(output, 'interrupted!', 'SIGINT interrupts the blocking call') test:done(true) > > Part of tarantool/tarantool#13442 > --- > > Branch:https://github.com/tarantool/luajit/tree/tmr_g/lj-noticket-improve-cli-sig-handling-2 > > src/luajit.c | 23 +++++-- > ...ket-fix-cli-posix-sigint-handling.test.lua | 17 +++++ > .../script.lua | 25 ++++++++ > .../script.sh | 28 +++++++++ > test/tarantool-tests/utils/exec.lua | 63 +++++++++++++++---- > 5 files changed, 141 insertions(+), 15 deletions(-) > create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua > create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua > create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh The commit mixes three things: the C change, its test, and a generic refactoring of `utils/exec.lua` (new `makecmd_exec`/`makecmd_bash`/`test_data`, with `makecmd` rewritten on top of them). I would split the existing patch to two patches. > > diff --git a/src/luajit.c b/src/luajit.c > index d9d85361..d35bea54 100644 > --- a/src/luajit.c > +++ b/src/luajit.c > @@ -35,6 +35,21 @@ > > #if !LJ_TARGET_CONSOLE > #include <signal.h> > + > +#if LJ_TARGET_POSIX > +/* Improve signal handling on POSIX. Try CTRL-C on: luajit -e 'io.read()' */ > +static void signal_set(int sig, void (*h)(int)) > +{ > + struct sigaction sa; > + memset(&sa, 0, sizeof(sa)); `signal_set()` drops `SA_RESTART` only implicitly: `memset(&sa, 0, sizeof(sa))` zeroes `sa_flags` and nothing is assigned to the field afterwards. That is exactly what the patch needs, but a reader (or a future "sync with glibc" change) may see the zero flags as an omission and restore `SA_RESTART`, silently reverting the fix. It might be worth explicitly noting this in the commit message. > + sa.sa_handler = h; > + sigemptyset(&sa.sa_mask); > + sigaction(sig, &sa, NULL); > +} > +#else > +#define signal_set signal > +#endif > + > #endif > > static lua_State *globalL = NULL; > @@ -54,8 +69,8 @@ static void lstop(lua_State *L, lua_Debug *ar) > > static void laction(int i) > { > - signal(i, SIG_DFL); /* if another SIGINT happens before lstop, > - terminate process (default action) */ > + /* Terminate process if another SIGINT happens (double CTRL-C). */ > + signal_set(i, SIG_DFL); > lua_sethook(globalL, lstop, LUA_MASKCALL | LUA_MASKRET | LUA_MASKCOUNT, 1); > } > #endif > @@ -129,11 +144,11 @@ static int docall(lua_State *L, int narg, int clear) > lua_pushcfunction(L, traceback); /* push traceback function */ > lua_insert(L, base); /* put it under chunk and args */ > #if !LJ_TARGET_CONSOLE > - signal(SIGINT, laction); > + signal_set(SIGINT, laction); > #endif > status = lua_pcall(L, narg, (clear ? 0 : LUA_MULTRET), base); > #if !LJ_TARGET_CONSOLE > - signal(SIGINT, SIG_DFL); > + signal_set(SIGINT, SIG_DFL); > #endif > lua_remove(L, base); /* remove traceback function */ > /* force a complete garbage collection in case of errors */ > diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua > new file mode 100644 > index 00000000..4ea316d8 > --- /dev/null > +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua > @@ -0,0 +1,17 @@ > +local tap = require('tap') > +local test = tap.test('lj-noticket-fix-cli-posix-sigint-handling'):skipcond({ > + ['Check LuaJIT CLI signal handling'] = _TARANTOOL, The skip reason `['Check LuaJIT CLI signal handling'] = _TARANTOOL` describes what the test checks, not why it is skipped under Tarantool. > + ['Disabled on POSIX nonconformant OS'] = jit.os ~= 'BSD' and > + jit.os ~= 'Linux' and > + jit.os ~= 'OSX' and > + jit.os ~= 'POSIX', the condition can be inverted and contain only two items: if jit.os == 'Windows' or jit.os == 'Other' Feel free to ignore. > +}) > + It would be great to describe, at least briefly, what the test is intended to do and what it checks. > +test:plan(1) > +local utils = require('utils') > +local script = utils.exec.makecmd_bash(arg, {t = test}) > +local output = script( > + utils.exec.luabin(arg), > + utils.exec.test_data(arg, 'script.lua')) test_data returns two values and I think it is bad idea to pass both values to script() > +test:is(output, 'OK') test description is missed > +test:done(true) > diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua > new file mode 100644 > index 00000000..0e6f4078 > --- /dev/null > +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua > @@ -0,0 +1,25 @@ > +local ffi = require("ffi") > + > +ffi.cdef[[ > + int fork(void); > + unsigned int sleep(unsigned int seconds); > + int waitpid(int pid, int *status, int options); > +]] > + > +local pid = ffi.C.fork() > + > +if pid == 0 then > + -- child. > + ffi.C.sleep(10) > +elseif pid > 0 then > + -- parent. > + local status = ffi.new("int[1]") > + -- wait() system call: it restarts after signal is handled > + -- if SA_RESTART flag is set for the handler in sigaction() > + -- or if old signal() is used and it's OS specific implementation > + -- is BSD-aware. > + ffi.C.waitpid(pid, status, 0) > +else > + -- fork() failed. > + os.exit(1) > +end > diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh > new file mode 100644 > index 00000000..7fa3c019 > --- /dev/null > +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh please check script.sh with shellcheck and fix all found warnings > @@ -0,0 +1,28 @@ > +LUA_BIN=$1 > +SCRIPT_TO_RUN=$2 > +$LUA_BIN $SCRIPT_TO_RUN 2>/dev/null & > +pid=$! > + > +# Wait for proccess is started and running. > +sleep 1.5 Using a delay in seconds is a poor method of synchronization. The same is below. With these sleeps the test will be a good candidate for flaky. Also, "1.5" is an outdated value, 3 seconds is used in the branch. > +if ! kill -0 "$pid" 2>/dev/null; then > + echo 'ERR' > + exit 1 > +fi > + > +# Send SIGINT and wait for termination. > +kill -INT "$pid" 2>/dev/null > +sleep 1.5 The test spends ~10 seconds of pure waiting for a single assertion, and it is not only the two `sleep 3`: - The forked child process inherits the `io.popen()` pipe and keeps its write end open until its `sleep(10)` ends, so the harness's `read('*all')` waits for the full 10 seconds even when the check at line 18 already says "terminated". Measured: `ctest -R lj-noticket...` takes 10.01 s, while `script.sh` alone (stdout to a terminal) returns in 6.00 s. - The two fixed sleeps make the test sensitive to a loaded CI in both directions: too short a startup wait gives a false `ERR`, too short a wait after SIGINT gives a false `FAIL`. Would it be more robust to (a) have `script.lua` print a readiness marker (e.g. right before `waitpid()`) and let the shell wait for that line instead of `sleep 3`, (b) poll `kill -0` in a short loop instead of the second `sleep 3`, and (c) make the child not hold the pipe (`ffi.C.close(1)` in the child, or kill/reap it from the parent after the checks)? > + > +# Terminated? OK, signal handled -- 'sigaction() behavior'. > +if ! kill -0 "$pid" 2>/dev/null; then > + wait "$pid" 2>/dev/null > + echo 'OK' > + exit 0 > +fi > + > +# Not terminated -- 'signal() BSD-behavior'. > +kill -KILL "$pid" 2>/dev/null > +wait "$pid" 2>/dev/null > +echo 'FAIL' > +exit 0 The child process created by `script.lua` outlives the parent in both branches: on success (line 21) the parent LuaJIT exits and leaves it orphaned for the rest of the 10 s sleep; on failure (line 25) only the parent is SIGKILLed. So a failed run leaves a stray LuaJIT process behind. Could the parent kill/reap the child before exiting (or `script.sh` signal the process group) so the test leaves nothing behind? > diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua > index 49f8ef06..f19405a2 100644 > --- a/test/tarantool-tests/utils/exec.lua > +++ b/test/tarantool-tests/utils/exec.lua > @@ -43,33 +43,74 @@ local function makeenv(tabenv) > return table.concat(flatenv, ' ') > end > > --- <makecmd> creates a command that runs %testname%/script.lua by > --- <LUAJIT_TEST_BINARY> with the given environment, launch options > --- and CLI arguments. The function yields an object (i.e. table) > --- with the aforementioned parameters. To launch the command just > --- call the object. > -function M.makecmd(arg, opts) > +-- <makecmd_exec> creates a command that runs given binary with the > +-- given environment, launch options and zero or more permanent > +-- arguments. > +-- The function yields an object (i.e. table) with the aforementioned > +-- parameters. To launch the command just call the object. Additional > +-- arguments which may vary from call to call are the `__call` args. > +-- Pass tap.test() object as opt.t for diagnostic. s/opt/opts/ > +function M.makecmd_exec(bin, opts) > + assert(type(bin) == 'string') > + assert(opts == nil or type(opts) == 'table') > + local perm_args = opts and opts.args or {} > + assert(type(perm_args) == 'table') > return setmetatable({ > - LUABIN = M.luacmd(arg), > - SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'), > + BIN = bin, > ENV = opts and makeenv(opts.env) or '', > REDIRECT = opts and opts.redirect or '', > + PERMARGS = (' %s'):rep(#perm_args):format(unpack(perm_args)), > + t = opts and opts.t, > }, { > __call = function(self, ...) > -- This line just makes the command for <io.popen> by the > -- following steps: > -- 1. Replace the placeholders with the corresponding values > - -- given to the command constructor (e.g. script, env). > + -- given to the command constructor (e.g. bin, env). > -- 2. Join all CLI arguments given to the __call metamethod. > -- 3. Concatenate the results of step 1 and step 2 to obtain > -- the resulting command. > - local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self) > + local cmd = ('<ENV> <BIN> <REDIRECT> <PERMARGS>'):gsub('%<(%w+)>', self) > .. (' %s'):rep(select('#', ...)):format(...) > + if self.t ~= nil then self.t:diag('cmd: '..cmd) end > -- Trim both leading and trailing whitespace from the output > -- produced by the child process. > - return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '') > + local output = io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '') > + if self.t ~= nil then self.t:diag('output: '..output) end > + return output > end > }) > end > > +-- Makes a file name for running test's data file in '%testname%' directory. > +function M.test_data(arg, name) > + return arg[0]:gsub('%.test%.lua$', '/'..name) > +end > + > +-- <makecmd_bash> creates a command that runs %testname%/script.sh > +-- with the given environment, launch options and CLI arguments. > +function M.makecmd_bash(arg, opts) > + local script = opts and opts.script or M.test_data(arg, 'script.sh') > + return M.makecmd_exec('/bin/bash', { `makecmd_bash()` hardcodes `/bin/bash`, while the test explicitly enables itself on `jit.os == 'BSD'` and on `'POSIX'`. On FreeBSD, bash is not part of the base system (and lives in `/usr/local/bin` when installed), so the test would produce an empty output and fail there. Since `script.sh` uses only POSIX sh (`$!`, `kill`, `wait`, `&`), would `/bin/sh` (and a rename to `makecmd_sh`) be enough? The CI matrix is Linux/macOS only, so this is not a CI breaker today, but the skip condition promises BSD support. > + env = opts and opts.env or nil, > + redirect = opts and opts.redirect or nil, > + args = {script}, > + t = opts and opts.t or nil, > + }) > +end > + > +-- <makecmd> creates a command that runs %testname%/script.lua by > +-- <LUAJIT_TEST_BINARY> with the given environment, launch options > +-- and CLI arguments. > +function M.makecmd(arg, opts) > + local luabin = M.luacmd(arg) > + local script = opts and opts.script or M.test_data(arg, 'script.lua') > + return M.makecmd_exec(luabin, { > + env = opts and opts.env or nil, > + redirect = opts and opts.redirect or nil, > + args = {script}, > + t = opts and opts.t or nil, > + }) > +end > + > return M [-- Attachment #2: Type: text/html, Size: 21716 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit] Improve CLI signal handling on POSIX. 2026-10-26 12:59 ` [Tarantool-patches] [PATCH luajit] " Sergey Bronnikov via Tarantool-patches @ 2026-11-02 0:02 ` Evgeniy Temirgaleev via Tarantool-patches 0 siblings, 0 replies; 4+ messages in thread From: Evgeniy Temirgaleev via Tarantool-patches @ 2026-11-02 0:02 UTC (permalink / raw) To: Sergey Bronnikov, Sergey Kaplun; +Cc: tarantool-patches Hi, Sergey! Thanks for review! Fixes and a rewritten test (thanks for suggestion) was force pushed to the branch https://github.com/tarantool/luajit/tree/tmr_g/lj-noticket-improve-cli-sig-handling-2. The patch set v3 was send to the mailing list. On 10/26/26 3:59 PM, Sergey Bronnikov wrote: > Hi, Evgeniy, > > thanks for the patch! See my comments below. > > Sergey > > On 10/21/26 20:42, Evgeniy Temirgaleev wrote: >> From: Mike Pall <mike> >> >> (cherry picked from commit 423ac2144b0447c925596f4d93e073ce501ed4e1) >> >> An obsolete signal() function in glibc implemented as a wrapper for > s/implemented/is implemented/ Fixed. >> POSIX sigaction() call with BSD semantic. In particular, it provides >> an SA_RESTART flag for SIGINT handler [1-2]. The last makes certain > s/last/latter/ Fixed. >> system calls, e.g. read() or wait(), restartable across signals [3]. >> >> The patch switches from signal() to sigaction() for POSIX systems. >> >> 1.https://man7.org/linux/man-pages/man2/signal.2.html >> 2.https://ftp.gnu.org/gnu/glibc/glibc-2.44.tar.xz (sysdeps/posix/signal.c, line 44) > > isn't better to replace it with a link to a file in repository? > > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/ > signal.c;h=7a80338fd6270b2b0e713c32c98c66fb284bd196;hb=HEAD > Unfortunately, sourceware.org is not accessible for me. So, I think, widely accessible ftp.gnu.org is better. >> 3.https://man7.org/linux/man-pages/man2/sigaction.2.html >> >> Temir Galeev: >> * added the description and the test for the problem > > Please consider replacing the shell scripts and helpers with an > implementation that uses FFI C > > and libc functions. The draft of the script.lua is below: > > local ffi = require('ffi') > > ffi.cdef[[ > int pipe(int *fds); > int getpid(void); > long read(int fd, void *buf, size_t count); > unsigned int alarm(unsigned int seconds); > ]] > > local fds = ffi.new('int[2]') > assert(ffi.C.pipe(fds) == 0) > > -- Fail instead of hanging forever if SIGINT does not break the call. > ffi.C.alarm(5) > > -- The CLI installs the SIGINT handler before the script starts, so this > -- line is the readiness marker for the test. > io.write(ffi.C.getpid(), '\n') > io.flush() > > -- Block in read(2) until SIGINT. Without SA_RESTART it returns EINTR and > -- the CLI runs the Lua hook, which raises "interrupted!". > ffi.C.read(fds[0], ffi.new('char[1]'), 1) > > and > > test.lua: > > > test:plan(1) > > local ffi = require('ffi') > ffi.cdef[[ > int kill(int pid, int sig); > ]] > > local SIGINT = 2 > local utils = require('utils') > local script = arg[0]:gsub('%.test%.lua$', '/script.lua') > local cmd = ('exec %s %s 2>&1'):format(utils.exec.luacmd(arg), script) > local proc = io.popen(cmd) > > -- The child prints its PID after the CLI has installed the SIGINT > -- handler, so it is a readiness marker. Skip possible noise lines. > local pid > for _ = 1, 10 do > local line = proc:read('*l') > if line == nil then break end > pid = line:match('^%d+$') and tonumber(line) > if pid ~= nil then break end > end > assert(pid ~= nil, 'the child process must report its PID') > > ffi.C.kill(pid, SIGINT) > local output = proc:read('*a') > proc:close() > test:like(output, 'interrupted!', 'SIGINT interrupts the blocking call') > test:done(true) > Many thanks! This scheme is much simpler than the waitpid() and a bash helper. >> Part of tarantool/tarantool#13442 >> --- >> >> Branch:https://github.com/tarantool/luajit/tree/tmr_g/lj-noticket-improve- >> cli-sig-handling-2 >> >> src/luajit.c | 23 +++++-- >> ...ket-fix-cli-posix-sigint-handling.test.lua | 17 +++++ >> .../script.lua | 25 ++++++++ >> .../script.sh | 28 +++++++++ >> test/tarantool-tests/utils/exec.lua | 63 +++++++++++++++---- >> 5 files changed, 141 insertions(+), 15 deletions(-) >> create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua >> create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua >> create mode 100644 test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh > The commit mixes three things: the C change, its test, and a generic > refactoring > of `utils/exec.lua` (new `makecmd_exec`/`makecmd_bash`/`test_data`, with > `makecmd` rewritten on top of them). I would split the existing patch to > two patches. > Split into two patches. >> diff --git a/src/luajit.c b/src/luajit.c >> index d9d85361..d35bea54 100644 >> --- a/src/luajit.c >> +++ b/src/luajit.c >> @@ -35,6 +35,21 @@ >> >> #if !LJ_TARGET_CONSOLE >> #include <signal.h> >> + >> +#if LJ_TARGET_POSIX >> +/* Improve signal handling on POSIX. Try CTRL-C on: luajit -e 'io.read()' */ >> +static void signal_set(int sig, void (*h)(int)) >> +{ >> + struct sigaction sa; >> + memset(&sa, 0, sizeof(sa)); > `signal_set()` drops `SA_RESTART` only implicitly: `memset(&sa, 0, > sizeof(sa))` > zeroes `sa_flags` and nothing is assigned to the field afterwards. That is > exactly what the patch needs, but a reader (or a future "sync with glibc" > change) may see the zero flags as an omission and restore `SA_RESTART`, > silently reverting the fix. It might be worth explicitly noting this in > the commit message. Thanks! Added the notice about removing of the SA_RESTART flag by the patch. >> + sa.sa_handler = h; >> + sigemptyset(&sa.sa_mask); >> + sigaction(sig, &sa, NULL); >> +} >> +#else >> +#define signal_set signal >> +#endif >> + >> #endif >> >> static lua_State *globalL = NULL; >> @@ -54,8 +69,8 @@ static void lstop(lua_State *L, lua_Debug *ar) >> >> static void laction(int i) >> { >> - signal(i, SIG_DFL); /* if another SIGINT happens before lstop, >> - terminate process (default action) */ >> + /* Terminate process if another SIGINT happens (double CTRL-C). */ >> + signal_set(i, SIG_DFL); >> lua_sethook(globalL, lstop, LUA_MASKCALL | LUA_MASKRET | LUA_MASKCOUNT, 1); >> } >> #endif >> @@ -129,11 +144,11 @@ static int docall(lua_State *L, int narg, int clear) >> lua_pushcfunction(L, traceback); /* push traceback function */ >> lua_insert(L, base); /* put it under chunk and args */ >> #if !LJ_TARGET_CONSOLE >> - signal(SIGINT, laction); >> + signal_set(SIGINT, laction); >> #endif >> status = lua_pcall(L, narg, (clear ? 0 : LUA_MULTRET), base); >> #if !LJ_TARGET_CONSOLE >> - signal(SIGINT, SIG_DFL); >> + signal_set(SIGINT, SIG_DFL); >> #endif >> lua_remove(L, base); /* remove traceback function */ >> /* force a complete garbage collection in case of errors */ >> diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua >> new file mode 100644 >> index 00000000..4ea316d8 >> --- /dev/null >> +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua >> @@ -0,0 +1,17 @@ >> +local tap = require('tap') >> +local test = tap.test('lj-noticket-fix-cli-posix-sigint-handling'):skipcond({ >> + ['Check LuaJIT CLI signal handling'] = _TARANTOOL, > The skip reason `['Check LuaJIT CLI signal handling'] = _TARANTOOL` > describes > what the test checks, not why it is skipped under Tarantool. Added a note. >> + ['Disabled on POSIX nonconformant OS'] = jit.os ~= 'BSD' and >> + jit.os ~= 'Linux' and >> + jit.os ~= 'OSX' and >> + jit.os ~= 'POSIX', > > the condition can be inverted and contain only two items: > > if jit.os == 'Windows' or jit.os == 'Other' > > Feel free to ignore. > Yes. I think the explicit enumeration is better. >> +}) >> + > It would be great to describe, at least briefly, what the test is > intended to do and what it checks. Added (for new test). >> +test:plan(1) >> +local utils = require('utils') >> +local script = utils.exec.makecmd_bash(arg, {t = test}) >> +local output = script( >> + utils.exec.luabin(arg), >> + utils.exec.test_data(arg, 'script.lua')) > test_data returns two values and I think it is bad idea to pass both > values to script() Fixed test_data(). >> +test:is(output, 'OK') > test description is missed Not actual: removed in new test. >> +test:done(true) >> diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua >> new file mode 100644 >> index 00000000..0e6f4078 >> --- /dev/null >> +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua >> @@ -0,0 +1,25 @@ >> +local ffi = require("ffi") >> + >> +ffi.cdef[[ >> + int fork(void); >> + unsigned int sleep(unsigned int seconds); >> + int waitpid(int pid, int *status, int options); >> +]] >> + >> +local pid = ffi.C.fork() >> + >> +if pid == 0 then >> + -- child. >> + ffi.C.sleep(10) >> +elseif pid > 0 then >> + -- parent. >> + local status = ffi.new("int[1]") >> + -- wait() system call: it restarts after signal is handled >> + -- if SA_RESTART flag is set for the handler in sigaction() >> + -- or if old signal() is used and it's OS specific implementation >> + -- is BSD-aware. >> + ffi.C.waitpid(pid, status, 0) >> +else >> + -- fork() failed. >> + os.exit(1) >> +end >> diff --git a/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh >> new file mode 100644 >> index 00000000..7fa3c019 >> --- /dev/null >> +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.sh > please check script.sh with shellcheck and fix all found warnings Not actual: removed in new test. >> @@ -0,0 +1,28 @@ >> +LUA_BIN=$1 >> +SCRIPT_TO_RUN=$2 >> +$LUA_BIN $SCRIPT_TO_RUN 2>/dev/null & >> +pid=$! >> + >> +# Wait for proccess is started and running. >> +sleep 1.5 > > Using a delay in seconds is a poor method of synchronization. The same > is below. > > With these sleeps the test will be a good candidate for flaky. > > Also, "1.5" is an outdated value, 3 seconds is used in the branch. > Used suggested scheme with 'pipe' sync. >> +if ! kill -0 "$pid" 2>/dev/null; then >> + echo 'ERR' >> + exit 1 >> +fi >> + >> +# Send SIGINT and wait for termination. >> +kill -INT "$pid" 2>/dev/null >> +sleep 1.5 > The test spends ~10 seconds of pure waiting for a single assertion, and > it is > not only the two `sleep 3`: > > - The forked child process inherits the `io.popen()` pipe and keeps its > write > end open until its `sleep(10)` ends, so the harness's `read('*all')` > waits > for the full 10 seconds even when the check at line 18 already says > "terminated". Measured: `ctest -R lj-noticket...` takes 10.01 s, while > `script.sh` alone (stdout to a terminal) returns in 6.00 s. > - The two fixed sleeps make the test sensitive to a loaded CI in both > directions: too short a startup wait gives a false `ERR`, too short a > wait > after SIGINT gives a false `FAIL`. > > Would it be more robust to (a) have `script.lua` print a readiness > marker (e.g. > right before `waitpid()`) and let the shell wait for that line instead of > `sleep 3`, (b) poll `kill -0` in a short loop instead of the second > `sleep 3`, > and (c) make the child not hold the pipe (`ffi.C.close(1)` in the child, or > kill/reap it from the parent after the checks)? Used suggested scheme with 'pipe' sync. >> + >> +# Terminated? OK, signal handled -- 'sigaction() behavior'. >> +if ! kill -0 "$pid" 2>/dev/null; then >> + wait "$pid" 2>/dev/null >> + echo 'OK' >> + exit 0 >> +fi >> + >> +# Not terminated -- 'signal() BSD-behavior'. >> +kill -KILL "$pid" 2>/dev/null >> +wait "$pid" 2>/dev/null >> +echo 'FAIL' >> +exit 0 > The child process created by `script.lua` outlives the parent in both > branches: > on success (line 21) the parent LuaJIT exits and leaves it orphaned for > the rest > of the 10 s sleep; on failure (line 25) only the parent is SIGKILLed. So a > failed run leaves a stray LuaJIT process behind. Could the parent kill/ > reap the > child before exiting (or `script.sh` signal the process group) so the test > leaves nothing behind? Not actual: removed in new test. >> diff --git a/test/tarantool-tests/utils/exec.lua b/test/tarantool-tests/utils/exec.lua >> index 49f8ef06..f19405a2 100644 >> --- a/test/tarantool-tests/utils/exec.lua >> +++ b/test/tarantool-tests/utils/exec.lua >> @@ -43,33 +43,74 @@ local function makeenv(tabenv) >> return table.concat(flatenv, ' ') >> end >> >> --- <makecmd> creates a command that runs %testname%/script.lua by >> --- <LUAJIT_TEST_BINARY> with the given environment, launch options >> --- and CLI arguments. The function yields an object (i.e. table) >> --- with the aforementioned parameters. To launch the command just >> --- call the object. >> -function M.makecmd(arg, opts) >> +-- <makecmd_exec> creates a command that runs given binary with the >> +-- given environment, launch options and zero or more permanent >> +-- arguments. >> +-- The function yields an object (i.e. table) with the aforementioned >> +-- parameters. To launch the command just call the object. Additional >> +-- arguments which may vary from call to call are the `__call` args. >> +-- Pass tap.test() object as opt.t for diagnostic. > s/opt/opts/ Fixed. >> +function M.makecmd_exec(bin, opts) >> + assert(type(bin) == 'string') >> + assert(opts == nil or type(opts) == 'table') >> + local perm_args = opts and opts.args or {} >> + assert(type(perm_args) == 'table') >> return setmetatable({ >> - LUABIN = M.luacmd(arg), >> - SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'), >> + BIN = bin, >> ENV = opts and makeenv(opts.env) or '', >> REDIRECT = opts and opts.redirect or '', >> + PERMARGS = (' %s'):rep(#perm_args):format(unpack(perm_args)), >> + t = opts and opts.t, >> }, { >> __call = function(self, ...) >> -- This line just makes the command for <io.popen> by the >> -- following steps: >> -- 1. Replace the placeholders with the corresponding values >> - -- given to the command constructor (e.g. script, env). >> + -- given to the command constructor (e.g. bin, env). >> -- 2. Join all CLI arguments given to the __call metamethod. >> -- 3. Concatenate the results of step 1 and step 2 to obtain >> -- the resulting command. >> - local cmd = ('<ENV> <LUABIN> <REDIRECT> <SCRIPT>'):gsub('%<(%w+)>', self) >> + local cmd = ('<ENV> <BIN> <REDIRECT> <PERMARGS>'):gsub('%<(%w+)>', self) >> .. (' %s'):rep(select('#', ...)):format(...) >> + if self.t ~= nil then self.t:diag('cmd: '..cmd) end >> -- Trim both leading and trailing whitespace from the output >> -- produced by the child process. >> - return io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '') >> + local output = io.popen(cmd):read('*all'):gsub('^%s+', ''):gsub('%s+$', '') >> + if self.t ~= nil then self.t:diag('output: '..output) end >> + return output >> end >> }) >> end >> >> +-- Makes a file name for running test's data file in '%testname%' directory. >> +function M.test_data(arg, name) >> + return arg[0]:gsub('%.test%.lua$', '/'..name) >> +end >> + >> +-- <makecmd_bash> creates a command that runs %testname%/script.sh >> +-- with the given environment, launch options and CLI arguments. >> +function M.makecmd_bash(arg, opts) >> + local script = opts and opts.script or M.test_data(arg, 'script.sh') >> + return M.makecmd_exec('/bin/bash', { > `makecmd_bash()` hardcodes `/bin/bash`, while the test explicitly enables > itself on `jit.os == 'BSD'` and on `'POSIX'`. On FreeBSD, bash is not > part of > the base system (and lives in `/usr/local/bin` when installed), so the test > would produce an empty output and fail there. Since `script.sh` uses > only POSIX > sh (`$!`, `kill`, `wait`, `&`), would `/bin/sh` (and a rename to > `makecmd_sh`) > be enough? The CI matrix is Linux/macOS only, so this is not a CI breaker > today, but the skip condition promises BSD support. The makecmd_bash() is not used in new test, so it is removed. >> + env = opts and opts.env or nil, >> + redirect = opts and opts.redirect or nil, >> + args = {script}, >> + t = opts and opts.t or nil, >> + }) >> +end >> + >> +-- <makecmd> creates a command that runs %testname%/script.lua by >> +-- <LUAJIT_TEST_BINARY> with the given environment, launch options >> +-- and CLI arguments. >> +function M.makecmd(arg, opts) >> + local luabin = M.luacmd(arg) >> + local script = opts and opts.script or M.test_data(arg, 'script.lua') >> + return M.makecmd_exec(luabin, { >> + env = opts and opts.env or nil, >> + redirect = opts and opts.redirect or nil, >> + args = {script}, >> + t = opts and opts.t or nil, >> + }) >> +end >> + >> return M -- -- Best regards, Evgeniy Temirgaleev ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-11-02 0:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-10-21 17:42 [Tarantool-patches] [PATCH luajit] Improve CLI signal handling on POSIX Evgeniy Temirgaleev via Tarantool-patches 2026-10-21 23:23 ` [Tarantool-patches] [PATCH luajit v2] " Evgeniy Temirgaleev via Tarantool-patches 2026-10-26 12:59 ` [Tarantool-patches] [PATCH luajit] " Sergey Bronnikov via Tarantool-patches 2026-11-02 0:02 ` Evgeniy Temirgaleev via Tarantool-patches
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox