Skip to content
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.

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

Check that stderr is valid before using it by default #71

Closed
wants to merge 1 commit into from

Conversation

mcsaucy
Copy link

@mcsaucy mcsaucy commented Nov 1, 2024

Windows Services are spawned without stdout and stderr, so any attempt to use them equates to referencing an invalid file Handle. stderrSink returns the error, which triggers the termination of the process. The result is that any call to glog.Error (or glog.Fatal) from a Windows Service will always crash the program.

This approach checks that os.Stderr's embedded file descriptor (Handle, for Windows) is non-NULL once during init to determine its validity and if it fails, then never registers stderrSink as a logsink.

Disabling based upon whether os.Stderr is NULL was chosen because it's precise, doesn't actually influence the result of any output and doesn't require any syscalls, but still detects the case in which stderr is unavailable on Windows.

A rejected approach here would've been to swallow the errors from stderrSink.Emit, thus sparing us program termination at the hands of our logging library. I rejected this approach because it felt like it could result in some subtle behavioral changes. For example, invoking glog.Error after os.Stderr.Close() would no longer exit the program and that felt like a non-trivial change (although I can think of no reason why one would desire this behavior).

Mirrored from cl/680817112 (google-internal)

@chressie
Copy link
Contributor

chressie commented Nov 4, 2024

Apologies, i didn't see this and already sent and merged #72. Closing this one.

@chressie chressie closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants