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 — streams: fixes for webstreams by MattiasBuelens · Pull Request #51168 · nodejs/node

Conversation

@MattiasBuelens

@MattiasBuelens MattiasBuelens commented Dec 15, 2023

Copy link
Copy Markdown
Contributor

Follow-up on #50126.

  • Avoid PromiseResolve in ensureIsPromise. Web IDL requires this to always return a new Promise, whereas PromiseResolve may return the same Promise. This leads to a different timing, since new Promise() takes at least one microtask to resolve. See my previous comment.
  • Fix handling sync errors from source cancel and sink abort. We were not correctly handling synchronously-thrown errors from these, which could leave the stream in an incorrect state.
  • Check for promise only when constructing from source/sink/transformer. When constructing an internal stream, these algorithms must always return a promise, and we shouldn't wrap them again with ensureIsPromise().
  • Fix TeeReadableStream. This should construct an internal stream with internal callbacks using setupReadableStreamDefaultController, but it was incorrectly using user-land callbacks with setupReadableStreamDefaultControllerFromSource.
  • Use internal streams for teeing a readable byte stream. This was using the user-land ReadableStream constructor, which is incorrect. The new implementation mirrors how we handle teeing "default" readable streams.
  • Use internal streams for ReadableStream.from(). Again, this was incorrectly using the user-land ReadableStream constructor.
  • Use internal streams for TransformStream. Same thing again.
  • Add a helper to create the internal state of a ReadableStream and WritableStream. This code was duplicated a few times, so it made sense to extract a helper function.
  • Remove an unused field in the ReadableStream internal state. This seemed like a copy-paste mistake: other classes (like ReadableStreamDefaultController) do have a stream field in their state, but ReadableStream shouldn't.
  • Fix validating callbacks. Previously, we asserted that the given callbacks were indeed typeof callback === "function" by virtue of bluntly trying to call FunctionPrototypeBind on them. We no longer use FunctionPrototypeBind for this, so we need to manually call validateFunction for each callback. I've wrapped that in a createPromiseCallback helper.
  • Update expectations for the Streams WPT tests.

Sorry, something went wrong.

Uh oh!

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

jasnell and mcollina reacted with hooray emoji bricss reacted with eyes emoji
All reactions
  • 🎉 2 reactions
  • 👀 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Dec 15, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/web-standards
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 Dec 15, 2023
MattiasBuelens added 8 commits December 16, 2023 00:11
@MattiasBuelens
stream: fix code style
cb4e020
@MattiasBuelens
stream: avoid PromiseResolve in ensureIsPromise
46022b8
@MattiasBuelens
stream: fix handling sync errors from source cancel and sink abort
62fc1e1
@MattiasBuelens
stream: check for promise only when constructing from source/sink/tra…
19e8940
…nsformer
@MattiasBuelens
stream: fix TeeReadableStream
ab33b45
@MattiasBuelens
stream: rename TeeReadableStream to InternalReadableStream
463f49a
@MattiasBuelens
stream: use internal ReadableStream for teeing byte streams
36324d5
@MattiasBuelens
stream: use internal ReadableStream for ReadableStream.from()
f0b1036
ljharb
ljharb reviewed Dec 15, 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/webstreams/util.js Outdated
Comment on lines 183 to 190

@ljharb ljharb Dec 15, 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.

if it's always making a new promise, any reason not to do this?

Suggested change
function ensureIsPromise(fn, thisArg, ...args) {
try {
const value = FunctionPrototypeCall(fn, thisArg, ...args);
return isPromise(value) ? value : PromiseResolve(value);
return new Promise((r) => r(value));
} catch (error) {
return PromiseReject(error);
}
}
async function ensureIsPromise(fn, thisArg, ...args) {
return await FunctionPrototypeCall(fn, thisArg, ...args);
}

Sorry, something went wrong.

Uh oh!

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

All reactions

@MattiasBuelens MattiasBuelens Dec 15, 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 like that a lot! An async function always returns a fresh Promise, so this should satisfy Web IDL. 🙂

Sorry, something went wrong.

Uh oh!

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

ljharb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@MattiasBuelens MattiasBuelens Dec 16, 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.

Unfortunately, this change causes some tests to fail again... 😞

However, if you remove the await, it does work:

async function ensureIsPromise(fn, thisArg, ...args) {
  return FunctionPrototypeCall(fn, thisArg, ...args);
}

You still get a fresh Promise (from async function), errors are still correctly turned into rejections, and the timings match Web IDL.

Still, I don't like how sensitive these tests are to the number of microtasks. I'll try to fix this in upstream WPT.

Sorry, something went wrong.

Uh oh!

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

ljharb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread lib/internal/webstreams/writablestream.js Resolved

Uh oh!

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

Comment thread lib/internal/webstreams/writablestream.js Resolved

Uh oh!

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

MattiasBuelens added 10 commits December 16, 2023 00:13
@MattiasBuelens
stream: use internal streams for TransformStream
3846bfe
@MattiasBuelens
stream: add helpers to create internal state
0d45e6f
@MattiasBuelens
stream: remove unused field in internal state
5df4f2e
@MattiasBuelens
stream: fix validating callbacks
15b30ca
@MattiasBuelens
test: update expectations for streams wpt
0bd501f
@MattiasBuelens
stream: set __proto__ to null for internal state
112a717
@MattiasBuelens
fixup! simplify ensureIsPromise
b9a13b5
@MattiasBuelens
fixup! remove unused imports
162cfdc
@MattiasBuelens
fixup! whitespace
a889e2c
@MattiasBuelens
fixup! code style
16a9cc7
@debadree25

debadree25 commented Dec 16, 2023

Copy link
Copy Markdown
Contributor

cc @nodejs/whatwg-stream

All reactions

Sorry, something went wrong.

Uh oh!

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

MattiasBuelens added 2 commits December 16, 2023 11:39
@MattiasBuelens
fixup! fix timing of ensureIsPromise
11e6f94
@MattiasBuelens
fixup! rename to invokePromiseCallback
0c0e3c1
@debadree25 debadree25 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 Dec 16, 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 Dec 16, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 16, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@debadree25
debadree25 requested a review from jasnell December 22, 2023 14:32
@nodejs-github-bot

nodejs-github-bot commented Dec 22, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@mcollina mcollina added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed needs-ci PRs that need a full CI run. labels Dec 26, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 26, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 20c6313 into nodejs:main Dec 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in 20c6313

All reactions

Sorry, something went wrong.

Uh oh!

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

@MattiasBuelens
MattiasBuelens deleted the webstreams-fixes branch December 27, 2023 10:27
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@MattiasBuelens @RafaelGSS
stream: fix code style
82de660
PR-URL: #51168
Reviewed-By: Matteo Collina <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
v21.6.0 proposal #51342
Merged
@marco-ippolito marco-ippolito added the backport-requested-v20.x label May 1, 2024
@marco-ippolito

marco-ippolito commented May 1, 2024

Copy link
Copy Markdown
Member

this doesn't land cleanly on v20 requires a manual backport

All reactions

Sorry, something went wrong.

Uh oh!

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

@MattiasBuelens

MattiasBuelens commented May 1, 2024

Copy link
Copy Markdown
Contributor Author

@marco-ippolito This PR builds upon #50107, specifically: 0d45e6f needs a85e418. However, that other PR is marked "dont-land-on-v20.x". 😕

I'm not sure how to proceed with a manual backport. Should I try to rework my changes without that PR, or should we backport #50107 to v20.x anyway?

All reactions

Sorry, something went wrong.

Uh oh!

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

@marco-ippolito

marco-ippolito commented May 1, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I guess then we wont backport it, @targos I saw you marked #50107 as dont not land on v20, wdyt

All reactions

Sorry, something went wrong.

Uh oh!

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

@marco-ippolito marco-ippolito added dont-land-on-v20.x and removed backport-requested-v20.x labels May 1, 2024
@MattiasBuelens

MattiasBuelens commented May 1, 2024

Copy link
Copy Markdown
Contributor Author

On second thought, I think I can rework my changes. Let me try something... 👨‍🔬

All reactions

Sorry, something went wrong.

Uh oh!

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

@MattiasBuelens MattiasBuelens mentioned this pull request May 1, 2024
[v20.x backport] streams: fixes for webstreams #52773
Closed
@MattiasBuelens

MattiasBuelens commented May 1, 2024

Copy link
Copy Markdown
Contributor Author

@marco-ippolito I managed to make it work without #50107, see #52773.

However, If we also want to backport #50107, then I'll need to change it up again. 😛 Let me know how you want to proceed.

All reactions

Sorry, something went wrong.

Uh oh!

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

@targos

targos commented May 1, 2024

Copy link
Copy Markdown
Member

I marked it dont-land because it depended on #47956, which was dont-land. Feel free to remove the label and backport.

All reactions

Sorry, something went wrong.

Uh oh!

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

@marco-ippolito marco-ippolito added backport-open-v20.x and removed dont-land-on-v20.x labels May 1, 2024
MattiasBuelens added a commit to MattiasBuelens/node that referenced this pull request Jun 25, 2024
@MattiasBuelens
stream: fixes for webstreams
573f5df
PR-URL: nodejs#51168
Backport-PR-URL: nodejs#52773
Reviewed-By: Matteo Collina <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@MattiasBuelens @marco-ippolito
stream: fixes for webstreams
48138af
PR-URL: #51168
Backport-PR-URL: #52773
Reviewed-By: Matteo Collina <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Jul 19, 2024
v20.16.0 proposal #53945
Merged
@targos targos added backported-to-v20.x and removed backport-open-v20.x labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@mcollina mcollina mcollina approved these changes
@jasnell jasnell Awaiting requested review from jasnell

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

7 participants

@MattiasBuelens @nodejs-github-bot @debadree25 @marco-ippolito @targos @ljharb @mcollina
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.