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

Restore sync=true on global stdout/stderr streams #2557

Merged

Conversation

cjlarose
Copy link
Member

@cjlarose cjlarose commented Feb 16, 2021

Description

Fixes #2545. In Puma 5.2, Puma stopped mutating the sync flag on the global STDOUT and STDERR objects. This can cause user application logs to be not flushed on every write if users were depending on this behavior. This PR restores sync behavior for those streams to the pre-5.2 behavior. We may re-introduce the change again in 6.0 with better documentation.

To verify this change, I used a Rack app that writes to stdout on every request

#stdout_test.ru
app = lambda do |env|
  $stdout.write "hello\n"
  [200, {"Content-Type" => "text/plain"}, ["Hello World"]]
end

run app

Start puma, piping its stdout to something that's not a TTY:

bundle exec ruby -Ilib bin/puma stdout_test.ru | tee puma.out

Then, send a bunch of requests

seq 100 | xargs -n 1 -I{} curl http://localhost:9292

On Puma 5.2, you can observe that no output is visible from Puma until many more requests complete.

This change introduces a config option to the config DSL, so that users can opt into making it so that puma doesn't set sync flag on the stdout and stderr streams. The default behavior is consistent with Puma's behavior prior to 5.2. In a future release (maybe 6.0), we may change the default or remove the option altogether.

mutate_stdout_and_stderr_to_sync_on_write false

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@cjlarose cjlarose force-pushed the set-sync-flags-on-stdout-stderr-again branch from ed689de to 9185bfc Compare February 16, 2021 20:31
@nateberkopec
Copy link
Member

Thanks. I started on a similar PR with some more stuff about deprecation/logging a warning, but I can just layer that on top of this.

@nateberkopec nateberkopec added the waiting-for-review Waiting on review from anyone label Feb 18, 2021
@MSP-Greg
Copy link
Member

The result of this I'm in favor of.

Re the name mutate_global_stdout_and_stderr_to_flush_on_write, when I see 'global', I think $stdout & $stderr. Also, consider removing mutate and add sync just to clarify the settings?

Maybe I'm getting too sensitive about naming, as I often choose poor names myself...

@cjlarose cjlarose marked this pull request as ready for review February 21, 2021 17:02
This isn't technically related to redirecting the STDOUT and STDERR
streams, but moving it here keeps all of the STDOUT/STDERR logic
together. It seems like a more natural place to put it.
@cjlarose cjlarose force-pushed the set-sync-flags-on-stdout-stderr-again branch from 9185bfc to a84b1fc Compare February 21, 2021 17:22
@cjlarose
Copy link
Member Author

Re the name mutate_global_stdout_and_stderr_to_flush_on_write, when I see 'global', I think $stdout & $stderr. Also, consider removing mutate and add sync just to clarify the settings?

Good point! I renamed the setting to mutate_stdout_and_stderr_to_sync_on_write.

JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
* Restore sync=true on STDOUT/STDERR streams

* Move mutation of STDOUT and STDERR streams to `redirect_io`

This isn't technically related to redirecting the STDOUT and STDERR
streams, but moving it here keeps all of the STDOUT/STDERR logic
together. It seems like a more natural place to put it.

* Add a test to ensure that STDOUT is flushed by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Puma 5.2 log not flushed
3 participants