Skip to content

stream: allow transfer of readable byte streams - #45955

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
mrbbot:fix/readable-bytes-stream-transfer
Feb 4, 2023
Merged

stream: allow transfer of readable byte streams#45955
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
mrbbot:fix/readable-bytes-stream-transfer

Conversation

@mrbbot

@mrbbotmrbbot commented Dec 23, 2022

Copy link
Copy Markdown
Contributor

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

conststream=newReadableStream({type: "bytes",pull(controller){controller.enqueue(newUint8Array([1,2,3]));controller.close();},});conststream2=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.

conststream=newReadableStream({type: "bytes",pull(controller){controller.enqueue(newUint8Array([1,2,3]));controller.close();},});conststream2=structuredClone(stream,{transfer: [stream]});constreader=awaitstream2.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-botnodejs-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
Bumpundicito5.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.
@daeyeondaeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2023
@github-actionsgithub-actionsBot 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

nodejs-github-bot commented Jan 3, 2023

Copy link
Copy Markdown
Collaborator

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

@daeyeondaeyeon 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

daeyeon commented Jan 3, 2023

Copy link
Copy Markdown
Member

/cc @nodejs/whatwg-stream

mcollina
mcollina approved these changes Jan 3, 2023
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@bnoordhuis

bnoordhuis commented Jan 4, 2023

Copy link
Copy Markdown
Member

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

@benjamingrbenjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 28, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. 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/

@mcollina
mcollina requested a review from jasnellJanuary 28, 2023 16:11
@debadree25debadree25 mentioned this pull request Jan 31, 2023
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/

@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/

mcollina
mcollina approved these changes Feb 4, 2023
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollinamcollina 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-botnodejs-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:mainFeb 4, 2023
@nodejs-github-bot

nodejs-github-bot commented Feb 4, 2023

Copy link
Copy Markdown
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 <daeyeon.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Feb 19, 2023
Merged
@mrbbotmrbbot 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 <daeyeon.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Apr 11, 2023
Merged
@dai-shidai-shi mentioned this pull request Jan 3, 2024
Merged
@Mohammed6777Mohammed6777 mentioned this pull request Feb 13, 2024
Closed
@octo-stsocto-stsBot mentioned this pull request Dec 13, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@benjamingrbenjamingrbenjamingr approved these changes
@daeyeondaeyeondaeyeon approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd 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