lib, net, child_process, streams, dgram, readline, http2: Use addAbortListener where applicable#48550
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
Collaborator
nodejs-github-bot commented Jun 25, 2023
Review requested:
|
nodejs-github-bot added lib / src 
atlowChemi added child_process 
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
atlowChemiforce-pushed the use-kResistStopPropagation-where-applicable-2 branch from 90922af to c0dbb4bCompare This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
ronag reviewed Jun 26, 2023
Member
ronag left a comment
ronag left a comment There was a problem hiding this comment.
@benjamingr This is... sad... I'm starting to feel that adding support for abort signals might not have been the optimal decision.
aduh95 added the commit-queue-rebase 
Member
benjamingr commented Jun 26, 2023
It was not, I (and others) totally missed this was unsafe and didn't understand how hard leaks would be. We also failed to appreciate how much the web would deviate in priorities (they're not really interested in fixing this, they like signal.reason which explodes our API surface etc). These may be fixable in the spec but I doubt there is implementor interest. We should provide a utility for memory safe registration that is guaranteed to trigger and deprecate AbortSignal support for everything that is a resource (like http.Server, streams etc) in favor of Symbol.asyncDispose and Symbol.dispose now that they exist. Note in an ideal where everything that needs to support a signal does - users won't run into this since they just pass signals down. |
benjamingr approved these changes Jun 26, 2023
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Jun 26, 2023
atlowChemi added request-ci github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Jun 26, 2023
24 tasks
Collaborator
nodejs-github-bot commented Jun 27, 2023
36 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Member
ruyadorno commented Sep 11, 2023
This commit does not land cleanly on |
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Merged
MemberAuthor
atlowChemi commented Sep 11, 2023
@ruyadorno is this still relevant? seems like the commits have been added to the proposal? |
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
PR-URL: #48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
H4ad pushed a commit to H4ad/node that referenced this pull request Sep 17, 2023
PR-URL: nodejs#48550 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
8 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.
Refs: #48301, #48521, #48596