Re: [Tarantool-patches] [PATCH luajit] Improve CLI signal handling on POSIX.

[Evgeniy Temirgaleev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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