Skip to content

Avoid mutating $stdout and $stderr #2486

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.

Sign up for GitHub

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

Jump to bottom
Merged
nateberkopec merged 2 commits into from
Dec 2, 2020
Merged

Avoid mutating $stdout and $stderr #2486

nateberkopec merged 2 commits into from
Dec 2, 2020

Conversation

@karloscodes
Copy link
Contributor

@karloscodes karloscodes commented Nov 15, 2020 •
edited
Loading

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.

@karloscodes karloscodes force-pushed the 1948-dont-mutate-stdin-out branch from 9488cd3 to 89fc1e3 Compare November 15, 2020 12:23
dentarg
dentarg reviewed Nov 15, 2020
View reviewed changes
dentarg
dentarg reviewed Nov 15, 2020
View reviewed changes
@nateberkopec nateberkopec added feature waiting-for-changes Waiting on changes from the requestor labels Nov 16, 2020
@nateberkopec
Copy link
Member

nateberkopec commented Nov 17, 2020

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

@karloscodes karloscodes force-pushed the 1948-dont-mutate-stdin-out branch from 89fc1e3 to 6de31e4 Compare November 18, 2020 09:38
@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

karloscodes commented Nov 18, 2020

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
Avoid mutating stdout and stderr
f519468 
Uses `flush` after every write if the stdio it is not synchronized.

Closes: puma#1948
@karloscodes karloscodes force-pushed the 1948-dont-mutate-stdin-out branch from bdfb27a to f519468 Compare November 18, 2020 11:02
@karloscodes
Copy link
Contributor Author

karloscodes commented Nov 18, 2020

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

nateberkopec commented Nov 18, 2020

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

@nateberkopec
Copy link
Member

nateberkopec commented Nov 18, 2020

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

@nateberkopec
Copy link
Member

nateberkopec commented Nov 19, 2020

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 •
edited by dentarg
Loading

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

puma/test/test_integration_single.rb

Line 126 in 441c474

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

nateberkopec commented Dec 2, 2020

Took another look, I think this is good as written

@nateberkopec nateberkopec mentioned this pull request Feb 2, 2021
Closed
@dependabot dependabot bot mentioned this pull request Mar 8, 2021
Merged
This was referenced Mar 8, 2021
Merged
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Closed
This was referenced Mar 15, 2021
Closed
Open
Closed
Closed
Closed
Closed
Closed
Closed
Merged
Closed
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
@nateberkopec @JuanitoFatas
Avoid mutating stdout and stderr (puma#2486)
dd10138 
Uses `flush` after every write if the stdio it is not synchronized.

Closes: puma#1948

Co-authored-by: Nate Berkopec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dentarg dentarg dentarg left review comments

@nateberkopec nateberkopec nateberkopec left review comments

Assignees

No one assigned

Labels

feature waiting-for-review Waiting on review from anyone

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Don't mutate global STDOUT/STDERR

3 participants

@karloscodes @nateberkopec @dentarg