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

stream: using an async iterator on a pipeline works on v18 but in v19 needs Readable.from(asyncItFn) #46141

Closed
ErickWendel opened this issue Jan 9, 2023 · 6 comments · Fixed by #46147
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@ErickWendel
Copy link
Member

ErickWendel commented Jan 9, 2023

Version

19.3

Platform

darwin

Subsystem

No response

What steps will reproduce the bug?

The code below works on v18.13.0 but crashes on v19.3.0

import { pipeline } from 'node:stream/promises'
async function* myCustomReadable() {
    yield Buffer.from(`tick: ${new Date().toISOString()}`)
}

async function* myCustomWritable(stream) {
  for await (const chunk of stream) {
    console.log(chunk.toString())
  }
}

await pipeline(
  myCustomReadable,
  myCustomWritable,
)
node:internal/streams/readable:192
  const isDuplex = this instanceof Stream.Duplex;
                        ^

TypeError: Right-hand side of 'instanceof' is not an object
    at PassThrough.Readable (node:internal/streams/readable:192:25)
    at PassThrough.Duplex (node:internal/streams/duplex:58:12)
    at PassThrough.Transform (node:internal/streams/transform:106:10)
    at new PassThrough (node:internal/streams/passthrough:42:13)
    at pipelineImpl (node:internal/streams/pipeline:280:20)
    at node:stream/promises:28:5
    at new Promise (<anonymous>)
    at pipeline (node:stream/promises:17:10)
    at file:///Users/erickwendel/Downloads/projetos/cursos/mastering-streams/mastering-streams-code/example.mjs:12:7
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

Node.js v19.3.0

but it works if explicitly convert it to a Readable

await pipeline(
  Readable.from(myCustomReadable()),
  myCustomWritable,
)

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

No response

What do you see instead?

TypeError: Right-hand side of 'instanceof' is not an object

Additional information

@nodejs/streams @nodejs/tooling

@ErickWendel ErickWendel added the stream Issues and PRs related to the stream subsystem. label Jan 9, 2023
@ronag
Copy link
Member

ronag commented Jan 9, 2023

Adding import 'node:stream' at the top fixes it :/

@ronag
Copy link
Member

ronag commented Jan 9, 2023

@mcollina the problem here is all the global stream init logic lives in 'stream', so at the moment you must first import 'stream' before anything else under 'stream/...'

@ronag ronag added the confirmed-bug Issues with confirmed bugs. label Jan 9, 2023
@ErickWendel
Copy link
Member Author

OMG 😂

I'd like to help working on it. Do you know where to look at?

@ronag
Copy link
Member

ronag commented Jan 9, 2023

node/lib/stream.js
node/lib/stream/promises.js

@ErickWendel
Copy link
Member Author

@mcollina the problem here is all the global stream init logic lives in 'stream', so at the moment you must first import 'stream' before anything else under 'stream/...'

do other bugs are happening because of it?

@ErickWendel
Copy link
Member Author

ErickWendel commented Jan 9, 2023

Analyzing here it works without going on 'node:stream/promises':

import { promises } from 'node:stream'
const { pipeline } = promises
async function* myCustomReadable() {
  yield Buffer.from(`tick: ${new Date().toISOString()}`)
}

async function* myCustomWritable(stream) {
  for await (const chunk of stream) {
    console.log(chunk.toString())
  }
}

await pipeline(
  myCustomReadable,
  myCustomWritable,
)

But looking at the code, here is the problem:

if I change from require('internal/streams/passthrough') to require('stream').Passthrough it works. it seems to be something on the loader side

Gonna take a look at it tonight

ErickWendel added a commit to ErickWendel/node that referenced this issue Jan 9, 2023
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
@ErickWendel ErickWendel changed the title stream: using a async iterator on a pipeline works on v18 but in v19 needs Readable.from(asyncItFn) stream: using an async iterator on a pipeline works on v18 but in v19 needs Readable.from(asyncItFn) Jan 9, 2023
nodejs-github-bot pushed a commit that referenced this issue Jan 20, 2023
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 <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this issue Feb 1, 2023
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 <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 3, 2023
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 <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 5, 2023
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 <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants