X Tutup
The Wayback Machine - https://web.archive.org/web/20230131200320/https://github.com/nodejs/node/pull/45955
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

Jump to bottom

stream: allow transfer of readable byte streams #45955

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

stream: allow transfer of readable byte streams #45955

wants to merge 2 commits into from
+63 −9

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 `undici@5.12.0` (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 `undici@5.14.0` (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 changes
@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
Contributor

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

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

@mcollina mcollina requested a review from jasnell Jan 28, 2023
@debadree25 debadree25 mentioned this pull request Jan 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina

@jasnell jasnell

@benjamingr benjamingr

@daeyeon daeyeon

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mrbbot @nodejs-github-bot @daeyeon @bnoordhuis @mcollina @jasnell @benjamingr
X Tutup