stream: add abort signal for ReadableStream and WritableStream by debadree25 · Pull Request #46273 · nodejs/node

debadree25 · 2023-01-19T14:02:34Z

nodejs-github-bot · 2023-01-19T14:02:40Z

Review requested:

@nodejs/streams

aduh95 · 2023-01-20T16:46:24Z

lib/internal/streams/utils.js

 const kIsDisturbed = Symbol('kIsDisturbed');

 const kIsClosedPromise = SymbolFor('nodejs.webstream.isClosedPromise');
+const kControllerErrorFunction = SymbolFor('nodejs.webstream.controllerErrorFunction');


Why are we using Symbol.for here?

I used the similar pattern to what we followed over here #46205 (comment)

@aduh95 I presume Node.js streams or another part that exists both in core and userland added this for interoperability (so that require("node:stream") and require("readable-stream") interoperate) and then everyone (probably me too) saw the code and cargo culted. Probably a bunch at the point we moved private variables to symbols when privates were not supported in snapshots back then.

It's also arguably easier to debug "from the outside" but not by a considerable margin.

lib/internal/webstreams/readablestream.js

lib/internal/streams/add-abort-signal.js

lib/internal/webstreams/writablestream.js

test/parallel/test-webstreams-abort-controller.js

debadree25 · 2023-01-21T19:29:24Z

Added more few more tests should be covering most scenarios

mcollina

lgtm

benjamingr

Neat (I'm +1 on this and approving). I'll add some questions later.

benjamingr · 2023-01-22T19:16:42Z

Hey, consider going through the standards track and adding this to streams through WHATAG (like a signal parameter to the constructor). This is not instead of this PR but in addition.

nodejs-github-bot · 2023-01-22T19:20:32Z

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

benjamingr · 2023-01-22T19:22:57Z

Can you also please add to the docs for addAbortSignal at stream.md ?

jasnell · 2023-01-22T19:24:15Z

~~The docs for addAbortSignal needs to be updated also~~

Heh, jinx.

debadree25 · 2023-01-22T19:27:14Z

Can you also please add to the docs for addAbortSignal at stream.md ?

Adding!

debadree25 · 2023-01-22T19:29:59Z

Hey, consider going through the standards track and adding this to streams through WHATAG (like a signal parameter to the constructor). This is not instead of this PR but in addition.

Hi so are you thinking something like new ReadableStream({ signal: ac.signal })

jasnell · 2023-01-22T19:36:58Z

Adding a signal to the ReadableStream constructor is not something I would really expect to see happen in whatwg, particularly since there is already a way of cancelling the stream via the existing API. Could be worth a try but I'd be surprised if it were accepted.

debadree25 · 2023-01-22T19:51:47Z

Have added to docs @benjamingr @jasnell

doc/api/stream.md

aduh95

Can you add an history entry in the YAML comment please?

debadree25 · 2023-01-23T08:28:58Z

What should the version be v19.5.0?

aduh95 · 2023-01-23T08:45:59Z

What should the version be v19.5.0?

It should be REPLACEME:

node/doc/contributing/pull-requests.md

Lines 125 to 136 in 5af2021

    
           `vcbuild.bat lint` on Windows). If you are adding to or deprecating an API, 
        
           add or change the appropriate YAML documentation. Use `REPLACEME` for the 
        
           version number in the documentation YAML: 
        
           ```markdown 
        
           ### `request.method` 
        
           <!-- YAML 
        
           added: REPLACEME 
        
           --> 
        
           * {string} The request method. 
        
           ```

debadree25 · 2023-02-02T19:57:25Z

Rebased and resolved conflict

nodejs-github-bot · 2023-02-02T19:58:11Z

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

debadree25 · 2023-02-07T18:06:54Z

could this have another CI run? the macOS failure seems unrelated

mcollina

lgtm

nodejs-github-bot · 2023-02-17T11:09:10Z

Landed in 96c720e

Refs: #39316 PR-URL: #46273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: TODO

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: #46725

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) nodejs#46673 * add ada as a dependency (Yagiz Nizipli) nodejs#46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) nodejs#46716 * add deokjinkim to collaborators (Deokjin Kim) nodejs#46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) nodejs#46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) nodejs#46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) nodejs#45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) nodejs#46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) nodejs#46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) nodejs#46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) nodejs#46273 test_runner: * add initial code coverage support (Colin Ihrig) nodejs#46017 url: * replace url-parser with ada (Yagiz Nizipli) nodejs#46410 PR-URL: nodejs#46725

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: #46725

Refs: #39316 PR-URL: #46273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: #46725

Refs: nodejs#39316 PR-URL: nodejs#46273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Refs: #39316 PR-URL: #46273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: TBD

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

Notable changes: Add initial support for single executable applications Compile a JavaScript file into a single executable application: ```console $ echo 'console.log(`Hello, ${process.argv[2]}!`);' > hello.js $ cp $(command -v node) hello $ npx postject hello NODE_JS_CODE hello.js \ --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 $ npx postject hello NODE_JS_CODE hello.js \ --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 \ --macho-segment-name NODE_JS $ ./hello world Hello, world! ``` Contributed by Darshan Sen in #45038 Replace url parser with Ada Node.js gets a new URL parser called Ada that is compliant with the WHATWG URL Specification and provides more than 100% performance improvement to the existing implementation. Contributed by Yagiz Nizipli in #46410 Other notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jan 19, 2023

mcollina requested review from benjamingr, jasnell and ronag January 19, 2023 20:26

aduh95 reviewed Jan 20, 2023

View reviewed changes

mcollina approved these changes Jan 21, 2023

View reviewed changes

aduh95 mentioned this pull request Jan 22, 2023

stream: expose stream symbols #45671

Merged

benjamingr approved these changes Jan 22, 2023

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2023

benjamingr approved these changes Jan 22, 2023

View reviewed changes

jasnell approved these changes Jan 22, 2023

View reviewed changes

aduh95 reviewed Jan 22, 2023

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

aduh95 reviewed Jan 22, 2023

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Jan 23, 2023

CI Reliability 2023-01-23 nodejs/reliability#492

Open

13 tasks

aduh95 reviewed Jan 23, 2023

View reviewed changes

aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 23, 2023

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 17, 2023

mcollina approved these changes Feb 17, 2023

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 17, 2023

nodejs-github-bot merged commit 96c720e into nodejs:main Feb 17, 2023

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

anonrig mentioned this pull request Feb 21, 2023

fetch() performance parity nodejs/undici#1203

Closed

debadree25 mentioned this pull request Feb 27, 2023

[v18.x backport] stream: streams and webstreams interop related changes #46314

Closed

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

Uh oh!

Conversation

debadree25 commented Jan 19, 2023

Uh oh!

nodejs-github-bot commented Jan 19, 2023

Uh oh!

aduh95 Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

debadree25 Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

benjamingr Jan 22, 2023

Choose a reason for hiding this comment

Uh oh!

benjamingr Jan 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

debadree25 commented Jan 21, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Jan 22, 2023

Uh oh!

nodejs-github-bot commented Jan 22, 2023

Uh oh!

benjamingr commented Jan 22, 2023

Uh oh!

jasnell commented Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

debadree25 commented Jan 22, 2023

Uh oh!

debadree25 commented Jan 22, 2023

Uh oh!

jasnell commented Jan 22, 2023

Uh oh!

debadree25 commented Jan 22, 2023

Uh oh!

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

debadree25 commented Jan 23, 2023

Uh oh!

aduh95 commented Jan 23, 2023

Uh oh!

debadree25 commented Feb 2, 2023

Uh oh!

nodejs-github-bot commented Feb 2, 2023

Uh oh!

debadree25 commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jasnell commented Jan 22, 2023 •

edited

Loading

debadree25 commented Feb 7, 2023 •

edited

Loading