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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- 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 --]