Avoid mutating global STDOUT & STDERR (#1837)#1
Open
MitchLewis930 wants to merge 1 commit intopr_051_beforefrom
Open
Avoid mutating global STDOUT & STDERR (#1837)#1MitchLewis930 wants to merge 1 commit intopr_051_beforefrom
MitchLewis930 wants to merge 1 commit intopr_051_beforefrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modifies the Puma::Events class to avoid mutating global STDOUT and STDERR objects by duplicating them instead of using the originals directly.
Changes:
- Updated
Puma::Events#initializeto duplicate stdout/stderr arguments instead of using them directly - Modified tests to verify that duplicated streams are used and that sync settings are respected
- Added test setup to ensure test infrastructure can access the duplicated logger outputs
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/puma/events.rb | Changed to duplicate stdout/stderr arguments to avoid mutating globals |
| test/test_events.rb | Updated assertions to compare inspected strings and added test for sync behavior |
| test/test_tcp_logger.rb | Added instance variable assignment to support test infrastructure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+31
| def test_stdio_respects_sync | ||
| STDOUT.sync = false | ||
| events = Puma::Events.stdio | ||
|
|
||
| assert !STDOUT.sync | ||
| assert events.stdout.sync |
There was a problem hiding this comment.
This test mutates the global STDOUT object by setting sync to false, which could affect other tests. The test should restore the original sync value in an ensure block to prevent side effects on other tests.
Suggested change
| def test_stdio_respects_sync | |
| STDOUT.sync = false | |
| events = Puma::Events.stdio | |
| assert !STDOUT.sync | |
| assert events.stdout.sync | |
| def test_stdio_respects_sync | |
| original_sync = STDOUT.sync | |
| STDOUT.sync = false | |
| events = Puma::Events.stdio | |
| assert !STDOUT.sync | |
| assert events.stdout.sync | |
| ensure | |
| STDOUT.sync = original_sync |
| # in lib/puma/launcher.rb:85 | ||
| # Puma::Events is default tcp_logger for cluster mode | ||
| logger = Puma::Events.new(STDOUT, STDERR) | ||
| logger.instance_variable_set(:@stdout, $stdout) # ensure capture_process_io has access to the loggers output |
There was a problem hiding this comment.
Corrected spelling of 'loggers' to 'logger's'.
Suggested change
| logger.instance_variable_set(:@stdout, $stdout) # ensure capture_process_io has access to the loggers output | |
| logger.instance_variable_set(:@stdout, $stdout) # ensure capture_process_io has access to the logger's output |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR_051