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

Avoid mutating $stdout and $stderr #2486

Merged
merged 2 commits into from Dec 2, 2020

Conversation

karloscodes
Copy link
Contributor

@karloscodes karloscodes commented Nov 15, 2020

Description

Closes #1948

Uses flush after writing to stdout and stderr so we don't need to use sync=true

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 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.

@nateberkopec nateberkopec added feature waiting-for-changes Waiting on changes from the requestor labels Nov 16, 2020
@nateberkopec
Copy link
Member

May want to rebase against master as Greg just fixed an issue where we were using puts instead of stdout

@karloscodes karloscodes changed the title Avoid mutating stdin, stdout and stderr Avoid mutating $stdout and $stderr Nov 18, 2020
@karloscodes karloscodes force-pushed the 1948-dont-mutate-stdin-out branch 4 times, most recently from 2d4d339 to 0f278af Compare November 18, 2020 10:11
@karloscodes
Copy link
Contributor Author

I have tried with different apps locally and on Heroku and everything works fine. Maybe someone has a clear case that should be tested with this change? Ideas?

@karloscodes karloscodes marked this pull request as ready for review November 18, 2020 10:40
@karloscodes karloscodes force-pushed the 1948-dont-mutate-stdin-out branch 2 times, most recently from 52a563a to bdfb27a Compare November 18, 2020 10:51
Uses `flush` after every write if the stdio it is not synchronized.

Closes: puma#1948
@karloscodes
Copy link
Contributor Author

Also, I have mixed feelings now because I introduced some duplication, do you think it's a good idea to encapsulate the new writing logic for the code outside events and error_logger?

@nateberkopec
Copy link
Member

Re: correctness, anything from #1906 could be extracted as a test

@nateberkopec
Copy link
Member

You can leave the refactor/DRYness for another PR if you want, don't let it block this feature.

@nateberkopec
Copy link
Member

Ideas for test re: comments in that thread:

output is no longer being flushed before Process.daemon calls fork

@karloscodes
Copy link
Contributor Author

karloscodes commented Nov 26, 2020

I've been checking the current integration tests and it seems that

def test_write_to_log
already covers #1906 am I missing something? I thought that maybe the same tests but with workers could be helpful to cover the issue we had with Process.daemon forks, WDYT?

@nateberkopec nateberkopec added waiting-for-review Waiting on review from anyone and removed waiting-for-changes Waiting on changes from the requestor labels Nov 30, 2020
@nateberkopec nateberkopec merged commit a284179 into puma:master Dec 2, 2020
@nateberkopec
Copy link
Member

Took another look, I think this is good as written

JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
Uses `flush` after every write if the stdio it is not synchronized.

Closes: puma#1948

Co-authored-by: Nate Berkopec <nate.berkopec@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't mutate global STDOUT/STDERR
3 participants