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

Allow multiple readers at once #119

Closed
ehmicky opened this issue Mar 13, 2024 · 3 comments · Fixed by #121
Closed

Allow multiple readers at once #119

ehmicky opened this issue Mar 13, 2024 · 3 comments · Fixed by #121

Comments

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 13, 2024

When get-stream is used, other readers cannot be used at the same time.

For example, this does not work (both calls will compete for the same data):

await Promise.all([
  getStream(stream),
  getStream(stream),
])

This creates some limitations for users. For example, in Execa, the following does not work:

import {execa} from 'execa'

const subprocess = execa(...)
subprocess.stdout.on('data', chunk => {...})
const {stdout} = await subprocess

Because the data event and the stdout buffering compete for the same data. Users have to opt-out of stdout buffering by using the buffer: false option.

However, it does not have the be this way. The issue here is that get-stream uses Readable[Symbol.asyncIterator]. The default implementation relies on calling stream.read() repeatedly. This works but does not allow for multiple readers at once.

Instead, using stream.on('data') would allow for multiple readers at once. We should make get-stream use that instead.

Since get-stream supports not only Node.js Readable streams but any async iterable, this logic would only be used when a Node.js Readable stream is passed.

@sindresorhus
Copy link
Owner

👍

@sindresorhus
Copy link
Owner

uses Readable[Symbol.asyncIterator]. The default implementation relies on calling stream.read() repeatedly. This works but does not allow for multiple readers at once.

Worth opening a Node.js issue about this?

@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 14, 2024

nodejs/node#52086

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants