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 A12EA6EFD6; Mon, 2 Nov 2026 03:02:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A12EA6EFD6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1793577736; bh=gDa+NMM81YEE1VGfoAeJBZO7mjMX2dNQmCqFeKrPkKI=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=sBv8tqsU22QGLaR5qtAKjQE7PpyHS9Ozxk9Zrs7z0J5Wg0ZevWVmnbl/K5ghe6k9D 16CDLH75Wu0iBFZN64frgg6Wewh92MyDpl1GXs4+Niix96feBWYQBEHFxlg4y0gdnD f0C3szv0UKXh0M/yP3N8A0+O4I/Sgr9vmn1+E4fM= Received: from send35.i.mail.ru (send35.i.mail.ru [89.221.237.130]) (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 DB7E86EFD6 for ; Mon, 2 Nov 2026 03:02:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DB7E86EFD6 Received: by exim-smtp-675c6798c6-6jjcs with esmtpa (envelope-from ) id 1xNfV8-00000000N3A-2k7a; Mon, 02 Nov 2026 03:02:15 +0300 Message-ID: Date: Mon, 2 Nov 2026 03:02:14 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Bronnikov , Sergey Kaplun References: <20261021174233.78149-1-e.temirgaleev@tarantool.org> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD995934D660C27459BC4378B0D46D635BDC5B91136A2CC65D5182A05F5380850407031DAE6E18DE9F63DE06ABAFEAF6705E518D20D67385D8E4C29320517C55D9070ABB576495EA039 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70A10A23A3B64B805EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637AC83A81C8FD4AD23D82A6BABE6F325AC2E85FA5F3EDFCBAA7353EFBB553375667B1596EEF16A891D7402964496D2BF2EFB791A9DD61605F9985E2721F5D028D7389733CBF5DBD5E9D5E8D9A59859A8B6957A4DEDD2346B42CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8090A508E0FED6299176DF2183F8FC7C08B62274D4421ED34CD04E86FAF290E2DB606B96278B59C421DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B66291100AA869EC22089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A54959D08ACFEA5B5C5002B1117B3ED696CFB9AD7A17552726250A03108B67251B823CB91A9FED034534781492E4B8EEADCAFEFF123806BC82BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0AD73CAD6646DEDE1918E10F71CB4DF9F96AB70F9BE574AE9C625B6776AC983F447FC0B9F89525902EE6F57B2FD27647F25E66C117BDB76D6597D026EA481A783219112C2F9C602B0382F1E987BC199B55538EA43A520FDF338DEE11975A164471CB8341EE9D5BE9A0AC3BC95C2468FEC867497E6BF069D97FC28B7C8A4CF581EFAC7CEAA0681F5848F4C41F94D744909CEEF9E35A692215F79327F81E6EEE0AEB67A0F9F91F029B6AF33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXOcIETfglQORZ0zpDET4Zrk3igikrdHlWPyZOzpxCp+em5/lFOSIJYM= X-Mailru-Sender: 015BA6FC0EFA223A63A6570A1A031285B2022FB897106749B951B70A5BD4BD8E13A92F3942EE7494D1256FE8925F02CFE62B12C1FE0372A178274A4A9E9E44FD2A114EDA040D8818F01FF9DF31C397A372D6B4FCE48DF648AE208404248635DF 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: Evgeniy Temirgaleev via Tarantool-patches Reply-To: Evgeniy Temirgaleev Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for review! Fixes and a rewritten test (thanks for suggestion) was force pushed to the branch https://github.com/tarantool/luajit/tree/tmr_g/lj-noticket-improve-cli-sig-handling-2. The patch set v3 was send to the mailing list. On 10/26/26 3:59 PM, Sergey Bronnikov wrote: > 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/ Fixed. >> 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/ Fixed. >> 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 > Unfortunately, sourceware.org is not accessible for me. So, I think, widely accessible ftp.gnu.org is better. >> 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) > Many thanks! This scheme is much simpler than the waitpid() and a bash helper. >> 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. > Split into 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. Thanks! Added the notice about removing of the SA_RESTART flag by the patch. >> + 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. Added a note. >> + ['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. > Yes. I think the explicit enumeration is better. >> +}) >> + > It would be great to describe, at least briefly, what the test is > intended to do and what it checks. Added (for new test). >> +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() Fixed test_data(). >> +test:is(output, 'OK') > test description is missed Not actual: removed in new test. >> +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 Not actual: removed in new test. >> @@ -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. > Used suggested scheme with 'pipe' sync. >> +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)? Used suggested scheme with 'pipe' sync. >> + >> +# 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? Not actual: removed in new test. >> 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/ Fixed. >> +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 = ('