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

fix: handle large throughput writes #1184

Merged
merged 1 commit into from
Sep 4, 2024
Merged

fix: handle large throughput writes #1184

merged 1 commit into from
Sep 4, 2024

Conversation

mdonnalley
Copy link
Contributor

Use console.log and console.error instead of process.stdout.write and process.stderr.write, to avoid losing information from back pressure on the streams

To replicate:

// write.js

import {stdout} from './src/ux/write.js'
const lines = Array.from(
  {length: 100},
  (_, i) =>
    `Line ${i} Lorem ipsum dolor sit amet, consectetur adipiscing elit. Integer convallis fringilla sollicitudin. Nunc scelerisque neque non ipsum accumsan commodo. In et porttitor eros, ut vestibulum magna. Morbi felis diam, pharetra eu dui non, sollicitudin feugiat nisi. Aliquam cursus malesuada risus, vel luctus leo ornare sed. Morbi condimentum odio id ex facilisis bibendum. Nullam consectetur consectetur viverra. Donec nec ante dui. Integer lacinia facilisis urna vitae feugiat.`,
)

for (const line of lines) {
  stdout(line)
}
bun write.js

NOTE that the issue can only be replicated with bun - not node

Fixes #1177

@W-16645420@

Choose a reason for hiding this comment

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

LGTM. I think it makes good sense to use console.log/console.error.

Choose a reason for hiding this comment

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

Does it make sense to put a comment with the tests to provide context as to the motivation?
i.e. previous use of process.stdout/process.stderr had backpressure issues on Bun

@shetzel
Copy link
Contributor

shetzel commented Sep 4, 2024

Using the repro script I saw skipped lines without the changes. With the changes no lines were skipped.

@shetzel shetzel merged commit 317be6c into main Sep 4, 2024
86 checks passed
@shetzel shetzel deleted the mdonnalley/write branch September 4, 2024 20:11
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 this pull request may close these issues.

process.stdout buffering issue
3 participants