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

IO: remove flush_on_newline and only sync on TTY devices #7470

Merged

Conversation

asterite
Copy link
Member

Fixes #7431

Explanation for this here

Merging this or not is still debatable, but the pros I see are:

  • remove the strange flush_on_newline property that's actually a hack to be able to see TTY output as it's written, when sync does that in a more unified way (and works too if you call io.print). This also simplifies code because there's less code :-)
  • improve performance when you redirect the output to a file

I checked and Logger already flushes the IO when a message is written to it.

@ysbaddaden
Copy link
Contributor

Maybe I'd use a String.build in logger's default formatter, to avoid too many sync writes, but that can wait / be benchmarked to see if there is any real benefit.

src/kernel.cr Outdated Show resolved Hide resolved
@asterite asterite force-pushed the feature/stdout-sync-flush-newline branch from 27fb3f5 to e2a460a Compare February 26, 2019 13:52
@asterite asterite requested a review from bcardiff March 27, 2019 17:32
@bcardiff bcardiff merged commit e7a6e19 into crystal-lang:master Mar 29, 2019
@bcardiff bcardiff added this to the 0.28.0 milestone Mar 29, 2019
@asterite asterite deleted the feature/stdout-sync-flush-newline branch March 30, 2019 16:34
@RX14
Copy link
Contributor

RX14 commented Apr 16, 2019

This seems to me like a bad idea, since pipes are not TTYs, this will break crystal programs which are run as subprocesses and cause hangs. I suggest making STDIO sync always. If people need more performance they can always turn it off.

@RX14
Copy link
Contributor

RX14 commented May 1, 2019

I've run across this again, I've written a program which implements the i3bar protocol which streams JSON over STDOUT. It's certainly confusing that the behavior of the program is different when you run it in the terminal and when it's run by another program (and this confused me for a couple of days despite having prior knowledge of this issue).

Does anyone have any comments on my above suggestion or I'll create a PR to make STDIO always sync.

@asterite
Copy link
Member Author

asterite commented May 1, 2019

I don't understand why this is a problem in Crystal but not in Ruby.

@RX14
Copy link
Contributor

RX14 commented May 1, 2019

@asterite I've just looked at ruby and python, they take the approach from this PR: behaving differently based on whether the output is a TTY or not. So it's clearly debatable, I just think that choosing consistency over performance is better. People can always turn off STDOUT.sync = false if they hit a performance bottleneck in their code. I just think that'll be rare.

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?
6 participants