fix(ext/node): signal listeners added via process.once can now be removed#32606
fix(ext/node): signal listeners added via process.once can now be removed#32606bartlomieju merged 3 commits intomainfrom
process.once can now be removed#32606Conversation
…emoved When `process.once()` was used to add a signal listener, EventEmitter wrapped the handler in a once-wrapper before passing it to `Process.prototype.on`, which registered the wrapper with `Deno.addSignalListener`. However, `Process.prototype.off` passed the original unwrapped handler to `Deno.removeSignalListener`, which didn't match, so the Deno-level signal listener was never removed. Fix by looking up the actual registered listener (which may be a once-wrapper) before calling `Deno.removeSignalListener`. Closes #32603 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| function _findSignalListener( | ||
| // deno-lint-ignore no-explicit-any | ||
| target: any, | ||
| event: string, | ||
| // deno-lint-ignore no-explicit-any | ||
| listener: (...args: any[]) => void, | ||
| // deno-lint-ignore no-explicit-any | ||
| ): ((...args: any[]) => void) | undefined { |
There was a problem hiding this comment.
The TS here makes me sad :(
| // Look up the actual registered listener for a signal event. When `once()` is | ||
| // used, EventEmitter wraps the listener in a function with a `.listener` | ||
| // property. We need the wrapper to pass to `Deno.removeSignalListener`. |
There was a problem hiding this comment.
FYI all these gymnastics is because we handle signals using runtime API, Node does that by handling signals in Process object in native code. Something to address in a follow up cleanup.
Override `removeAllListeners` on Process prototype to call `Deno.removeSignalListener` for each registered signal handler before delegating to EventEmitter, preventing OS-level signal handler leaks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kajukitli
left a comment
There was a problem hiding this comment.
lgtm, good fix
the core issue is that EventEmitter.once() wraps the listener in a function that has .listener pointing to the original. when removing, you were passing the original to Deno.removeSignalListener but Deno has the wrapper registered.
the _findSignalListener lookup before removal is the right fix. also good that you handle removeAllListeners() for signal cleanup — that was an easy miss.
one edge case to consider: if someone calls process.off("SIGINT", someHandler) where someHandler was never registered, _findSignalListener returns undefined, and you call Deno.removeSignalListener with the original handler. this might throw or no-op depending on Deno's behavior. probably fine since that's user error, just noting it.
|
Re: the edge case with |
Summary
process.once("SIGINT", handler)followed byprocess.removeListener("SIGINT", handler)not actually removing the Deno-level signal listenerEventEmitter.once()wraps a handler,Process.prototype.offnow looks up the wrapper before passing it toDeno.removeSignalListeneronce,off,on, andprependOnceListenerremoval pathsCloses #32603
Test plan
process_once_remove_signal_listenerverifies listener count drops to 0 after removal for all registration methodsprocess.oncecannot be removed byprocess.removeListener#32603 no longer fires the handler after removal🤖 Generated with Claude Code