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

stream: add AbortSignal support to promisified pipeline by Linkgoron · Pull Request #37359 · nodejs/node · GitHub

/ node Public

stream: add AbortSignal support to promisified pipeline - #37359

Closed
Linkgoron wants to merge 1 commit into
nodejs:masterfrom
Linkgoron:stream-pipeline-abort-signal
ConversationCommits (1)ChecksFiles changed
Closed

stream: add AbortSignal support to promisified pipeline#37359
Linkgoron wants to merge 1 commit into
nodejs:masterfrom
Linkgoron:stream-pipeline-abort-signal

Conversation

Linkgoron commented Feb 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR adds support for AbortSignal to promisified pipeline (in stream/promises)

This resolves #37321.

As an aside, this wouldn't be hard to actually push it into pipeline itself and support it in the regular version, however it's already pretty simple to just call addAbortSignal from stream on the received pipeline.

  • 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

Linkgoron mentioned this pull request Feb 13, 2021
Closed
Linkgoron changed the title stream: add AbortSignal to promisified pipeline stream: add AbortSignal support to promisified pipeline Feb 13, 2021
benjamingr reviewed Feb 13, 2021
View reviewed changes
Comment thread test/parallel/test-stream-pipeline.js Outdated
Linkgoron force-pushed the stream-pipeline-abort-signal branch from 54d0ed7 to 0100a60 Compare February 13, 2021 20:37

benjamingr commented Feb 13, 2021

Copy link
Copy Markdown
Member

@ronag

ronag commented Feb 13, 2021

Copy link
Copy Markdown
Member

@benjamingr: Isn't the usual pattern to send an options object i.e. { signal }?

ronag requested changes Feb 13, 2021
View reviewed changes

ronag 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

needs docs?

benjamingr commented Feb 14, 2021

Copy link
Copy Markdown
Member

Yeah I tend to prefer options with a signal too

stream: add AbortSignal to promisified pipeline
dc74b35 
add support for AbortSignal to promisified pipeline.

Resolves: nodejs#37321
Linkgoron force-pushed the stream-pipeline-abort-signal branch from 0100a60 to dc74b35 Compare February 14, 2021 11:38

Linkgoron commented Feb 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've updated the docs, and also changed it to receive an options parameter, in the promise version (and added some checks, to make sure that the options object is really an options object).

@ronag I could also "push" the changes from the promise version, into the regular pipeline implementation, so that both the "regular" pipeline and promisified pipeline support cancellation. where the API will change to:
stream.pipeline(source[, ...transforms], destination[,options], callback)
stream.pipeline(streams[,options], callback)

Similarly to what was done in readline.question, a few weeks ago.

nodejs-github-bot commented Feb 18, 2021

Copy link
Copy Markdown
Collaborator

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

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

benjamingr commented Feb 18, 2021

Copy link
Copy Markdown
Member

Landed in 38f6e5a 🎉

benjamingr closed this Feb 18, 2021
benjamingr pushed a commit that referenced this pull request Feb 18, 2021
stream: add AbortSignal to promisified pipeline
38f6e5a 
add support for AbortSignal to promisified pipeline.

Resolves: #37321

PR-URL: #37359
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Linkgoron deleted the stream-pipeline-abort-signal branch February 18, 2021 16:05
RaisinTen mentioned this pull request Feb 25, 2021
Merged
targos pushed a commit that referenced this pull request Feb 28, 2021
stream: add AbortSignal to promisified pipeline
429dffd 
add support for AbortSignal to promisified pipeline.

Resolves: #37321

PR-URL: #37359
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
targos mentioned this pull request Mar 2, 2021
Merged
targos added backport-blocked-v14.x semver-minor PRs that contain new features and should be released in the next minor version. labels May 27, 2021
darksabrefr mentioned this pull request Sep 23, 2021
Closed
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

benjamingr benjamingr approved these changes
ronag ronag approved these changes
Lxxyx Lxxyx approved these changes
marsonya marsonya 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.

promisified pipeline missing AbortSignal support

7 participants

Back | FazBrowse Home | New Git URL