Skip to content

stream: allow transfer of readable byte streams #45955

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.

Sign up for GitHub

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
Merged
merged 2 commits into from
Feb 4, 2023
Merged

stream: allow transfer of readable byte streams #45955

merged 2 commits into from
Feb 4, 2023

Conversation

@mrbbot
Copy link
Contributor

@mrbbot mrbbot commented Dec 23, 2022

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

@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 labels Dec 23, 2022
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 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@daeyeon daeyeon added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 3, 2023
@daeyeon
Copy link
Member

daeyeon commented Jan 3, 2023

/cc @nodejs/whatwg-stream

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

@bnoordhuis
Copy link
Member

Would this conflict with / make harder / make impossible #45853?

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

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

@mcollina mcollina requested a review from jasnell January 28, 2023 16:11
@debadree25 debadree25 mentioned this pull request Jan 31, 2023
Closed
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina approved these changes Feb 4, 2023
View reviewed 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

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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 Add this label to land a pull request using GitHub Actions. label Feb 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit 03854f6 into nodejs:main Feb 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 03854f6

This was referenced Feb 5, 2023
Open
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
Merged
@mrbbot mrbbot mentioned this pull request Mar 1, 2023
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
Merged
@dai-shi dai-shi mentioned this pull request Jan 3, 2024
Merged
@Mohammed6777 Mohammed6777 mentioned this pull request Feb 13, 2024
Closed
@octo-sts octo-sts bot mentioned this pull request Dec 13, 2024
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

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. web streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mrbbot @nodejs-github-bot @daeyeon @bnoordhuis @mcollina @jasnell @benjamingr