net/http: ErrorLog not generic enough to get at the net.Conn info for errors

[amalaviy]

Please answer these questions before submitting your issue. Thanks!

1. What version of Go are you using (go version)?

go1.6.2

1. What operating system and processor architecture are you using (go env)?
   GOARCH="amd64"
   GOOS="linux"
2. What did you do?
   Intentionally made TLS handshake fail, got log to standard out. Wanted to get ConnectionState() of the c.rwc to get the SNI but there is no way to get at it.

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

1. What did you expect to see?
   Better error handling in server.go using an ErrorHandler interface that gets a net.Conn and an error as arguments (in addition to a ...interface{} for additional arguments that might be useful) rather than the call to logf that uses log.Logger.
2. What did you see instead?
   English log line that doesn't have anything related to the connection in it and no possibility of using translations, or looking into the ConnectionState() tls SNI.

[amalaviy]

Something like:
type Server struct { ErrorHandler func(net.Conn, error, ...interface{}) }
added to the Server struct and having a check for ErrorHandler != nil and calling it if non-nil with the w.conn.rwc while only calling w.conn.server.logf if ErrorHandler is nil would work. We would need to add more ErrBodyNotAllowed type errors to server.go that correspond to the errors logf is used for instead of just en_us strings as well. Doing it this way doesn't break current functionality of ErrorLog (and logf), but gives us the option to do better error handling if the function is set.

[tombergan]

Would this generalize to a feature request for better tracing hooks on http.Server, similar to the client-side hooks added for go 1.7?
https://tip.golang.org/pkg/net/http/httptrace/#ClientTrace

Probably related to #3344 (comment)

[amalaviy]

If tracing hooks on http.Server are created then yes but might be overkill as to me, just calling the Logger with an English error message on errors is not a great design for a standard library as it leaves no options for translations or any deep dive into the net.Conn that errored out and simply adding a "real" handler that takes arguments like the Err code and the net.Conn so as not to break compatibility is all that is needed to fix this.

[ianlancetaylor]

Tracing hooks are already implemented for 1.7. The link above is to the current 1.7 docs. Please take a look at those docs, or try the 1.7 beta, and see if it addresses your need.

[amalaviy]

No, it does not. I am trying to get the ErrorLog handler to be more generic - basically the error logs that server.go:2259 is throwing are useless as they are in English and have no way to pass along the net.Conn that caused the issue nor an Error code that could be used to generate a localized string in the logs. My suggestion is to actually have an ErrorHandler(net.Conn, error, ...interface{}) that is called if set, and the current processing done only if the ErrorHandler isn't set. Basically, the ErrorLog interface is flawed, and I am suggesting a workaround to not break current compatibility. I want to know about the errors and want to be able to look at the net.Conn's TLS ConnectionState() and find the SNI for example to generate more informational logs and ErrorLog cannot do that.

[bradfitz]

I'm not a fan of this proposal:

 type Server struct { ErrorHandler func(net.Conn, error, ...interface{}) } 

In addition to the unclarity around the ...interface{}, I don't like providing the net.Conn (which permits reading, writing, closing). It's providing way too many internals to support long-term. And it's especially incompatible with HTTP/2, where the net.Conn is shared amongst different requests.

I'd review a more formal proposal around structured error handling in the net/http package, but this issue barely scratches the surface.

I'd want to see an audit of all uses of ErrorLog and how what each call site should do instead to provide more context around the error, providing a rich error type.

Is anybody interested in writing a proposal?

[bradfitz]

Timeout.