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

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

Sergey, thanks for review!

Please, see my answers below. Fixes are force pushed to the branch.

On 11/3/26 3:14 PM, Sergey Kaplun wrote:
> 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/
> 
Fixed.

>> 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/
> 
Fixed

>> +-- 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/
> 
Fixed

>> +-- 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/
> 
Fixed

>> +-- 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/
>
Fixed


>> +-- 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/
> 
Fixed

>> +-- 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
> 
Here is a single quite inside a string.

>> +  ['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
> 
Fixed.

>> +-- 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.
> 
`io.read()` may not block in non console environment. Added a comment.

>> -- 
>> 2.49.0
>>
> 

---
Commit msg:

...
An obsolete signal() function in glibc is implemented as a wrapper for
POSIX sigaction() call with BSD semantics. In particular, it provides
an SA_RESTART flag for SIGINT handler [1-2]. The latter makes certain
system calls, e.g. read() or wait(), restartable across signals [3].
...

---

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
index 5b235660..e080b7f3 100644
--- 
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
@@ -1,17 +1,17 @@
  local tap = require('tap')

--- The test checks that luajit CLI stop hook is executed on
+-- The test checks that LuaJIT CLI stop hook is executed on
  -- SIGINT on a blocking system call (read(), waitpid(), etc.).
  --
--- The test runs luajit CLI with a script, which prints it's PID
+-- The test runs LuaJIT CLI with a script, which prints its PID
  -- 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
--- SIGINT to the child.
+-- the child's PID number is read from child's pipe, the test
+-- 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.
+-- is executed.
  -- Without a patch (SA_RESTART flag is present for SIGINT
--- handler) read() call is restarted, the test and the child is
+-- handler) read() call is restarted, the test and the child are
  -- blocked on a pipe read until the child is terminated by
  -- SIGALRM. The test reads nothing from the child and fails.

@@ -37,7 +37,7 @@ 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+)$'
+local RMARKER = '^' .. MARKER .. '(%d+)$'
  -- 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.
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
index d2430a52..fab558e7 100644
--- 
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
@@ -1,15 +1,10 @@
  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);
  ]]
+
+-- The script runs via 'popen()' with closed pipe read-end,
+-- and the stdin is not guaranteed to be connected somewhere.
+-- Reading from it may result in EOF.
+-- So, establish the new pipe, which read-end would block.
  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)

-- 
--
Best regards,
Evgeniy Temirgaleev