<!DOCTYPE html>

<html>

  <head>

    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">

  </head>

  <body>

    <p>Hi, Evgeniy,</p>

    <p>thanks for the patch! See my comments below.</p>

    <p>Sergey</p>

    <div class="moz-cite-prefix">On 10/21/26 20:42, Evgeniy Temirgaleev

      wrote:<br>

    </div>

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">From: Mike Pall <mike>

(cherry picked from commit 423ac2144b0447c925596f4d93e073ce501ed4e1)

An obsolete signal() function in glibc implemented as a wrapper for</pre>

    </blockquote>

    s/implemented/is implemented/

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

POSIX sigaction() call with BSD semantic. In particular, it provides

an SA_RESTART flag for SIGINT handler [1-2]. The last makes certain</pre>

    </blockquote>

    s/last/latter/

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

system calls, e.g. read() or wait(), restartable across signals [3].

The patch switches from signal() to sigaction() for POSIX systems.

1. <a class="moz-txt-link-freetext" href="https://man7.org/linux/man-pages/man2/signal.2.html">https://man7.org/linux/man-pages/man2/signal.2.html</a>

2. <a class="moz-txt-link-freetext" href="https://ftp.gnu.org/gnu/glibc/glibc-2.44.tar.xz">https://ftp.gnu.org/gnu/glibc/glibc-2.44.tar.xz</a> (sysdeps/posix/signal.c, line 44)</pre>

    </blockquote>

    <p>isn't better to replace it with a link to a file in repository?</p>

    <p><a class="moz-txt-link-freetext" href="https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/signal.c;h=7a80338fd6270b2b0e713c32c98c66fb284bd196;hb=HEAD">https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/signal.c;h=7a80338fd6270b2b0e713c32c98c66fb284bd196;hb=HEAD</a></p>

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

3. <a class="moz-txt-link-freetext" href="https://man7.org/linux/man-pages/man2/sigaction.2.html">https://man7.org/linux/man-pages/man2/sigaction.2.html</a>

Temir Galeev:

* added the description and the test for the problem</pre>

    </blockquote>

    <p><span class="HwtZe" lang="en"><span class="jCAhz"><span

            class="ryNqvb">Please consider replacing the shell scripts

            and helpers with an implementation that uses FFI C</span></span></span></p>

    <p><span class="HwtZe" lang="en"><span class="jCAhz"><span

            class="ryNqvb">and libc functions. The draft of the

            script.lua is below:</span></span></span></p>

    <p><span class="HwtZe" lang="en"><span class="jCAhz"><span

            class="ryNqvb">local ffi = require('ffi')<br>

            <br>

            ffi.cdef[[<br>

              int pipe(int *fds);<br>

              int getpid(void);<br>

              long read(int fd, void *buf, size_t count);<br>

              unsigned int alarm(unsigned int seconds);<br>

            ]]<br>

            <br>

            local fds = ffi.new('int[2]')<br>

            assert(ffi.C.pipe(fds) == 0)<br>

            <br>

            -- Fail instead of hanging forever if SIGINT does not break

            the call.<br>

            ffi.C.alarm(5)<br>

            <br>

            -- The CLI installs the SIGINT handler before the script

            starts, so this<br>

            -- line is the readiness marker for the test.<br>

            io.write(ffi.C.getpid(), '\n')<br>

            io.flush()<br>

            <br>

            -- Block in read(2) until SIGINT. Without SA_RESTART it

            returns EINTR and<br>

            -- the CLI runs the Lua hook, which raises "interrupted!".<br>

            ffi.C.read(fds[0], ffi.new('char[1]'), 1)</span></span></span></p>

    <p><span class="HwtZe" lang="en"><span class="jCAhz"><span

            class="ryNqvb">and </span></span></span></p>

    <p><span class="HwtZe" lang="en"><span class="jCAhz"><span

            class="ryNqvb">test.lua:</span></span></span></p>

    <p><span class="HwtZe" lang="en"><span class="jCAhz"><span

            class="ryNqvb"><br>

          </span></span></span></p>

    <p><span class="HwtZe" lang="en"><span class="jCAhz"><span

            class="ryNqvb">test:plan(1)<br>

            <br>

            local ffi = require('ffi')<br>

            ffi.cdef[[<br>

              int kill(int pid, int sig);<br>

            ]]<br>

            <br>

            local SIGINT = 2<br>

            local utils = require('utils')<br>

            local script = arg[0]:gsub('%.test%.lua$', '/script.lua')<br>

            local cmd = ('exec %s %s

            2>&1'):format(utils.exec.luacmd(arg), script)<br>

            local proc = io.popen(cmd)<br>

            <br>

            -- The child prints its PID after the CLI has installed the

            SIGINT<br>

            -- handler, so it is a readiness marker. Skip possible noise

            lines.<br>

            local pid<br>

            for _ = 1, 10 do<br>

              local line = proc:read('*l')<br>

              if line == nil then break end<br>

              pid = line:match('^%d+$') and tonumber(line)<br>

              if pid ~= nil then break end<br>

            end<br>

            assert(pid ~= nil, 'the child process must report its PID')<br>

            <br>

            ffi.C.kill(pid, SIGINT)<br>

            local output = proc:read('*a')<br>

            proc:close()<br>

            test:like(output, 'interrupted!', 'SIGINT interrupts the

            blocking call')<br>

            test:done(true)</span></span></span></p>

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

Part of tarantool/tarantool#13442

---

Branch: <a class="moz-txt-link-freetext" href="https://github.com/tarantool/luajit/tree/tmr_g/lj-noticket-improve-cli-sig-handling-2">https://github.com/tarantool/luajit/tree/tmr_g/lj-noticket-improve-cli-sig-handling-2</a>

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

    </blockquote>

    The commit mixes three things: the C change, its test, and a generic

    refactoring<br>

    of `utils/exec.lua` (new `makecmd_exec`/`makecmd_bash`/`test_data`,

    with<br>

    `makecmd` rewritten on top of them). I would split the existing

    patch to two patches.<br>

    <br>

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

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));</pre>

    </blockquote>

    `signal_set()` drops `SA_RESTART` only implicitly: `memset(&sa,

    0, sizeof(sa))`<br>

    zeroes `sa_flags` and nothing is assigned to the field afterwards.

    That is<br>

    exactly what the patch needs, but a reader (or a future "sync with

    glibc"<br>

    change) may see the zero flags as an omission and restore

    `SA_RESTART`,<br>

    silently reverting the fix. <span class="HwtZe" lang="en"><span

        class="jCAhz JpY6Fd"><span class="ryNqvb"></span></span> <span

        class="jCAhz ChMk0b"><span class="ryNqvb">It might be worth

          explicitly noting this in the commit message.</span></span></span>

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

+  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,</pre>

    </blockquote>

    The skip reason `['Check LuaJIT CLI signal handling'] = _TARANTOOL`

    describes<br>

    what the test checks, not why it is skipped under Tarantool. 

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

+  ['Disabled on POSIX nonconformant OS'] = jit.os ~= 'BSD' and

+                                           jit.os ~= 'Linux' and

+                                           jit.os ~= 'OSX' and

+                                           jit.os ~= 'POSIX',</pre>

    </blockquote>

    <p>the condition can be inverted and contain only two items:</p>

    <p>if jit.os == 'Windows' or jit.os == 'Other'</p>

    <p>Feel free to ignore.</p>

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

+})

+</pre>

    </blockquote>

    <span class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span

          class="ryNqvb">It would be great to describe, at least

          briefly, what the test is intended to do and what it checks.</span></span></span>

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

+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'))</pre>

    </blockquote>

    test_data returns two values and I think it is bad idea to pass both

    values to script()

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

+test:is(output, 'OK')</pre>

    </blockquote>

    test description is missed

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

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

    </blockquote>

    please check script.sh with shellcheck and fix all found warnings

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

@@ -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</pre>

    </blockquote>

    <p><span class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span

            class="ryNqvb">Using a delay in seconds is a poor method of

            synchronization. The same is below.</span></span></span></p>

    <p><span class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span

            class="ryNqvb">With these sleeps the test will be a good

            candidate for flaky.</span></span></span></p>

    <p>Also, "1.5" is an outdated value, 3 seconds is used in the

      branch.</p>

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

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

    </blockquote>

    The test spends ~10 seconds of pure waiting for a single assertion,

    and it is<br>

    not only the two `sleep 3`:<br>

    <br>

    - The forked child process inherits the `io.popen()` pipe and keeps

    its write<br>

      end open until its `sleep(10)` ends, so the harness's

    `read('*all')` waits<br>

      for the full 10 seconds even when the check at line 18 already

    says<br>

      "terminated". Measured: `ctest -R lj-noticket...` takes 10.01 s,

    while<br>

      `script.sh` alone (stdout to a terminal) returns in 6.00 s.<br>

    - The two fixed sleeps make the test sensitive to a loaded CI in

    both<br>

      directions: too short a startup wait gives a false `ERR`, too

    short a wait<br>

      after SIGINT gives a false `FAIL`.<br>

    <br>

    Would it be more robust to (a) have `script.lua` print a readiness

    marker (e.g.<br>

    right before `waitpid()`) and let the shell wait for that line

    instead of<br>

    `sleep 3`, (b) poll `kill -0` in a short loop instead of the second

    `sleep 3`,<br>

    and (c) make the child not hold the pipe (`ffi.C.close(1)` in the

    child, or<br>

    kill/reap it from the parent after the checks)?

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

+

+# 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</pre>

    </blockquote>

    The child process created by `script.lua` outlives the parent in

    both branches:<br>

    on success (line 21) the parent LuaJIT exits and leaves it orphaned

    for the rest<br>

    of the 10 s sleep; on failure (line 25) only the parent is

    SIGKILLed. So a<br>

    failed run leaves a stray LuaJIT process behind. Could the parent

    kill/reap the<br>

    child before exiting (or `script.sh` signal the process group) so

    the test<br>

    leaves nothing behind?

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

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.</pre>

    </blockquote>

    s/opt/opts/

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

+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', {</pre>

    </blockquote>

    `makecmd_bash()` hardcodes `/bin/bash`, while the test explicitly

    enables<br>

    itself on `jit.os == 'BSD'` and on `'POSIX'`. On FreeBSD, bash is

    not part of<br>

    the base system (and lives in `/usr/local/bin` when installed), so

    the test<br>

    would produce an empty output and fail there. Since `script.sh` uses

    only POSIX<br>

    sh (`$!`, `kill`, `wait`, `&`), would `/bin/sh` (and a rename to

    `makecmd_sh`)<br>

    be enough? The CI matrix is Linux/macOS only, so this is not a CI

    breaker<br>

    today, but the skip condition promises BSD support.

    <blockquote type="cite"

      cite="mid:20261021174233.78149-1-e.temirgaleev@tarantool.org">

      <pre wrap="" class="moz-quote-pre">

+    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

</pre>

    </blockquote>

  </body>

</html>