X Tutup
The Wayback Machine - https://web.archive.org/web/20230131200418/https://github.com/nodejs/node/pull/46273
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

Jump to bottom

stream: add abort signal for ReadableStream and WritableStream #46273

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

stream: add abort signal for ReadableStream and WritableStream #46273

wants to merge 13 commits into from

Conversation

debadree25
Copy link
Contributor

Refs: #39316

@nodejs-github-bot
Copy link
Contributor

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jan 19, 2023
@mcollina mcollina requested review from ronag, jasnell and benjamingr Jan 19, 2023
aduh95
aduh95 reviewed Jan 20, 2023
View changes
lib/internal/streams/utils.js
@@ -13,6 +13,7 @@ const kIsReadable = Symbol('kIsReadable');
const kIsDisturbed = Symbol('kIsDisturbed');

const kIsClosedPromise = SymbolFor('nodejs.webstream.isClosedPromise');
const kControllerErrorFunction = SymbolFor('nodejs.webstream.controllerErrorFunction');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using Symbol.for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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

lib/internal/webstreams/readablestream.js Outdated Show resolved Hide resolved
lib/internal/streams/add-abort-signal.js Outdated Show resolved Hide resolved
lib/internal/webstreams/writablestream.js Outdated Show resolved Hide resolved
test/parallel/test-webstreams-abort-controller.js Outdated Show resolved Hide resolved
test/parallel/test-webstreams-abort-controller.js Outdated Show resolved Hide resolved
debadree25 and others added 6 commits Jan 20, 2023
@debadree25 @aduh95
fixup! make onAbort const
14bd149 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@debadree25 @aduh95
fixup! fix test assertion
31c024f 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@debadree25 @aduh95
fixup! fix assertion
8a405e4 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@debadree25 @aduh95
fixup! use primordial
2f33583 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@debadree25 @aduh95
fixup! use primordial
ccc65af 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@debadree25
fixup! add more tests
31afe61
@debadree25
Copy link
Contributor Author

Added more few more tests should be covering most scenarios

mcollina
mcollina approved these changes Jan 21, 2023
View changes
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

@aduh95 aduh95 mentioned this pull request Jan 22, 2023
Open
benjamingr
benjamingr approved these changes Jan 22, 2023
View changes
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.

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

@benjamingr
Copy link
Member

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.

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 22, 2023
@nodejs-github-bot
Copy link
Contributor

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

@benjamingr
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Jan 22, 2023 •
edited

The docs for addAbortSignal needs to be updated also

Heh, jinx.

@debadree25
Copy link
Contributor Author

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

Adding!

@debadree25
Copy link
Contributor Author

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

jasnell commented Jan 22, 2023

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
Copy link
Contributor Author

Have added to docs @benjamingr @jasnell

aduh95
aduh95 reviewed Jan 22, 2023
View changes
doc/api/stream.md Outdated Show resolved Hide resolved
@debadree25 @aduh95
fixup! doc
7255a78 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95
aduh95 reviewed Jan 22, 2023
View changes
doc/api/stream.md Outdated Show resolved Hide resolved
debadree25 and others added 2 commits Jan 22, 2023
@debadree25 @aduh95
fixup! full stop
a464999 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@debadree25
fixup! lint trailing space
fc1300c
@github-actions github-actions bot mentioned this pull request Jan 23, 2023
Open
13 tasks
aduh95
aduh95 reviewed Jan 23, 2023
View changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

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

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

What should the version be v19.5.0?

@aduh95
Copy link
Contributor

aduh95 commented Jan 23, 2023

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
Copy link
Contributor Author

Added the version @aduh95

This was referenced Jan 24, 2023
Open
Open
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 25, 2023
@nodejs-github-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95

@mcollina mcollina

@jasnell jasnell

@benjamingr benjamingr

@ronag ronag

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@debadree25 @nodejs-github-bot @benjamingr @jasnell @aduh95 @mcollina @lpinca
X Tutup