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

streams: pipeline with signal #39067

Closed
wants to merge 3 commits into from
Closed

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 17, 2021

Generators in pipeline must be able to be aborted or pipeline
can deadlock.

@github-actions github-actions bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Jun 17, 2021
@ronag ronag force-pushed the pipeline-signal branch 2 times, most recently from d9494c9 to 8578ba0 Compare June 18, 2021 05:37
doc/api/stream.md Outdated Show resolved Hide resolved
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

@targos
Copy link
Member

targos commented Jun 18, 2021

Can you please improve the commit message title? It should start with an imperative verb.

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

@ronag
Copy link
Member Author

ronag commented Jun 18, 2021

@nodejs/streams

Copy link
Member

@benjamingr benjamingr 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 sure this makes sense and I'll need a few days to think about this API (Sorry!).

The thing is - generators already have a way to signal cancellation (through calling .return on them which is runs finally blocks).

This is kind of scary since it behaves differently for example from doing an addAbortSignal on a Readable.from which will call .return on the generator instead of passing a signal argument.

I think Readable.from(generator) and generator should behave relatively similarly when passed to pipeline?

Is the right thing to pass an abort signal to generators passed to from and other places that accept async iterables and abort the related controller when the stream is destroyed?

@benjamingr
Copy link
Member

In any case I feel strongly that async generators should behave similarly in terms of AbortSignal in all of our APIs.

@ronag
Copy link
Member Author

ronag commented Jun 19, 2021

I’m fine with that but we need a way to abort calls inside a async generator.

@benjamingr
Copy link
Member

And would that replace .return being called on the generator so we'll no longer be calling .return on generators when doing Readable.from (for instance) but instead passing in and aborting a signal?

@benjamingr
Copy link
Member

(You can already abort it by calling .return (or .destroy after Readable.from which will run finally blocks - it will also abort sub-actions if they are generators as well and using yield* but that's quirky and users would find it surprising so I lean towards passing AbortSignal and avoiding .return altogether)

@ronag
Copy link
Member Author

ronag commented Jun 23, 2021

@benjamingr I'm unsure how .return() would help with the following case...

{
  const ac = new AbortController();
  const signal = ac.signal;
  pipelinep(
    async function * (signal) {
      await tsp.setTimeout(1e6, signal); // How to abort without passing signal?
    },
    async function(source) {

    },
    { signal }
  ).catch(common.mustCall((err) => {
    assert.strictEqual(err.name, 'AbortError');
  }));
  ac.abort();
}

@ronag
Copy link
Member Author

ronag commented Jun 23, 2021

@benjamingr have you given this PR any more thought?

@ronag ronag requested a review from benjamingr June 23, 2021 12:54
@benjamingr
Copy link
Member

Yes I think this should land if and only if we make Readable.from (and other APIs that take an async iterator) take a signal - otherwise we’re left with an inconsistent API - if you’re willing to do that (or wait a month for me I’ll happily contribute the code once I’m back from paternity leave) this sounds good to me.

@ronag
Copy link
Member Author

ronag commented Jun 29, 2021

if we make Readable.from (and other APIs that take an async iterator) take a signal - otherwise we’re left with an inconsistent API

I'm not sure what all those API's are and how you imagine it should look so I guess I'll wait and leave it to you.

@ronag
Copy link
Member Author

ronag commented Jun 29, 2021

I'm a little unsure if the signature should be (source, signal) or (source, { signal }).

@ronag ronag added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 1, 2021
@benjamingr
Copy link
Member

The latter if we follow the DOM's API design guidelines

@ronag ronag requested a review from benjamingr August 23, 2021 16:42
@ronag
Copy link
Member Author

ronag commented Aug 23, 2021

@nodejs/streams

@nodejs-github-bot
Copy link
Collaborator

Generators in pipeline must be able to be aborted or pipeline
can deadlock.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Aug 24, 2021
@ronag ronag requested a review from lpinca August 24, 2021 13:43
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 24, 2021

@ronag ronag added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 25, 2021
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

ronag added a commit that referenced this pull request Aug 25, 2021
Generators in pipeline must be able to be aborted or pipeline
can deadlock.

PR-URL: #39067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ronag
Copy link
Member Author

ronag commented Aug 25, 2021

Landed in c04d621

@ronag ronag closed this Aug 25, 2021
targos pushed a commit that referenced this pull request Sep 7, 2021
Generators in pipeline must be able to be aborted or pipeline
can deadlock.

PR-URL: #39067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Sep 10, 2021
Generators in pipeline must be able to be aborted or pipeline
can deadlock.

PR-URL: #39067
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs added a commit that referenced this pull request Sep 21, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927
doc:
  * add Ayase-252 to collaborators (Qingyu Deng) #40078
fs:
  * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013
http:
  * (SEMVER-MINOR) limit requests per connection (Artur K) #40082
src:
  * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754
  * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926
stream:
  * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067

PR-URL: TODO
BethGriggs added a commit that referenced this pull request Sep 21, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927
doc:
  * add Ayase-252 to collaborators (Qingyu Deng) #40078
fs:
  * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013
http:
  * (SEMVER-MINOR) limit requests per connection (Artur K) #40082
src:
  * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754
  * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926
stream:
  * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067

PR-URL: TODO
@BethGriggs BethGriggs mentioned this pull request Sep 21, 2021
1 task
BethGriggs added a commit that referenced this pull request Sep 22, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927
doc:
  * add Ayase-252 to collaborators (Qingyu Deng) #40078
fs:
  * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013
http:
  * (SEMVER-MINOR) limit requests per connection (Artur K) #40082
src:
  * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754
  * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926
stream:
  * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067

PR-URL: #40175
BethGriggs added a commit that referenced this pull request Sep 22, 2021
Notable changes:

crypto:
  * (SEMVER-MINOR) add rsa-pss keygen parameters (Filip Skokan) #39927
doc:
  * add Ayase-252 to collaborators (Qingyu Deng) #40078
fs:
  * (SEMVER-MINOR) make `open` and `close` stream override optional when unused (Antoine du Hamel) #40013
http:
  * (SEMVER-MINOR) limit requests per connection (Artur K) #40082
src:
  * (SEMVER-MINOR) add --no-global-search-paths cli option (Cheng Zhao) #39754
  * (SEMVER-MINOR) add option to disable global search paths (Cheng Zhao) #39754
  * (SEMVER-MINOR) make napi_create_reference accept symbol (JckXia) #39926
stream:
  * (SEMVER-MINOR) add signal support to pipeline generators (Robert Nagy) #39067

PR-URL: #40175
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. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants