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

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

Closed
amalaviy opened this issue Jun 3, 2016 · 7 comments
Closed
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@amalaviy
Copy link
Contributor

amalaviy commented Jun 3, 2016

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
Copy link
Contributor Author

amalaviy commented Jun 3, 2016

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
Copy link
Contributor

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
Copy link
Contributor Author

amalaviy commented Jun 3, 2016

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
Copy link
Contributor

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
Copy link
Contributor Author

amalaviy commented Jun 3, 2016

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.

@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Jun 3, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Sep 2, 2016

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 bradfitz modified the milestones: Go1.8Maybe, Go1.8 Sep 2, 2016
@bradfitz bradfitz added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 2, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8Maybe Oct 20, 2016
@bradfitz
Copy link
Contributor

bradfitz commented Jan 6, 2017

Timeout.

@bradfitz bradfitz closed this as completed Jan 6, 2017
@golang golang locked and limited conversation to collaborators Jan 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

6 participants