From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 19F836ECCD; Mon, 26 Oct 2026 15:59:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 19F836ECCD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1793019589; bh=plYg574840Xn8gj9s7qdk+Qy63CG+r3btN6JzkZLeKA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=dfuAED712PkXecnEvaokQ+SCbawYf2Lhw/mc5OiE7pTr5fr4qhXCrUOYZmjcqO9wz HAkYr5X1QwcmHSS6HPXe3hXjOAi4Lb4JnlI6iBLoLn56VYHBcMw7EdSr7PWkf9glnI vCmDlpYFZVCXB2lmV/fyBQbqukCl9byYaN378TeE= Received: from send103.i.mail.ru (send103.i.mail.ru [89.221.237.198]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 351686ECCD for ; Mon, 26 Oct 2026 15:59:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 351686ECCD Received: by exim-smtp-66b45bc555-ksp8n with esmtpa (envelope-from ) id 1xLKIj-00000000RC9-3Nli; Mon, 26 Oct 2026 15:59:46 +0300 Content-Type: multipart/alternative; boundary="------------u70rHqEaJEXw64aAT3tpk5JH" Message-ID: Date: Mon, 26 Oct 2026 15:59:44 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Evgeniy Temirgaleev , Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org References: <20261021174233.78149-1-e.temirgaleev@tarantool.org> Content-Language: en-US In-Reply-To: <20261021174233.78149-1-e.temirgaleev@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD91C9F741F9EDD82FD3F02C43105D005BECF2E61454A620773182A05F538085040A34C5C4B0C49C4B93DE06ABAFEAF6705043E0E9BC08C57DC27D1953332CAA6DF10FFD2E5AA185B68 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70A10A23A3B64B805EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB5533756651C56E7D1F94A30BFE7225EE3A3D367B4DB6D394E2D61B2D33012EDAB8886B5E389733CBF5DBD5E9D5E8D9A59859A8B6D082881546D93491CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB86D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE73349F3DFB73D9B11731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5A6F4FA762A371CE55002B1117B3ED696532F35E98E479BD7B2920F75BA9A967F823CB91A9FED034534781492E4B8EEADEC87C13A22AA34BEBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0AD73CAD6646DEDE191716CD42B3DD1D34CAB70F9BE574AE9C625B6776AC983F447FC0B9F89525902EE6F57B2FD27647F25E66C117BDB76D65965D6A20E6D6250346970ACB03413D1BA7795A3ED69DF1A7089639F674ADF7760574B7CA3A1530852B8341EE9D5BE9A0A67FB1B8C848647FF6D788CDB14B69A70A16DEF2B06041B4C6536EB022892E5344C41F94D744909CE2512F26BEC029E55448553D2254B8D95CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXOcIETfglQORZ0zpDET4Zrk3igikrdHlWFI3hTINnl9Ql/VZul3y37o= X-Mailru-Sender: 689FA8AB762F73937C9FA53A4753B31383E74AA07D2E63A0DD511F906A46E135675B55E463DED49AEF86D5F70DA33880E41E8EF7A07863ECB274557F927329BE2DDF8182D28ACDB545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Improve CLI signal handling on POSIX. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------u70rHqEaJEXw64aAT3tpk5JH Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Evgeniy, thanks for the patch! See my comments below. Sergey On 10/21/26 20:42, Evgeniy Temirgaleev wrote: > From: Mike Pall > > (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 > + > +#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 > > --- creates a command that runs %testname%/script.lua by > --- 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) > +-- 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 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 = ('