Fix regression in signal unlisten and document current EE behaviour #687
Conversation
This file contains 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
The order of the `newListener` and `removeListener` events with respect to the actual adding and removing from the underlying listeners array should be deterministic. There is no compelling reason for leaving it indeterminate. Changing the ordering is likely to result in breaking code that was unwittingly relying on the current behaviour, and the indeterminancy makes it impossible to use these events to determine when the first or last listener is added for an event.
When the last signal listener is removed, the signal wrap should be closed, restoring the default signal handling behaviour. This is done in a (patched) process.removeListener(). However, events.removeAllListeners has an optimization to avoid calling removeListener() if there are no listeners for the 'removeListener' event, introduced in 56668f5. That caused the following code to fail to terminate: process.stdin.resume(); function listener() {}; process.on('SIGINT', listener); process.removeAllListeners('SIGINT'); process.kill(process.pid, 'SIGINT') while the following will terminate: process.stdin.resume(); function listener() {}; process.on('SIGINT', listener); process.removeListener('SIGINT', listener); process.kill(process.pid, 'SIGINT') Replace the method patching with use of the 'newListener' and 'removeListener' events, which will fire no matter which methods are used to add or remove listeners.
sam-github
force-pushed
the
fix-regression-in-signal-unlisten
branch
from
90a5e2b to
09653fb
Compare
|
@bnoordhuis fixed the things you commented on, thanks, and rebased, PTAL |
bnoordhuis
pushed a commit
that referenced
this pull request
Feb 2, 2015
The order of the `newListener` and `removeListener` events with respect to the actual adding and removing from the underlying listeners array should be deterministic. There is no compelling reason for leaving it indeterminate. Changing the ordering is likely to result in breaking code that was unwittingly relying on the current behaviour, and the indeterminancy makes it impossible to use these events to determine when the first or last listener is added for an event. PR-URL: #687 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
bnoordhuis
pushed a commit
that referenced
this pull request
Feb 2, 2015
When the last signal listener is removed, the signal wrap should be closed, restoring the default signal handling behaviour. This is done in a (patched) process.removeListener(). However, events.removeAllListeners has an optimization to avoid calling removeListener() if there are no listeners for the 'removeListener' event, introduced in 56668f5. That caused the following code to fail to terminate: process.stdin.resume(); function listener() {}; process.on('SIGINT', listener); process.removeAllListeners('SIGINT'); process.kill(process.pid, 'SIGINT') while the following will terminate: process.stdin.resume(); function listener() {}; process.on('SIGINT', listener); process.removeListener('SIGINT', listener); process.kill(process.pid, 'SIGINT') Replace the method patching with use of the 'newListener' and 'removeListener' events, which will fire no matter which methods are used to add or remove listeners. PR-URL: #687 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
|
Thanks Sam, landed in 233e333...e9eb2ec. |
This was referenced Jun 23, 2023
This was referenced Jul 7, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
process.removeAllListeners()doesn't work for signals. It should, and it used to, prior to 56668f5I originally PRed a fix for this in nodejs/node-v0.x-archive#8953. That fix is a one-liner, wherein a subtle optimization in
EventEmitter.removeAllListeners()is evaded by a seeming no-opprocess.on('removeListener', function(){}), which is pretty awful from a comprehensibility point of view.src/node.jsis simpler and more understandable if it just uses the new/remove listener events, but they are theoretically useless for this kind of use-case, since the documentation is officially weasely about whether they fire before or after the listener is actually added. see: nodejs/node-v0.x-archive#8853 and piscisaureus@ff350f7I've never seen the new/remove listener used for any purpose other than what node itself could use it for: detecting whether there is indeed a listener for an event, and starting or stopping some processing accordingly. It should be easier to use the EE for this.
Even if you don't agree the indeterminancy in the EE API should be removed, node is free to depend on implementation details of itself, so 90a5e2b doesn't require c1d5fab. For node to assume that the EE behaves as it does seems better than my original fix/hack in sam-github@06b6e75
R=@bnoordhuis