fix(ext/node): pass signal name to process signal handlers#33425
Merged
Conversation
Node.js calls signal listeners with the signal name as the first argument (e.g. "SIGINT"), but Deno.addSignalListener calls them with no arguments. Wrap signal listeners registered via process.on() to pass the signal name, matching Node.js behavior. Closes #33379
test-signal-args.js relies on setImmediate keeping the event loop alive for signal delivery, which does not work in Deno (signals need a real async tick like setTimeout). Replace with a unit test using setTimeout.
fibibot
approved these changes
Apr 25, 2026
fibibot
left a comment
fibibot
left a comment
Contributor
There was a problem hiding this comment.
LGTM. The wrap/unwrap structure threads through both flows cleanly:
- Direct
process.on("SIGINT", handler):_wrapSignalListenerlazily allocatesW = () => handler("SIGINT")keyed on(handler, event)in the outerWeakMap+ innerMap, and registersWwithDeno.addSignalListener.process.off("SIGINT", handler)finds nothing via_findSignalListener(sincehandleris its own list entry), then_unwrapSignalListenerlookshandlerup, returnsW, and deletes the inner-map entry — so the correspondingDeno.removeSignalListener(W)lines up. process.once("SIGINT", handler): EventEmitter wraps aswrapped(with.listener = handler). The wrap path stores the entry underwrappedin the WeakMap. Whenwrappedfires and re-enters viaprocess.removeListener("SIGINT", wrapFn),_findSignalListenerreturnswrapped(matched via.listener === handler), and_unwrapSignalListener("SIGINT", wrapped)recovers the sameWthat was registered, so the Deno-side listener is correctly cleaned up.- WeakMap-keyed-on-listener means once the user lets go of the handler, the wrappers are GCd automatically;
wrappersByEvent.delete(event)prevents reuse but keeps the inner map so a re-onafteroffallocates a fresh wrapper rather than re-using a stale one.
Unit test exercises the SIGUSR1 → received: ["SIGUSR1"] happy path. CI fully green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #33379
Node.js calls signal listeners with the signal name as the first argument (e.g.
"SIGINT"), butDeno.addSignalListenercalls them with no arguments. This causedprocess.on('SIGINT', (signal) => ...)to receiveundefinedinstead of the signal name.Fix: wrap signal listeners registered via
process.on()/process.prependListener()to pass the signal name. A WeakMap tracks the mapping soprocess.off()/process.removeListener()can remove the correct wrapped listener from Deno's signal system.Test plan
Before:
After: