FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: add AbortSignal support to finished by Linkgoron · Pull Request #37354 · nodejs/node · GitHub

/ node Public

stream: add AbortSignal support to finished - #37354

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
Linkgoron:stream-finished-abort-signal
Feb 26, 2021
ConversationCommits (1)ChecksFiles changed
Merged

stream: add AbortSignal support to finished#37354
aduh95 merged 1 commit into
nodejs:masterfrom
Linkgoron:stream-finished-abort-signal

Conversation

Linkgoron commented Feb 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Add AbortSignal support to stream.finished
This PR adds support for AbortSignal in stream.finished (eos).

Originally, I thought about adding it to the promisified version only, however I think that it could be useful to both.

I've implemented it so that if the stream is already closed, and the AbortSignal is also aborted, the closed gets prioritised (no error is emitted).

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

aduh95 reviewed Feb 13, 2021
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you add an entry to the changes YAML list in the docs please?

  - version: REPLACEME
    pr-url: https://github.com/nodejs/node/pull/37354
    description: The `signal` option was added.

Comment thread lib/internal/streams/end-of-stream.js Outdated
Linkgoron force-pushed the stream-finished-abort-signal branch from b330729 to caa5a1b Compare February 13, 2021 14:57
aduh95 reviewed Feb 13, 2021
View reviewed changes
Comment thread lib/internal/streams/end-of-stream.js Outdated
Comment thread test/parallel/test-stream-finished.js Outdated
Comment thread test/parallel/test-stream-finished.js Outdated
Comment thread lib/internal/streams/end-of-stream.js Outdated
Comment thread test/parallel/test-stream-finished.js Outdated
Linkgoron force-pushed the stream-finished-abort-signal branch from caa5a1b to 3ca500a Compare February 13, 2021 17:43

benjamingr commented Feb 14, 2021

Copy link
Copy Markdown
Member

@nodejs/streams

mcollina requested changes Feb 15, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not convinced by this change - I might expect for the stream to be destroyed on abort. What's the rationale for not doing so?

Linkgoron commented Feb 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

IMO finished is more of a passive listener than a stream controller. It doesn't really do anything except add listeners on the underlying stream (It's also in the description A function to get notified when a stream is no longer readable, writable or has experienced an error or a premature close event). The cleanup function that (already) returns from finished also doesn't destroy the underlying stream.

However, maybe aborting should invoke the cleanup function (which it currently doesn't).

mcollina commented Feb 17, 2021

Copy link
Copy Markdown
Member

If finished does not destroy the stream, then pipeline will?

ronag commented Feb 17, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

If finished does not destroy the stream, then pipeline will?

Yes. The way I see it pipeline has "non recoverable side effects" on the stream and should destroy while finished has no side effects and should not destroy.

Linkgoron force-pushed the stream-finished-abort-signal branch from 3ca500a to e027154 Compare February 17, 2021 15:58
mcollina requested changes Feb 17, 2021
View reviewed changes
Comment thread lib/internal/streams/end-of-stream.js Outdated
Linkgoron force-pushed the stream-finished-abort-signal branch 2 times, most recently from 44bd8c6 to 7938420 Compare February 17, 2021 20:11
mcollina approved these changes Feb 18, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

nodejs-github-bot commented Feb 18, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36202/

nodejs-github-bot commented Feb 18, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36222/

nodejs-github-bot commented Feb 18, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36227/

Linkgoron commented Feb 25, 2021

Copy link
Copy Markdown
Contributor Author

Is there anything else that I need to do here to get this merged?

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 25, 2021

nodejs-github-bot commented Feb 26, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36364/

nodejs-github-bot commented Feb 26, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36366/

stream: add AbortSignal support to finished
7837d3f 
PR-URL: nodejs#37354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95 force-pushed the stream-finished-abort-signal branch from 7938420 to 7837d3f Compare February 26, 2021 10:21
aduh95 merged commit 7837d3f into nodejs:master Feb 26, 2021

aduh95 commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

Landed in 7837d3f

Linkgoron deleted the stream-finished-abort-signal branch February 26, 2021 14:28
targos pushed a commit that referenced this pull request Feb 28, 2021
stream: add AbortSignal support to finished
3d3df0c 
PR-URL: #37354
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos mentioned this pull request Mar 2, 2021
Merged
targos added the semver-minor PRs that contain new features and should be released in the next minor version. label May 27, 2021
debadree25 mentioned this pull request Jan 29, 2023
Merged
nodejs-github-bot pushed a commit that referenced this pull request Feb 2, 2023
stream: add suport for abort signal in finished() for webstreams
ebcc711 
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
stream: add suport for abort signal in finished() for webstreams
4cf4b41 
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
stream: add suport for abort signal in finished() for webstreams
f141b73 
Refs: nodejs#46205
PR-URL: nodejs#46403
Refs: nodejs#37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
stream: add suport for abort signal in finished() for webstreams
91a550e 
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
ronag ronag approved these changes
aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL