Avoid mutating global STDOUT & STDERR (#1837)#1
Avoid mutating global STDOUT & STDERR (#1837)#1MitchLewis930 wants to merge 1 commit intopr_051_beforefrom
Conversation
📝 WalkthroughWalkthroughPuma's Events class now duplicates provided stdout and stderr IO objects during initialization to prevent unintended shared-state side effects. Corresponding tests were updated to reflect the new object identity behavior and verify sync attribute handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 RuboCop (1.84.0)lib/puma/events.rbError: The test/test_events.rbError: The test/test_tcp_logger.rbError: The Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/test_events.rb`:
- Around line 26-32: The test test_stdio_respects_sync mutates global
STDOUT.sync and doesn't restore it; wrap the body that sets STDOUT.sync = false
and asserts behavior of Puma::Events.stdio in a begin...ensure block that
captures the original value (orig = STDOUT.sync) and restores it in ensure
(STDOUT.sync = orig) to avoid test pollution, and replace the `assert
!STDOUT.sync` assertion with the Minitest idiom `refute STDOUT.sync` for
clarity.
🧹 Nitpick comments (2)
test/test_tcp_logger.rb (1)
21-21: Fix typo and consider test fragility.The comment has a typo:
capture_process_ioshould becapture_subprocess_io.Using
instance_variable_setto override internal state is fragile—if the implementation changes the instance variable name, this test will silently break. Consider adding a brief comment explaining why this workaround is necessary, or alternatively, verify if there's a cleaner approach (e.g., testing with a custom IO object that doesn't require subprocess capture).✏️ Suggested fix for the typo
- logger.instance_variable_set(:`@stdout`, $stdout) # ensure capture_process_io has access to the loggers output + logger.instance_variable_set(:`@stdout`, $stdout) # ensure capture_subprocess_io has access to the logger's outputtest/test_events.rb (1)
21-24: Consider usingfilenofor more robust comparison.Comparing
inspectstrings works but could be fragile if Ruby's inspect format changes. A more robust approach is to compare the underlying file descriptor:♻️ Alternative using fileno
- # events.stdout is a dup, so same file handle, different ruby object, but inspect should show the same file handle - assert_equal STDOUT.inspect, events.stdout.inspect - assert_equal STDERR.inspect, events.stderr.inspect + # events.stdout is a dup, so same file descriptor, different Ruby object + assert_equal STDOUT.fileno, events.stdout.fileno + assert_equal STDERR.fileno, events.stderr.fileno
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/puma/events.rbtest/test_events.rbtest/test_tcp_logger.rb
🔇 Additional comments (1)
lib/puma/events.rb (1)
32-33: LGTM! Duplicating IO objects prevents unintended side effects on global streams.This change correctly addresses the issue of mutating shared IO objects (like
STDOUT.sync = true). The duplication ensures theEventsinstance has its own independent IO streams.One consideration: this assumes all passed IO-like objects respond to
dup. Standard IO, StringIO, and File objects support this, but custom IO-like objects may not. IfNullIOis used elsewhere, verify it inheritsdupbehavior correctly (it likely does viaObject#dup).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def test_stdio_respects_sync | ||
| STDOUT.sync = false | ||
| events = Puma::Events.stdio | ||
|
|
||
| assert !STDOUT.sync | ||
| assert events.stdout.sync | ||
| end |
There was a problem hiding this comment.
Restore global state after test to prevent test pollution.
This test modifies STDOUT.sync but doesn't restore it, which could affect subsequent tests. Add an ensure block to restore the original value.
Also, consider using refute instead of assert ! for better Minitest idiom.
🛡️ Proposed fix to restore global state
def test_stdio_respects_sync
+ original_sync = STDOUT.sync
STDOUT.sync = false
events = Puma::Events.stdio
- assert !STDOUT.sync
+ refute STDOUT.sync
assert events.stdout.sync
+ ensure
+ STDOUT.sync = original_sync
end🤖 Prompt for AI Agents
In `@test/test_events.rb` around lines 26 - 32, The test test_stdio_respects_sync
mutates global STDOUT.sync and doesn't restore it; wrap the body that sets
STDOUT.sync = false and asserts behavior of Puma::Events.stdio in a
begin...ensure block that captures the original value (orig = STDOUT.sync) and
restores it in ensure (STDOUT.sync = orig) to avoid test pollution, and replace
the `assert !STDOUT.sync` assertion with the Minitest idiom `refute STDOUT.sync`
for clarity.
2 participants
PR_051
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.