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

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

Envgeniy,
Thanks for the patch!
LGTM, with minor nits below.

On 02.11.26, Evgeniy Temirgaleev wrote:
> From: Mike Pall <mike>
> 
> (cherry picked from commit 423ac2144b0447c925596f4d93e073ce501ed4e1)
> 
> An obsolete signal() function in glibc is implemented as a wrapper for
> POSIX sigaction() call with BSD semantic. In particular, it provides

Typo: s/semantic/semantics/

> an SA_RESTART flag for SIGINT handler [1-2]. The latter makes certain
> system calls, e.g. read() or wait(), restartable across signals [3].
> 
> The patch switches from signal() to sigaction() for POSIX systems
> to obtain an explicit control of the signal flag set, which must
> not contain SA_RESTART flag. (Actually, the empty flag set is
> used.)
> 
> 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
> ---
>  src/luajit.c                                  | 23 +++++--
>  ...ket-fix-cli-posix-sigint-handling.test.lua | 64 +++++++++++++++++++
>  .../script.lua                                | 26 ++++++++
>  3 files changed, 109 insertions(+), 4 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
> 
> diff --git a/src/luajit.c b/src/luajit.c
> index d9d85361..d35bea54 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c

<snipped>

> 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..5b235660
> --- /dev/null
> +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling.test.lua
> @@ -0,0 +1,64 @@
> +local tap = require('tap')
> +
> +-- The test checks that luajit CLI stop hook is executed on

Typo: s/luajit/the LuaJIT/

> +-- SIGINT on a blocking system call (read(), waitpid(), etc.).
> +--
> +-- The test runs luajit CLI with a script, which prints it's PID

Typo: s/luajit/the LuaJIT/
Typo: s/it's/its/

> +-- as a ready marker and blocks on a read() call. After the
> +-- child's PID number is read from child's pipe, the test send

Typo: s/child's/the child's/
Typo: s/send/sends/

> +-- SIGINT to the child.
> +-- With a patch applied (no SA_RESTART flag for SIGINT handler)
> +-- read() call is interrupted and the CLI's Lua stop hook
> +-- executed.

Typo: s/executed/is executed/

> +-- Without a patch (SA_RESTART flag is present for SIGINT
> +-- handler) read() call is restarted, the test and the child is

Typo: s/is blocked/are blocked/

> +-- blocked on a pipe read until the child is terminated by
> +-- SIGALRM. The test reads nothing from the child and fails.
> +
> +local test = tap.test('lj-noticket-fix-cli-posix-sigint-handling'):skipcond({
> +  -- The patched code is not present in Tarantool binary:
> +  ["Disabled due to Tarantool uses it's own signal handling"] = _TARANTOOL,

Nit: s/"/'/g

> +  ['Disabled on POSIX nonconformant OS'] = jit.os ~= 'BSD' and
> +                                           jit.os ~= 'Linux' and
> +                                           jit.os ~= 'OSX' and
> +                                           jit.os ~= 'POSIX',
> +})
> +
> +local ffi = require('ffi')
> +ffi.cdef[[
> +  int kill(int pid, int sig);
> +]]
> +local SIGINT = 2
> +
> +test:plan(1)
> +local utils = require('utils')
> +local script = utils.exec.test_data(arg, 'script.lua')
> +local cmd = ('exec %s %s 2>&1'):format(utils.exec.luacmd(arg), script)
> +local proc = io.popen(cmd)
> +
> +local MARKER = 'CHILD_PID:'
> +local RMARKER = '^'..MARKER..'(%d+)$'

Typo: s/../ .. /g

> +-- The child prints its PID after the CLI has installed the SIGINT
> +-- handler, so it is a readiness marker. Skip possible noise
> +-- lines, collect pre-marker output for error reporting.
> +local lines_for_diag = {'child output:'}
> +local pid, line
> +repeat
> +  line = proc:read('*l')
> +  if line ~= nil then
> +    table.insert(lines_for_diag, line)
> +    local match = line:match(RMARKER)
> +    if match ~= nil then pid = tonumber(match) end
> +  end
> +until line == nil or pid ~= nil
> +if pid == nil then
> +  test:diag(table.concat(lines_for_diag, '\n'))
> +end
> +assert(pid ~= nil, 'the child process must report its PID')
> +
> +local res = ffi.C.kill(pid, SIGINT)
> +assert(res == 0, 'SIGINT delivery failed')
> +local output = proc:read('*a')
> +proc:close()
> +test:like(output, 'interrupted!', 'SIGINT interrupts the blocking call')
> +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..d2430a52
> --- /dev/null
> +++ b/test/tarantool-tests/lj-noticket-fix-cli-posix-sigint-handling/script.lua
> @@ -0,0 +1,26 @@
> +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. (SIGALRM isn't handled by CLI.)
> +ffi.C.alarm(10)
> +
> +-- The CLI installs the SIGINT handler before the script
> +-- starts, so this line is the readiness marker for the test.
> +local MARKER = 'CHILD_PID:'
> +io.write(MARKER, 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)

Why don't use `io.read()` here? Same semantics but additional
correlation with the comment in sources.

> -- 
> 2.49.0
> 

-- 
Best regards,
Sergey Kaplun