-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
stream: fix readable stream as async iterator function #46147
stream: fix readable stream as async iterator function #46147
Conversation
Since v19.2 it's not possible to use readableStreams as async iterators (confirmed bug). This patch fixes the problem by reading the Stream.Duplex property from 'streams/duplex' instead of 'streams/legacy' module Fixes: nodejs#46141
Review requested:
|
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, we need the stream
module to be acyclic for readable-stream
to be bundle friendly. We need a different solution for this problem.
On my tests, the Stream legacy is a function and never has the Duplex as a property there thus was crashing. A different solution was instead of using 'internal/streams/duplex' in the transform.js file is to use "streams".Duplex. This works right away but I'm not sure if it's a good solution pointing to the public api from a internal api |
This is not possible. That value will be populated once all of
You should load it from |
that's how it was and wasn't working :( Gonna try more tonight |
I'm working on it today. It seems when it's importing the I agree with what @ronag said in the issue that the problem is that all the global stream init logic lives in 'stream', so at the moment you must first import 'stream' before anything else under 'stream/...' To fix that I'd just add in the Although I'm not sure if it's a good thing to do here. WDYT? |
Go for it. |
8e15dc2
to
88a656a
Compare
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
Commit Queue failed- Loading data for nodejs/node/pull/46147 ✔ Done loading data for nodejs/node/pull/46147 ----------------------------------- PR info ------------------------------------ Title stream: fix readable stream as async iterator function (#46147) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch ErickWendel:fix/async-iterator-as-readable -> nodejs:main Labels needs-ci Commits 3 - stream: fix readable stream as async iterator function - stream: rollback duplex from new api - stream: load stream module when using stream/promises Committers 1 - Erick Wendel PR-URL: https://github.com/nodejs/node/pull/46147 Fixes: https://github.com/nodejs/node/issues/46141 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46147 Fixes: https://github.com/nodejs/node/issues/46141 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Robert Nagy Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ℹ This PR was created on Mon, 09 Jan 2023 18:50:41 GMT ✔ Approvals: 3 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/46147#pullrequestreview-1241128088 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/46147#pullrequestreview-1247926897 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/46147#pullrequestreview-1247891473 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-01-18T15:27:58Z: https://ci.nodejs.org/job/node-test-pull-request/49044/ - Querying data for job/node-test-pull-request/49044/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD 5d560978ff..d896f5befd main -> origin/main ✔ origin/main is now up-to-date main is out of sync with origin/main. Mismatched commits: - b413a58c20 src: make BuiltinLoader threadsafe and non-global - d896f5befd src: make BuiltinLoader threadsafe and non-global -------------------------------------------------------------------------------- HEAD is now at d896f5befd src: make BuiltinLoader threadsafe and non-global ✔ Reset to origin/main - Downloading patch for 46147 From https://github.com/nodejs/node * branch refs/pull/46147/merge -> FETCH_HEAD ✔ Fetched commits as 5d560978ff50..88a656afa339 -------------------------------------------------------------------------------- [main 268412028e] stream: fix readable stream as async iterator function Author: Erick Wendel Date: Mon Jan 9 15:43:11 2023 -0300 3 files changed, 33 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-stream3-pipeline-async-iterator.js [main c7946f1f1d] stream: rollback duplex from new api Author: Erick Wendel Date: Fri Jan 13 12:04:20 2023 -0300 2 files changed, 4 insertions(+), 6 deletions(-) [main 1a473662ce] stream: load stream module when using stream/promises Author: Erick Wendel Date: Fri Jan 13 12:28:02 2023 -0300 2 files changed, 13 insertions(+), 11 deletions(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6)https://github.com/nodejs/node/actions/runs/3953273782 |
Landed in 5d0946a |
Since v19.2 it's not possible to use readableStreams as async iterators (confirmed bug). This patch fixes the problem by reading the Stream.Duplex property from 'streams/duplex' instead of 'streams/legacy' module Fixes: #46141 PR-URL: #46147 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Since v19.2 it's not possible to use readableStreams as async iterators (confirmed bug). This patch fixes the problem by reading the Stream.Duplex property from 'streams/duplex' instead of 'streams/legacy' module Fixes: #46141 PR-URL: #46147 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Since v19.2 it's not possible to use readableStreams as async iterators (confirmed bug). This patch fixes the problem by reading the Stream.Duplex property from 'streams/duplex' instead of 'streams/legacy' module Fixes: #46141 PR-URL: #46147 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Since v19.2 it's not possible to use Readable streams as async iterators (confirmed bug).
This patch fixes the problem by loading the 'stream' module when using node/streams/promises
Fixes: #46141
cc @nodejs/streams