Hi, Evgeniy,

thanks for the patch! See my comments below.

Sergey

From: Mike Pall <mike>

(cherry picked from commit 423ac2144b0447c925596f4d93e073ce501ed4e1)

An obsolete signal() function in glibc implemented as a wrapper for

POSIX sigaction() call with BSD semantic. In particular, it provides
an SA_RESTART flag for SIGINT handler [1-2]. The last makes certain

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

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

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

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

+})
+

+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:is(output, 'OK')

+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

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

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

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.

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

+    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