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

promisified pipeline missing AbortSignal support #37321

Closed
ronag opened this issue Feb 11, 2021 · 4 comments
Closed

promisified pipeline missing AbortSignal support #37321

ronag opened this issue Feb 11, 2021 · 4 comments
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Feb 11, 2021

No description provided.

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Feb 11, 2021
@ronag
Copy link
Member Author

ronag commented Feb 11, 2021

@benjamingr FYI

@benjamingr
Copy link
Member

I can fix this in a week or so. @Linkgoron want to take a stab at this?

@Linkgoron
Copy link
Member

Linkgoron commented Feb 11, 2021 •
edited
Loading

I can fix this in a week or so. @Linkgoron want to take a stab at this?

I'll give it a shot. It currently essentially receives (...streams, callback) does it make sense to check if the last parameter is an AbortSignal (i.e. after the callback)?

What about promisified stream.finished? Does it make sense to add AbortSignal support to it? In theory, you'd expect the consumed stream itself to have cancellation support, but I can see how someone might want to abort waiting.

Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 13, 2021
stream: add AbortSignal to promisified pipeline
e74b4dd 
add support for AbortSignal to promisified pipeline.

Resolves: nodejs#37321
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 13, 2021
stream: add AbortSignal to promisified pipeline
54d0ed7 
add support for AbortSignal to promisified pipeline.

Resolves: nodejs#37321
@Linkgoron Linkgoron mentioned this issue Feb 13, 2021
Closed
4 tasks
@Linkgoron
Copy link
Member

created PR #37359

Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 13, 2021
stream: add AbortSignal to promisified pipeline
0100a60 
add support for AbortSignal to promisified pipeline.

Resolves: nodejs#37321
Linkgoron pushed a commit to Linkgoron/node that referenced this issue Feb 14, 2021
stream: add AbortSignal to promisified pipeline
dc74b35 
add support for AbortSignal to promisified pipeline.

Resolves: nodejs#37321
targos pushed a commit that referenced this issue Feb 28, 2021
@targos
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

stream: add AbortSignal support to promisified pipeline Linkgoron/node
3 participants
@Linkgoron @benjamingr @ronag