SIGTERM handler does not run if there are no tasks running and exit code is always 0

[TrevorSundberg]

• Version: v11.15.0
• Platform: Linux tsundberg-dev 4.18.0-20-generic add issue contributing section #21~18.04.1-Ubuntu SMP Wed May 8 08:43:37 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: process

---

To run the following tests paste them into index.js and run node index.js; echo $?

---

Case 1

The following results in an expected output:

process.kill(process.pid, 'SIGTERM')

Output:

Terminated
143

---

Case 2

Adding a SIGTERM results in an exit code of 0, and the handler never runs:

process.on('SIGTERM', () => {
    console.error('Handle ran!')
    process.exit(1)
})

process.kill(process.pid, 'SIGTERM')

Output:

0

---

Case 3

Adding a timeout or some other task that keeps node alive results in the expected behavior:

process.on('SIGTERM', () => {
    console.error('Handle ran!')
    process.exit(1)
})

process.kill(process.pid, 'SIGTERM')

setTimeout(() => {}, 100000)

Output:

Handle ran!
1

[mscdex]

Signals are processed asynchronously, so if there is nothing keeping the event loop alive (to be able to process any signals), the process will exit naturally.

[addaleax]

I feel like an activated libuv signal handler should keep the process alive until its callback has been called.

@nodejs/libuv

[TrevorSundberg]

@mscdex I would then expect case 1 to "exit naturally" with exit code 0, or for case 2 to exit with 143.

[mscdex]

I would then expect case 1 to "exit naturally" with exit code 0

From the process documentation:

'SIGTERM' and 'SIGINT' have default handlers on non-Windows platforms that reset the terminal mode before exiting with code 128 + signal number.

These default handlers are not something that node sets up, so it makes sense that it exits the way it does.

or for case 2 to exit with 143

Following the process documentation text quoted above:

If one of these signals has a listener installed, its default behavior will be removed

and again because the userland signal handlers are invoked asynchronously, the process exits naturally because there isn't anything keeping the event loop alive. As you found, if the event loop is kept alive at least for the next tick, it has a chance to execute the registered JS signal handler.

[addaleax]

@mscdex I think the issue is not a lack of understanding of why this happens, but rather a statement that a triggered event handler should keep the loop alive. I think this is a real bug.

[bnoordhuis]

@addaleax It's not a libuv issue. Its signal handler records the event but Node.js closes the uv_signal_t handle before libuv gets a chance to deliver it. My debugger tells me that's coming from node::Environment::RunCleanup().

One way around that is to ref at least one signal wrap (they're unref'd when created, see below) and call uv_run(loop, UV_RUN_NOWAIT) at pre-exit.

node/lib/internal/process/main_thread_only.js

Lines 134 to 141 in 83c1e17

	const wrap = new Signal();
	wrap.unref();
	wrap.onsignal = process.emit.bind(process, type, type);
	const signum = signals[type];
	const err = wrap.start(signum);

---

Libuv could turn UV_RUN_DEFAULT or UV_RUN_ONCE into UV_RUN_NOWAIT when there are no active handles (as is the case here) in order to poll for I/O at least once, see diff below.

It makes test case 2 work but a change like that needs more discussion because it might have has other effects too. If you think it's worth pursuing, please open a libuv issue.

diff --git a/deps/uv/src/unix/core.c b/deps/uv/src/unix/core.c
index 202c75bbb5..2d15a9349b 100644
--- a/deps/uv/src/unix/core.c
+++ b/deps/uv/src/unix/core.c
@@ -351,8 +351,11 @@ int uv_run(uv_loop_t* loop, uv_run_mode mode) {
   int ran_pending;
 
   r = uv__loop_alive(loop);
-  if (!r)
+  if (r == 0) {
     uv__update_time(loop);
+    mode = UV_RUN_NOWAIT;
+    r = 1;
+  }
 
   while (r != 0 && loop->stop_flag == 0) {
     uv__update_time(loop);

[bnoordhuis]

I opened libuv/libuv#2370 for discussion.

There's another way to address this in Node.js, by creating an uv_idle_t at startup to keep the event loop going for at least one tick:

diff --git a/src/env.cc b/src/env.cc
index df59eaaa94..73538a2c26 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -478,6 +478,18 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
   thread_stopper()->set_stopped(false);
   uv_unref(reinterpret_cast<uv_handle_t*>(thread_stopper()->GetHandle()));
 
+  {
+    auto handle = new uv_idle_t();
+    auto idle_cb = [](uv_idle_t* handle) {
+      auto close_cb = [](uv_handle_t* handle) {
+        delete reinterpret_cast<uv_idle_t*>(handle);
+      };
+      uv_close(reinterpret_cast<uv_handle_t*>(handle), close_cb);
+    };
+    CHECK_EQ(0, uv_idle_init(event_loop(), handle));
+    CHECK_EQ(0, uv_idle_start(handle, idle_cb));
+  }
+
   // Register clean-up cb to be called to clean up the handles
   // when the environment is freed, note that they are not cleaned in
   // the one environment per process setup, but will be called in

It does make test/parallel/test-timers-immediate-unref-simple.js fail because it tests exactly the condition that the patch circumvents.

[addaleax]

There's another way to address this in Node.js, by creating an uv_idle_t at startup to keep the event loop going for at least one tick:

@bnoordhuis I don’t think that solves this problem – that patch only solves the issue when it’s coming from code executed synchronously after bootstrap.

The kind of solution I’d imagine is setting a flag on the loop from the signal handler, and checking that before letting the loop exit? Is that feasible? It doesn’t solve the issue when the signal is coming from another process, but that’s a race condition anyway…

[bnoordhuis]

setting a flag on the loop from the signal handler, and checking that before letting the loop exit

How would the "checking" part work?

Special-casing signal handles was one of the options I suggested in libuv/libuv#2370 but the (unanimous) consensus was to keep the status quo.

[jasnell]

There's been no further discussion or follow up on this issue in many years. The reported behavior still exists. Is there anything further that can be done here or maybe we just document the issue and move on?

[soreavis]

@jasnell — documenting it looks right; there's no consensus for a behavioral change (libuv/libuv#2370 kept the status quo, the uv_idle_t patch bnoordhuis floated fails test-timers-immediate-unref-simple.js, and addaleax's loop-flag idea was never pursued).

The inaccuracy is in the doc: process.md says a SIGTERM/SIGINT listener means "its default behavior will be removed (Node.js will no longer exit)", but the listener is invoked asynchronously — with an otherwise-empty event loop the process still exits with code 0 and the listener never runs, which is what this issue reported. I can send a docs-only PR.