Avoid mutating global STDOUT & STDERR

[montanalow]

The current behavior prevents apps from benefitting from STDOUT.sync = false during configuration phases, since the initial call to Event.stdio is relatively late in the boot cycle.

[nateberkopec]

I'm afraid I don't understand the use case.

This PR mostly changes how our internal logs interact with flush. This seems weird, first of all, because the whole point of sync = false is, IMO, to let Ruby do the flushing for you.

I also don't understand the PR title - how was global stdout and stderr being modified before this PR?

[montanalow]

$stdout = STDOUT by default.

This call results in setting $stdout.sync = true.
https://github.com/puma/puma/blob/master/lib/puma/events.rb#L145

In the current code path, whenever Puma::Events.stdio is called, STDOUT & STDERR will be changed to sync. sync is not the default behavior for STDOUT & STDERR so this is a side effect on a global variable that will have application wide results.

I would like to prevent this side effect, so Puma applications can run with STDOUT.sync # => false

I'm unfamiliar with the reasoning behind the current code setting sync = true, but my patch preserves the behavior of synchronized output for Events, by flushing stdout and stderr any time output is written.

[nateberkopec]

Why not just change the default IO to a new STDOUT/STDERR object, then?

E.g.

def self.stdio
    Events.new $stdout.dup, $stderr.dup
end

[nateberkopec]

That leaves the separate issue of us forcing you to use sync = true, but at least we avoid the global side effect.

[nateberkopec]

Actually, even better:

def self.stdio 
    stdout = $stdout.dup
    stdout.sync = false 
    stderr = $stderr.dup 
    stderr.sync = false
    Events.new stdout, stderr
end

then rm the sync code from Events#initialize

[montanalow]

@nateberkopec To catch DEFAULT = new(STDOUT, STDERR) as well, I moved the dup inside initialize, which I think aligns with your suggestions.

[nateberkopec]

I think that's OK, but you'll need to make sure that the rack handler changes it's arguments so that the correct item is used.

[montanalow]

Not sure I follow, are you worried about the resulting StringIO.new.dup from Rack::Events.strings?

[nateberkopec]

On that line, ::Puma::Events.stdio (which is the code path we'll take if the the silent option is NOT present) will return the global stdout/stderr. That is then passed into Puma::Launcher#new, avoiding the change that you've made here b/c the event is passed in explicitly.

[montanalow]

Sorry if I'm still missing something, but I thought that's what my change would address. By duping the args from all callers inside Events#intitialize, before sync is set, there shouldn't be any way to get update the globals, even if you pass them in.

Events.stdio will be dupped, so won't impact $stdout, and Events::DEFAULT will also be dupped to avoid mutating STDOUT, when called from Puma::Launcher#initialize here https://github.com/puma/puma/blob/master/lib/puma/launcher.rb#L48

[nateberkopec]

Well, write a test and we'll find out then 😆 You'll have to fix the tests that are looking for output at $stdout/$stderr too unfortunately.

[nateberkopec]

Looking at this again and I think you're probably write, but it does need new tests + fixes of the old ones for a merge.

[nateberkopec]

Thanks for all your work @montanalow !

[nateberkopec]

Who could have guessed the entire Puma logging ecosystem somehow depended on us modifying STDOUT.sync to true... a good lesson for software maintainership - modifying a global is probably worth a major version bump!