Repository navigation
Removing a signal handler from a process newListener event handler doesn't work correctly #51016
Description
Activity
Thanks for the report and the repro case!
I'm gonna work on a fix for this <3Reacted by Rafał RzepeckiI'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
SIGINTinprocessisfallbackHandler, not[fallbackHandler, realHandler](is not an array containing both handlers), at that point you have that state and you scheduled aremoveHandlerthat which will reach this pointwho 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 theLine 700 in 99f6084
realHandlerbut the scheduled task will delete all the listeners associated toSIGINT, to avoid that, you schedule the delete of thefallbackListeneron the next tick, that's why thesetInmediatemakes that work as expected, it has theeventshandler 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();
Thanks for opening this issue, I had a bit of fun!
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()
events["SIGINT"] = realHandler
Which should work as expected. The fact that I can't replicate the same issue with a plain
EventEmitterseems 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.
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):
Run
node sigTest.jsand 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?
Note the process shouldn't terminate. Adding the "real" handler should cause the "fallback" handler to be removed by the
newListenerevent listener, leaving only the "real" handler as the SIGINT listener.What do you see instead?
Note the process terminates with exit code 1. Neither SIGINT handler runs.
Additional information
Wrapping
process.offin asetImmediatemakes the code work as expected.