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

Add IO::Buffered#flush_on_newline back and set it to true for non-tty #8935

Merged
merged 3 commits into from
Mar 25, 2020

Conversation

asterite
Copy link
Member

@asterite asterite commented Mar 24, 2020

Reverts #7470
Fixes #7431 (somehow)

Before #7470 if you output to the console and redirected that output to somewhere else, for example a file, then you would see output right after each line was emitted.

Once we merged #7470 you would only see output after the IO buffer was full and it was flushed.

That meant that if you did tail -f on that file you wouldn't see new lines appended to it immediately. I believe this is not very intuitive. If you don't output enough data you might think something is broken and lose a lot of time debugging this.

This PR reverts the behavior back to how it was before.

This is slower, but you still have the option to make is faster (if you don't care about not seeing "correct" output using tail -f) by doing:

STDOUT.flush_on_newline = false

If we don't add this method back, the only options you have are:

  • fully buffer the output: fast, but not very intuitive
  • no buffer: very slow, but intuitive

With flush_on_newline it's not fast but it's also not very slow. So I think this is a good default.

💭 We could also consider having an environment variable, for example CRYSTAL_STDOUT_FLUSH_ON_NEWLINE so you can control, at runtime, how the program behaves (on startup). That means that you can compile your program once and control how it behaves later on. But I didn't include this here. We can discuss it in a different issue if needed, and do it in a separate PR.

Please 👍 if you agree with this change, 👎 if not.

src/io/file_descriptor.cr Outdated Show resolved Hide resolved
@asterite
Copy link
Member Author

Let's go forward with this. It's been like this for years, and we only had one complaint and then I changed it. And there's always a way to go back to the old behavior (by changing your code).

@asterite asterite merged commit e200fce into master Mar 25, 2020
@asterite asterite added this to the 0.34.0 milestone Mar 25, 2020
@asterite asterite deleted the bring-back-flush-on-newline branch March 25, 2020 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should STDOUT always set flush_on_newline by default?
4 participants