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: allow transfer of readable byte streams by mrbbot · Pull Request #45955 · nodejs/node

Conversation

@mrbbot

@mrbbot mrbbot commented Dec 23, 2022

Copy link
Copy Markdown
Contributor

Previosuly, attempting to transfer a type: "bytes" ReadableStream like so...

const stream = new ReadableStream({
  type: "bytes",
  pull(controller) {
    controller.enqueue(new Uint8Array([1, 2, 3]));
    controller.close();
  },
});
const stream2 = structuredClone(stream, { transfer: [stream] });

...would fail with...

node:internal/structured_clone:23
  channel.port1.postMessage(value, options?.transfer);
                ^

TypeError: Found invalid object in transferList
    at structuredClone (node:internal/structured_clone:23:17)
    at file:///.../streams.mjs:8:17
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25) {
  code: 'ERR_INVALID_TRANSFER_OBJECT'
}

Node.js v19.3.0

This PR updates the ReadableStream constructor to mark byte streams as transferable. When transferred, byte streams become regular streams.

const stream = new ReadableStream({
  type: "bytes",
  pull(controller) {
    controller.enqueue(new Uint8Array([1, 2, 3]));
    controller.close();
  },
});

const stream2 = structuredClone(stream, { transfer: [stream] });
const reader = await stream2.getReader(); // `{ mode: "byob" }` would fail here as
                                          // `stream2` is no longer a byte stream

(tested with Chrome 108.0.5359.124)

Refs: #39062
Refs: https://streams.spec.whatwg.org/#rs-transfer

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and X7md reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@mrbbot
stream: allow transfer of readable byte streams
ca855be 
Updates the `ReadableStream` constructor to mark byte streams as
transferable. When transferred, byte streams become regular streams.

Refs: nodejs#39062
Refs: https://streams.spec.whatwg.org/#rs-transfer
@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 23, 2022
@mrbbot
fixup! stream: allow transfer of readable byte streams
8e6988f
mrbbot added a commit to cloudflare/miniflare that referenced this pull request Dec 23, 2022
@mrbbot
Bump undici to 5.11.0. closes #351
f3f1ae9 
`5.11.0` includes nodejs/undici#1643.

There are some non-trivial changes required to upgrade to `5.14.0`:

- Since `[email protected]` (nodejs/undici#1697),
  `structuredClone` is used on bodies, which may be byte streams.
  Due to a Node bug (nodejs/node#45955),
  readable byte streams cannot be transferred, breaking `fetch`.
- Since `[email protected]` (nodejs/undici#1793),
  global `ReadableStream` and `TransformStream` are used if
  available. In the Vitest environment, (which modifies the global
  scope unlike Jest which runs tests in a VM context), if the
  `streams_enable_constructors` compatibility flag isn't enabled,
  `fetch` breaks as `ReadableStream`s can't be constructed.
daeyeon
daeyeon approved these changes Jan 3, 2023
View reviewed changes
@daeyeon daeyeon 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 3, 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 3, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Jan 3, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daeyeon daeyeon added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 3, 2023
@daeyeon

daeyeon commented Jan 3, 2023

Copy link
Copy Markdown
Member

/cc @nodejs/whatwg-stream
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Jan 3, 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
@bnoordhuis

bnoordhuis commented Jan 4, 2023

Copy link
Copy Markdown
Member

Would this conflict with / make harder / make impossible #45853?
benjamingr reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Jan 28, 2023
View reviewed changes
@benjamingr benjamingr 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 28, 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 28, 2023
@nodejs-github-bot

nodejs-github-bot commented Jan 28, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina
mcollina requested a review from jasnell January 28, 2023 16:11
jasnell
jasnell approved these changes Jan 28, 2023
View reviewed changes
@debadree25 debadree25 mentioned this pull request Jan 31, 2023
stream: make readable byte stream cloneable #46437
Closed
@nodejs-github-bot

nodejs-github-bot commented Feb 3, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Feb 4, 2023

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Feb 4, 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 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 Feb 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Feb 4, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 03854f6 into nodejs:main Feb 4, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 4, 2023

Copy link
Copy Markdown
Collaborator

Landed in 03854f6
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Feb 5, 2023
CI Reliability 2023-02-05 nodejs/reliability#495
Open
CI Reliability 2023-02-06 nodejs/reliability#497
Open
@mrbbot
mrbbot deleted the fix/readable-bytes-stream-transfer branch February 6, 2023 08:42
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@mrbbot @MylesBorins
stream: allow transfer of readable byte streams
6bf7388 
Updates the `ReadableStream` constructor to mark byte streams as
transferable. When transferred, byte streams become regular streams.

Refs: #39062
Refs: https://streams.spec.whatwg.org/#rs-transfer
PR-URL: #45955
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
v19.7.0 proposal #46725
Merged
@mrbbot mrbbot mentioned this pull request Mar 1, 2023
TypeError: Found invalid object in transferList cloudflare/miniflare#527
Closed
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@mrbbot @danielleadams
stream: allow transfer of readable byte streams
4d81124 
Updates the `ReadableStream` constructor to mark byte streams as
transferable. When transferred, byte streams become regular streams.

Refs: #39062
Refs: https://streams.spec.whatwg.org/#rs-transfer
PR-URL: #45955
Reviewed-By: Daeyeon Jeong <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
v18.16.0 release proposal #47502
Merged
@dai-shi dai-shi mentioned this pull request Jan 3, 2024
chore: update node requirement wakujs/waku#343
Merged
@Mohammed6777 Mohammed6777 mentioned this pull request Feb 13, 2024
Previosuly, attempting to transfer a type: "bytes" ReadableStream like so... al3xbusk/icracked#3
Closed
@octo-sts octo-sts Bot mentioned this pull request Dec 13, 2024
prometheus-3.0/3.0.1-r0: cve remediation wolfi-dev/os#36782
Merged
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
@benjamingr benjamingr benjamingr approved these changes
@daeyeon daeyeon daeyeon approved these changes

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

@mrbbot @nodejs-github-bot @daeyeon @bnoordhuis @mcollina @jasnell @benjamingr
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.