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.

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

stream: add AbortSignal support to promisified pipeline #37359

Closed

Conversation

Linkgoron
Copy link
Member

@Linkgoron Linkgoron commented Feb 13, 2021

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 Linkgoron changed the title stream: add AbortSignal to promisified pipeline stream: add AbortSignal support to promisified pipeline Feb 13, 2021
@benjamingr
Copy link
Member

@ronag

@ronag
Copy link
Member

ronag commented Feb 13, 2021

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

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

needs docs?

@benjamingr
Copy link
Member

Yeah I tend to prefer options with a signal too

add support for AbortSignal to promisified pipeline.

Resolves: nodejs#37321
@Linkgoron
Copy link
Member Author

Linkgoron commented Feb 14, 2021

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

@Lxxyx Lxxyx added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 18, 2021
@benjamingr
Copy link
Member

Landed in 38f6e5a 🎉

@benjamingr benjamingr closed this Feb 18, 2021
benjamingr pushed a commit that referenced this pull request Feb 18, 2021
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 Linkgoron deleted the stream-pipeline-abort-signal branch February 18, 2021 16:05
targos pushed a commit that referenced this pull request Feb 28, 2021
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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

promisified pipeline missing AbortSignal support
7 participants