Skip to content

Avoid mutating global STDOUT & STDERR (#1837) #1

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

Open
MitchLewis930 wants to merge 1 commit into
base: pr_051_before
Choose a base branch
from
Open

Avoid mutating global STDOUT & STDERR (#1837) #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/puma/events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ def call(str)
#
def initialize(stdout, stderr)
@formatter = DefaultFormatter.new
@stdout = stdout
@stderr = stderr
@stdout = stdout.dup
@stderr = stderr.dup

@stdout.sync = true
@stderr.sync = true
Expand Down
14 changes: 11 additions & 3 deletions test/test_events.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ def test_null

assert_instance_of Puma::NullIO, events.stdout
assert_instance_of Puma::NullIO, events.stderr
assert_equal events.stdout, events.stderr
end

def test_strings
Expand All @@ -19,8 +18,17 @@ def test_strings
def test_stdio
events = Puma::Events.stdio

assert_equal STDOUT, events.stdout
assert_equal STDERR, events.stderr
# 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
end

def test_stdio_respects_sync
STDOUT.sync = false
events = Puma::Events.stdio

assert !STDOUT.sync
assert events.stdout.sync
end
Comment on lines +26 to 32
Copy link

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


def test_register_callback_with_block
Expand Down
1 change: 1 addition & 0 deletions test/test_tcp_logger.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ def test_events
# 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
out, err = capture_subprocess_io do
Puma::TCPLogger.new(logger, @server.app).call({}, @socket)
end
Expand Down