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

proposal: log: flush output on Fatal and Panic methods if the destination writer has a Flush method #21751

Closed
rgooch opened this issue Sep 3, 2017 · 4 comments

Comments

@rgooch
Copy link

rgooch commented Sep 3, 2017

If the output destination for a logger is buffered, a call to one of the Fatal methods can lose logs, because os.Exit is called immediately after logging, likely before the buffer is flushed. To fix this, if the destination writer has a Flush method, then call it. Example implementation:
type flusher interface {
Flush() error
}

func (l *Logger) Fatal(v ...interface{}) {
l.Output(2, fmt.Sprint(v...))
if fl, ok := l.out.(flusher); ok {
fl.Flush()
}
os.Exit(1)
}
The Panic methods suffer from the same loss of logs, since panic is called immediately after logging, prior to the buffer being flushed. This can be solved in a similar way, calling Flush prior to calling panic.

@odeke-em odeke-em changed the title log: Flush output for Fatal and Panic methods proposal: log: flush output on Fatal and Panic methods if the destination writer has a Flush method Sep 4, 2017
@gopherbot gopherbot added this to the Proposal milestone Sep 4, 2017
@odeke-em
Copy link
Member

odeke-em commented Sep 4, 2017

@rgooch I've made a code sample that can help support your proposal with a repro at https://github.com/odeke-em/bugs/tree/master/golang/21751 or https://play.golang.org/p/V2Cz3q1AN1 or inlined below

package main

import (
	"bufio"
	"flag"
	"log"
	"os"
	"strings"
)

func main() {
	var flush bool
	flag.BoolVar(&flush, "flush", false, "if set, will flush the buffered io before exiting")
	flag.Parse()

	br := bufio.NewWriter(os.Stdout)
	logger := log.New(br, "", log.Ldate)
	logger.Printf("%s\n", strings.Repeat("This is a test\n", 5))
	if flush {
		br.Flush()
	}
	logger.Fatalf("exiting now!")
}

Without invoking flush -- the problem illustrated

$ echo -e "\033[32mCommon case, flush not invoked before Fatalf\033[00m" && go run main.go 
Common case, flush not invoked before Fatalf
exit status 1

With flush invoked -- the manual way, illustrating the solution

$ echo -e "\033[32mSupporting case, flush invoked before Fatalf\033[00m" && go run main.go --flush
Supporting case, flush invoked before Fatalf
2017/09/03 This is a test
This is a test
This is a test
This is a test
This is a test

exit status 1

@rgooch
Copy link
Author

rgooch commented Sep 4, 2017

Thanks. Note how the "exiting now!" message is not displayed in either test, which shows that the solution is needed inside the log package, since even if the application performs a Flush, the fatal message is still lost.

@odeke-em
Copy link
Member

odeke-em commented Sep 4, 2017

Totally!

@rsc
Copy link
Contributor

rsc commented Oct 16, 2017

This can't work. If you have a bufio writing to a gzip writing to an os.File, flushing the bufio will not flush the gzip and so will not actually put any of the log message into the file. This is by design - Flush doesn't chain.

@rsc rsc closed this as completed Oct 16, 2017
@golang golang locked and limited conversation to collaborators Oct 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants