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: implement finished() for ReadableStream and WritableStream #46205

Merged
merged 16 commits into from
Jan 18, 2023

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Jan 13, 2023

Implemented finished() for ReadableStreams and WritableStreams and added tests.

Refs: #39316

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jan 13, 2023
lib/internal/streams/end-of-stream.js Outdated Show resolved Hide resolved
lib/internal/streams/end-of-stream.js Show resolved Hide resolved
test/parallel/test-stream-end-of-streams.js Outdated Show resolved Hide resolved
test/parallel/test-stream-finished.js Outdated Show resolved Hide resolved
@debadree25
Copy link
Member Author

So far now its not breaking any more tests hence opening the PR for review

@debadree25 debadree25 marked this pull request as ready for review January 14, 2023 09:43
@debadree25 debadree25 changed the title streams: implement finished() for webstreams stream: implement finished() for webstreams Jan 14, 2023
throw new ERR_INVALID_THIS('ReadableStream');
return this[kState].streamClosed.promise;
}

Copy link
Member

Choose a reason for hiding this comment

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

You can't add new public properties to web streams. That is not allowed by the spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok updating this to private access?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have updated to access using [kState]

lib/internal/streams/utils.js 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.

Can you please add some more tests for error conditions? You'd also need tests for Duplex too.

@debadree25
Copy link
Member Author

Can you please add some more tests for error conditions? You'd also need tests for Duplex too.

Understood trying to add the same!

@debadree25 debadree25 marked this pull request as draft January 14, 2023 11:18
@debadree25
Copy link
Member Author

Converting this to a draft again, since there seems to be some work left and it seems to be more complex than I anticipated 😅 but any comments on the general approach I am taking regarding adding promises is it a good enough path? @mcollina, @ronag

lib/internal/streams/end-of-stream.js Show resolved Hide resolved
PromisePrototypeThen(
stream[kState].streamClosed,
() => callback.call(stream),
(err) => callback.call(stream, err)
Copy link
Member

Choose a reason for hiding this comment

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

You need to call the callback with a process.nextTick

Copy link
Member Author

Choose a reason for hiding this comment

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

understood updating

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed this to properly use process.nextTick

@@ -1869,6 +1870,7 @@ function readableStreamCancel(stream, reason) {
function readableStreamClose(stream) {
assert(stream[kState].state === 'readable');
stream[kState].state = 'closed';
stream[kState].streamClosed?.resolve?.();
Copy link
Member

Choose a reason for hiding this comment

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

Can streamClosed be nully?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it can because initially when I tested without null checks it failed

Copy link
Member

Choose a reason for hiding this comment

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

Then I think there might be a bug somewhere. What does it mean when it's nully?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay trying to investigate

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed this, removed the null checks basically it was breaking tests where tee() was being used to construct streams

lib/internal/webstreams/readablestream.js Outdated Show resolved Hide resolved
lib/internal/streams/utils.js Outdated Show resolved Hide resolved
@debadree25
Copy link
Member Author

Have added a number of tests on the error conditions and promises on both readable & writable streams,

You'd also need tests for Duplex too.

Didn't get you here?

@mcollina

@debadree25
Copy link
Member Author

Reopening the PR again for review, it seems stable now

@debadree25 debadree25 marked this pull request as ready for review January 14, 2023 21:03
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

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

@debadree25 debadree25 changed the title stream: implement finished() for webstreams stream: implement finished() for ReadableStream and WritableStream Jan 15, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
Refs: #39316
PR-URL: #46205
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jan 31, 2023
PR-URL: #46312
Refs: #46190
Refs: #46205
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
PR-URL: #46312
Refs: #46190
Refs: #46205
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
juanarbol added a commit that referenced this pull request Feb 1, 2023
Notable changes:

* deps:
  * upgrade npm to 9.3.1 (npm team) #46242
* doc:
  * add parallelism note to os.cpus() (Colin Ihrig) #45895
* http:
  * join authorization headers (Marco Ippolito) #45982
  * improved timeout defaults handling (Paolo Insogna) #45778
* stream:
  * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205

PR-URL: #46396
juanarbol added a commit that referenced this pull request Feb 1, 2023
Notable changes:

* deps:
  * upgrade npm to 9.3.1 (npm team) #46242
* doc:
  * add parallelism note to os.cpus() (Colin Ihrig) #45895
* http:
  * join authorization headers (Marco Ippolito) #45982
  * improved timeout defaults handling (Paolo Insogna) #45778
* stream:
  * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205

PR-URL: #46396
juanarbol added a commit that referenced this pull request Feb 1, 2023
Notable changes:

* deps:
  * upgrade npm to 9.3.1 (npm team) #46242
* doc:
  * add parallelism note to os.cpus() (Colin Ihrig) #45895
* http:
  * join authorization headers (Marco Ippolito) #45982
  * improved timeout defaults handling (Paolo Insogna) #45778
* stream:
  * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205

PR-URL: #46396
juanarbol added a commit that referenced this pull request Feb 2, 2023
Notable changes:

* deps:
  * upgrade npm to 9.3.1 (npm team) #46242
* doc:
  * add parallelism note to os.cpus() (Colin Ihrig) #45895
* http:
  * join authorization headers (Marco Ippolito) #45982
  * improved timeout defaults handling (Paolo Insogna) #45778
* stream:
  * implement finished() for ReadableStream and WritableStream (Debadree Chatterjee) #46205

PR-URL: #46396
nodejs-github-bot pushed a commit that referenced this pull request Feb 2, 2023
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
PR-URL: nodejs#46312
Refs: nodejs#46190
Refs: nodejs#46205
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#46190
Refs: nodejs#46205 (comment)
PR-URL: nodejs#46315
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#46205
PR-URL: nodejs#46403
Refs: nodejs#37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
PR-URL: nodejs#46312
Refs: nodejs#46190
Refs: nodejs#46205
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#46190
Refs: nodejs#46205 (comment)
PR-URL: nodejs#46315
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#46205
PR-URL: nodejs#46403
Refs: nodejs#37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
PR-URL: nodejs#46312
Refs: nodejs#46190
Refs: nodejs#46205
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#46190
Refs: nodejs#46205 (comment)
PR-URL: nodejs#46315
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
Refs: nodejs#46205
PR-URL: nodejs#46403
Refs: nodejs#37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Copy link
Contributor

@kanongil kanongil left a comment

Choose a reason for hiding this comment

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

While it is nice to be able to know when a stream has closed, I'm not a fan of this feature.

It will be cumbersome (impossible?) to implement in readable-stream for browser usage since it will need to rely on the public WebStreams API.

Comment on lines +64 to +66
if (isReadableStream(stream) || isWritableStream(stream)) {
return eosWeb(stream, options, callback);
}
Copy link
Contributor

@kanongil kanongil Apr 13, 2023

Choose a reason for hiding this comment

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

These match non-node WebStreams – which I guess is fine, except that eosWeb() will throw a TypeError since the private property kIsClosedPromise will not exist on it. This will in turn mean that the callback is never called.

Copy link
Member Author

Choose a reason for hiding this comment

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

As in you want eosWeb to additionally have a check to throw TypeError ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what node should do – probably still throw some kind of error.

Currently non-node WebStreams would just throw a TypeError from accessing the promise property on the undefined kIsClosedPromise property here:

stream[kIsClosedPromise].promise,

debadree25 added a commit to debadree25/node that referenced this pull request May 23, 2023
PR-URL: nodejs#46312
Refs: nodejs#46190
Refs: nodejs#46205
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
debadree25 added a commit to debadree25/node that referenced this pull request May 23, 2023
Refs: nodejs#46190
Refs: nodejs#46205 (comment)
PR-URL: nodejs#46315
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request May 29, 2023
PR-URL: #46312
Backport-PR-URL: #46314
Refs: #46190
Refs: #46205
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request May 29, 2023
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Backport-PR-URL: #46314
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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
Development

Successfully merging this pull request may close these issues.

8 participants