GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: implement finished() for ReadableStream and WritableStream by debadree25 · Pull Request #46205 · nodejs/node

Conversation

@debadree25

@debadree25 debadree25 commented Jan 13, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Refs: #39316

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Jan 13, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/streams
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams Issues and PRs related to the Web Streams API. labels Jan 13, 2023
RaisinTen
RaisinTen reviewed Jan 14, 2023
View reviewed changes
Comment thread lib/internal/streams/end-of-stream.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/streams/end-of-stream.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-stream-end-of-streams.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/parallel/test-stream-finished.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@debadree25
debadree25 force-pushed the ft/finished-webstreams branch from d99602f to e036f56 Compare January 14, 2023 08:09
@debadree25

debadree25 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
ronag
ronag requested changes Jan 14, 2023
View reviewed changes
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ronag ronag Jan 14, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/streams/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina requested changes Jan 14, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@debadree25

debadree25 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor 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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@debadree25
debadree25 marked this pull request as draft January 14, 2023 11:18
@debadree25

debadree25 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronag
ronag requested changes Jan 14, 2023
View reviewed changes
Comment thread lib/internal/streams/end-of-stream.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/streams/end-of-stream.js Outdated

@ronag ronag Jan 14, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

understood updating

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ronag
ronag reviewed Jan 14, 2023
View reviewed changes
Comment thread lib/internal/webstreams/readablestream.js Outdated

@ronag ronag Jan 14, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can streamClosed be nully?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ronag ronag Jan 14, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Okay trying to investigate

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ronag
ronag requested changes Jan 14, 2023
View reviewed changes
Comment thread lib/internal/webstreams/readablestream.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/streams/utils.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@debadree25

debadree25 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor 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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@debadree25

debadree25 commented Jan 14, 2023

Copy link
Copy Markdown
Contributor Author

Reopening the PR again for review, it seems stable now
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@debadree25
debadree25 marked this pull request as ready for review January 14, 2023 21:03
mcollina
mcollina approved these changes Jan 14, 2023
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 14, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jan 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 14, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/48988/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@debadree25 debadree25 changed the title stream: implement finished() for webstreams stream: implement finished() for ReadableStream and WritableStream Jan 15, 2023
debadree25 added 6 commits January 15, 2023 14:41
@debadree25
stream: implement finished() for ReadableStream and WritableStream
fd2fa5a 
Refs: nodejs#39316
@debadree25
fixup! restore tests
7938b4a
@debadree25
fixup! add non-stream check
f0b8c92
@debadree25
fixup! fix return
3937b22
@debadree25
fixup! remove public property
70630b3
@debadree25
fixup! use the promise and use process.nextTick
2d7ea18
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
@debadree25 @ruyadorno
stream: remove brandchecks from stream duplexify
2e5e7a9 
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
@debadree25 @ruyadorno
doc: include webstreams in finished() and Duplex.from() parameters
996bac0 
PR-URL: #46312
Refs: #46190
Refs: #46205
Reviewed-By: Antoine du Hamel <[email protected]>
juanarbol added a commit that referenced this pull request Feb 1, 2023
@juanarbol
2023-02-02, Version 18.14.0 'Hydrogen' (LTS)
220a5a1 
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
@juanarbol
2023-02-02, Version 18.14.0 'Hydrogen' (LTS)
66ab03d 
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
@debadree25
stream: add suport for abort signal in finished() for webstreams
ebcc711 
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@debadree25 @MylesBorins
stream: add suport for abort signal in finished() for webstreams
4cf4b41 
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
@debadree25
doc: include webstreams in finished() and Duplex.from() parameters
7eb38e1 
PR-URL: nodejs#46312
Refs: nodejs#46190
Refs: nodejs#46205
Reviewed-By: Antoine du Hamel <[email protected]>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
@debadree25
stream: remove brandchecks from stream duplexify
a980376 
Refs: nodejs#46190
Refs: nodejs#46205 (comment)
PR-URL: nodejs#46315
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
debadree25 added a commit to debadree25/node that referenced this pull request Feb 27, 2023
@debadree25
stream: add suport for abort signal in finished() for webstreams
f141b73 
Refs: nodejs#46205
PR-URL: nodejs#46403
Refs: nodejs#37354
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
@debadree25 @juanarbol
stream: remove brandchecks from stream duplexify
5290766 
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
@debadree25 @juanarbol
stream: remove brandchecks from stream duplexify
63e92ea 
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@debadree25 @danielleadams
stream: add suport for abort signal in finished() for webstreams
91a550e 
Refs: #46205
PR-URL: #46403
Refs: #37354
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
kanongil
kanongil reviewed Apr 13, 2023
View reviewed changes

@kanongil kanongil left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/streams/end-of-stream.js
Comment on lines +64 to +66
if (isReadableStream(stream) || isWritableStream(stream)) {
return eosWeb(stream, options, callback);
}

@kanongil kanongil Apr 13, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@debadree25 debadree25 Apr 13, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@kanongil kanongil Apr 13, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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:

node/lib/internal/streams/end-of-stream.js

Line 289 in 4667b07

stream[kIsClosedPromise].promise,

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@wooorm wooorm mentioned this pull request Apr 20, 2023
does not work with Node's stream.pipeline() built-in unifiedjs/unified-stream#1
Closed
4 tasks done
debadree25 added a commit to debadree25/node that referenced this pull request May 23, 2023
@debadree25
doc: include webstreams in finished() and Duplex.from() parameters
631afeb 
PR-URL: nodejs#46312
Refs: nodejs#46190
Refs: nodejs#46205
Reviewed-By: Antoine du Hamel <[email protected]>
debadree25 added a commit to debadree25/node that referenced this pull request May 23, 2023
@debadree25
stream: remove brandchecks from stream duplexify
0291981 
Refs: nodejs#46190
Refs: nodejs#46205 (comment)
PR-URL: nodejs#46315
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
danielleadams pushed a commit that referenced this pull request May 29, 2023
@debadree25 @danielleadams
doc: include webstreams in finished() and Duplex.from() parameters
402c1a3 
PR-URL: #46312
Backport-PR-URL: #46314
Refs: #46190
Refs: #46205
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request May 29, 2023
@debadree25 @danielleadams
stream: remove brandchecks from stream duplexify
e8b5a61 
Refs: #46190
Refs: #46205 (comment)
PR-URL: #46315
Backport-PR-URL: #46314
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@KhafraDev KhafraDev mentioned this pull request Apr 15, 2024
stream.finished docs are lacking #52544
Closed
aduh95 pushed a commit to aduh95/node that referenced this pull request Feb 18, 2025
@debadree25 @RafaelGSS
doc: include webstreams in finished() and Duplex.from() parameters
230a984 
PR-URL: nodejs#46312
Backport-PR-URL: nodejs#46314
Refs: nodejs#46190
Refs: nodejs#46205
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this pull request Feb 18, 2025
@debadree25 @RafaelGSS
stream: remove brandchecks from stream duplexify
cfb18d8 
Refs: nodejs#46190
Refs: nodejs#46205 (comment)
PR-URL: nodejs#46315
Backport-PR-URL: nodejs#46314
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@jasnell jasnell jasnell approved these changes
@ronag ronag ronag approved these changes
@RaisinTen RaisinTen RaisinTen approved these changes
+1 more reviewer
@kanongil kanongil kanongil left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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 Issues and PRs related to the Web Streams API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@debadree25 @nodejs-github-bot @mcollina @jasnell @kanongil @ronag @RaisinTen @aduh95
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.