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: support AbortSignal in constructor #36431

Closed

Conversation

benjamingr
Copy link
Member

As requested by @ronag (I agree it's useful especially when subclassing streams) - an API to pass AbortSignal directly to the readable/writeable constructor + tests.

It uses addAbortSignal internally to make maintenance easier.

Checklist
  • 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

@benjamingr benjamingr requested a review from ronag December 7, 2020 16:44
@benjamingr benjamingr added the stream Issues and PRs related to the stream subsystem. label Dec 7, 2020
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.

Won't it be added twice in case of duplex?

@benjamingr
Copy link
Member Author

Won't it be added twice in case of duplex?

Maybe? Probably not because destroy already checks that stream is destroyed and does not call _destroy.

  • I'm happy to add a test for that if you think we should have one.
  • I'm also happy to make that flow and anything else you don't like in the PR more explicit if you think that improves code quality.

@ronag
Copy link
Member

ronag commented Dec 7, 2020

Maybe? Probably not because destroy already checks that stream is destroyed and does not call _destroy.

I'm not sure I follow. How is destroy related? This happens in the constructor which is invoked for both Readable and Writable.

@benjamingr
Copy link
Member Author

I'm not sure I follow. How is destroy related? This happens in the constructor which is invoked for both Readable and Writable.

destroy would be called twice on the stream (since the signal has two listeners - one for the readable and one for the writeable).

@ronag
Copy link
Member

ronag commented Dec 7, 2020

destroy would be called twice on the stream (since the signal has two listeners - one for the readable and one for the writeable).

I was more thinking about addAbortSignal being called twice through constructor.

@benjamingr
Copy link
Member Author

I was more thinking about addAbortSignal being called twice through constructor.

Yes, that's probably fine, the signal is added twice, destroys the stream twice and the second destroy is ignored.

@benjamingr
Copy link
Member Author

I added a test for duplex that makes sure error is called exactly once and close is called to illustrate that point e71ae8f#diff-9772e10e4bd234c2a0d41c8043e02338add0b46578d4782ffc02bcb27cfbb85cR242-R256

@ronag
Copy link
Member

ronag commented Dec 7, 2020

Can't we just make use of isDuplex and add it once?

@benjamingr
Copy link
Member Author

benjamingr commented Dec 7, 2020

Can't we just make use of isDuplex and add it once?

We can, would you prefer that?

Edit: I think I prefer that too. Pushed a fix + the "ignore bad signal" logic for the construct version

@benjamingr benjamingr force-pushed the abort-signal-stream-constructor branch from 4579e20 to 03ff368 Compare December 7, 2020 18:29
});
let count = 0;
duplex.on('error', common.mustCall((e) => {
assert.strictEqual(count++, 0); // Ensure not called twice
Copy link
Member

Choose a reason for hiding this comment

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

The mustCall already handles this doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell I want to make sure error is called, but not called twice - I'd need a comon.mustCallOnce or something.

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

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2020
@nodejs-github-bot
Copy link
Collaborator

@benjamingr benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 10, 2020
@nodejs-github-bot
Copy link
Collaborator

benjamingr added a commit that referenced this pull request Dec 10, 2020
PR-URL: #36431
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@benjamingr
Copy link
Member Author

Landed with NCU in 040a27a 🎉

@benjamingr benjamingr closed this Dec 10, 2020
@benjamingr benjamingr deleted the abort-signal-stream-constructor branch December 10, 2020 21:42
targos pushed a commit that referenced this pull request Dec 21, 2020
PR-URL: #36431
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit that referenced this pull request Dec 22, 2020
Notable changes:

    child_process:
      * (SEMVER-MINOR) add signal support to spawn (Benjamin Gruenbaum) #36432
    doc:
      * add PoojaDurgad to collaborators (Pooja D P) #36511
    lib:
      * (SEMVER-MINOR) support BigInt in querystring.stringify (raisinten) #36499
    src:
      * (SEMVER-MINOR) add way to get IsolateData and allocator from Environment (Anna Henningsen) #36441
      * (SEMVER-MINOR) allow preventing SetPrepareStackTraceCallback (Shelley Vohr) #36447
    stream:
      * (SEMVER-MINOR) support abortsignal in constructor (Benjamin Gruenbaum) #36431

PR-URL: #36597
targos added a commit that referenced this pull request Dec 22, 2020
Notable changes:

    child_process:
      * (SEMVER-MINOR) add signal support to spawn (Benjamin Gruenbaum) #36432
    doc:
      * add PoojaDurgad to collaborators (Pooja D P) #36511
    lib:
      * (SEMVER-MINOR) support BigInt in querystring.stringify (raisinten) #36499
    src:
      * (SEMVER-MINOR) add way to get IsolateData and allocator from Environment (Anna Henningsen) #36441
      * (SEMVER-MINOR) allow preventing SetPrepareStackTraceCallback (Shelley Vohr) #36447
    stream:
      * (SEMVER-MINOR) support abortsignal in constructor (Benjamin Gruenbaum) #36431

PR-URL: #36597
targos added a commit that referenced this pull request Dec 22, 2020
Notable changes:

    child_process:
      * (SEMVER-MINOR) add signal support to spawn (Benjamin Gruenbaum) #36432
    doc:
      * add PoojaDurgad to collaborators (Pooja D P) #36511
    lib:
      * (SEMVER-MINOR) support BigInt in querystring.stringify (raisinten) #36499
    src:
      * (SEMVER-MINOR) add way to get IsolateData and allocator from Environment (Anna Henningsen) #36441
      * (SEMVER-MINOR) allow preventing SetPrepareStackTraceCallback (Shelley Vohr) #36447
    stream:
      * (SEMVER-MINOR) support abortsignal in constructor (Benjamin Gruenbaum) #36431

PR-URL: #36597
aduh95 added a commit to aduh95/node that referenced this pull request Mar 16, 2023
nodejs-github-bot pushed a commit that referenced this pull request Mar 18, 2023
Refs: #36431
PR-URL: #47122
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
Refs: #36431
PR-URL: #47122
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
Refs: #36431
PR-URL: #47122
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
Refs: #36431
PR-URL: #47122
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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