Removing a signal handler from a process newListener event handler doesn't work correctly

[dividedmind]

Version

v21.3.0

Platform

Linux new-hope 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1 (2023-10-07) x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Consider this code (the idea is to set up a fallback SIGINT handler that will auto-remove if something else sets a handler):

// sigTest.js
const fallbackHandler = () => console.debug("in fallback handler");

process.on("SIGINT", fallbackHandler);
process.on("newListener", (event) => event === "SIGINT" && process.off("SIGINT", fallbackHandler));

process.on("SIGINT", () => console.debug("in real handler"));

process.stdin.resume();

Run node sigTest.js and press Ctrl+C.

How often does it reproduce? Is there a required condition?

This happens regardless of whether the new listener is installed immediately or at a later point.

What is the expected behavior? Why is that the expected behavior?

$ node sigintTest.mjs
^Cin real handler
^Cin real handler

Note the process shouldn't terminate. Adding the "real" handler should cause the "fallback" handler to be removed by the newListener event listener, leaving only the "real" handler as the SIGINT listener.

What do you see instead?

$ node sigintTest.mjs ; printf "\nExit code: $?\n"
^C
Exit code: 1
$

Note the process terminates with exit code 1. Neither SIGINT handler runs.

Additional information

Wrapping process.off in a setImmediate makes the code work as expected.

[juanarbol]

Thanks for the report and the repro case!
I'm gonna work on a fix for this <3

[juanarbol]

I'm sorry to say that this is a race condition. The code itself is a bit confusing, and the problem is that you are setting the handlers for an event at the same time that you are unsettling them.

When you listen to the new handler, the events for SIGINT in process is fallbackHandler, not [fallbackHandler, realHandler] (is not an array containing both handlers), at that point you have that state and you scheduled a removeHandler that which will reach this point

node/lib/events.js

Line 700 in 99f6084

delete events[type];

who will schedule a delete to all the handlers (no matter what cuz' that was the state when you schedule that task, having just one handler, not both), then it will append the realHandler but the scheduled task will delete all the listeners associated to SIGINT, to avoid that, you schedule the delete of the fallbackListener on the next tick, that's why the setInmediate makes that work as expected, it has the events handler list object ready to be modified again.

I don't think Node.js needs to change the behavior for this specific case, it could break a lot of things over there; this behavior has been since Node.js v1.2.0 see #601, especially if your case can be easily fixed. Another working "equivalent" snippet is:

const fallbackHandler = () => console.debug("in fallback handler");

process.on("SIGINT", fallbackHandler);
process.on("newListener", (event) => event === "SIGINT" && process.nextTick(() => process.off("SIGINT", fallbackHandler)));

process.on("SIGINT", function realHandler () { console.debug("in real handler")});
process.stdin.resume();

[juanarbol]

Thanks for opening this issue, I had a bit of fun!

[dividedmind]

That implementation is a bit weird in how it switches between a list and a single value; I assume that's for efficiency? Anyway, I'm not sure that's what's happening here. Unless I'm missing something, the sequence seems to be (and the code looks perfectly sequential, with no race possible that I can see):

• addListener()
  • emit('newListener')
    • removeListener()
      • delete events["SIGINT"]
  • events["SIGINT"] = realHandler

Which should work as expected. The fact that I can't replicate the same issue with a plain EventEmitter seems to confirm it:

// evtest.js
const { EventEmitter } = require("node:events");

const ee = new EventEmitter();
const fallbackHandler = () => console.debug("in fallback handler");

ee.on("SIGINT", fallbackHandler);
ee.on("newListener", (event) => event === "SIGINT" && ee.off("SIGINT", fallbackHandler));

ee.on("SIGINT", () => console.debug("in real handler"));

process.on("SIGINT", () => ee.emit("SIGINT"));

process.stdin.resume();

$ node evtest.js
^Cin real handler

Something else seems to be going on here.