Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stream: add AbortSignal support to finished #37354

Merged
merged 1 commit into from
Feb 26, 2021
Merged

stream: add AbortSignal support to finished #37354

merged 1 commit into from
Feb 26, 2021

Conversation

Linkgoron
Copy link
Member

@Linkgoron Linkgoron commented Feb 13, 2021 •
edited
Loading

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
aduh95 reviewed Feb 13, 2021
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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.

lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved
aduh95
aduh95 reviewed Feb 13, 2021
View reviewed changes
lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved
test/parallel/test-stream-finished.js Outdated Show resolved Hide resolved
test/parallel/test-stream-finished.js Outdated Show resolved Hide resolved
lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved
test/parallel/test-stream-finished.js Outdated Show resolved Hide resolved
@benjamingr
Copy link
Member

@nodejs/streams

mcollina
mcollina requested changes Feb 15, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Linkgoron commented Feb 15, 2021 •
edited
Loading

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
Copy link
Member

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

@ronag
Copy link
Member

ronag commented Feb 17, 2021 •
edited
Loading

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.

mcollina
mcollina requested changes Feb 17, 2021
View reviewed changes
lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved
@Linkgoron Linkgoron force-pushed the stream-finished-abort-signal branch 2 times, most recently from 44bd8c6 to 7938420 Compare February 17, 2021 20:11
mcollina
mcollina approved these changes Feb 18, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Linkgoron
Copy link
Member Author

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

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 25, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
stream: add AbortSignal support to finished
7837d3f 
PR-URL: nodejs#37354
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95 aduh95 force-pushed the stream-finished-abort-signal branch from 7938420 to 7837d3f Compare February 26, 2021 10:21
@aduh95 aduh95 merged commit 7837d3f into nodejs:master Feb 26, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 26, 2021

Landed in 7837d3f

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

@aduh95 aduh95 aduh95 approved these changes

@ronag ronag ronag approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@Linkgoron @benjamingr @mcollina @ronag @nodejs-github-bot @aduh95 @jasnell @targos