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: error messages are not introspectable #9383

Closed
lavalamp opened this issue Dec 18, 2014 · 10 comments

Comments

Projects
None yet
6 participants
@lavalamp
Copy link

commented Dec 18, 2014

I just reviewed a PR with the line case err.Error() == "http: can't write HTTP request on broken connection". That's brittle code, and it's hard to test in a way that ensures it stays in sync with any changes to the error message wording in net/http--but there's no way to improve upon it given the errors the http package emits.

Possible improments:

  • net/http could predeclare its errors
  • or make a custom error type

Any mechanism is fine as long as it lets the compiler check that people are referring to the error they intend to.

(I realize this is probably low priority.)

@smarterclayton

This comment has been minimized.

Copy link

commented Dec 18, 2014

The underlying error was #3514

EDIT: Updated, the error was similar to 3514.

go version go1.3.1 darwin/amd64

Scenario is a go http.Client talking to a go http.Server with ReadTimeout set. The server endpoint is using chunked encoding and periodically emitting JSON objects onto the response. The client has a number of open connections like this (10 or so in the trigger) to the server, and when the server terminates the connection on ReadTimeout it appears that all of the open connections are closed in a very short window, and then the clients immediately try to reconnect. The first client seems to get EOF (reusing the existing connection?), the remainder get a mix of tcp connection reset by peer and http: can't write HTTP request on broken connection.

I will create a reduced repro case for the actual 'http: can't write HTTP request'

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 18, 2014

@smarterclayton, elaborate. Which version of Go were you running and which error did you get?

@rthomas

This comment has been minimized.

Copy link

commented Dec 19, 2014

It looks like we should be using the same ProtocolError struct for this as request.go is using. This will give us compile-time safety for new users and still allow for backwards compatibility for users doing the string comparison.

I have a patch for transport.go but this should really be applied to all of http - I am waiting on internal approval to send a patch.

@smarterclayton

This comment has been minimized.

Copy link

commented Dec 19, 2014

This reproduces the error as simply as I can manage: https://play.golang.org/p/eTFgX7yah5

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 20, 2014

Related issue #9405 was just fixed, FWIW.

It is a concrete instance of this larger bug. If you want to see progress on this bug, a list of concrete examples would be best.

@bradfitz bradfitz added this to the Go1.5Maybe milestone Dec 20, 2014

@mikioh mikioh changed the title Error messages in net/http are not introspectable net/http: error messages are not introspectable Dec 20, 2014

@mikioh mikioh added the repo-main label Dec 20, 2014

@rthomas

This comment has been minimized.

Copy link

commented Dec 22, 2014

@smarterclayton my interpretation of the issue that @lavalamp reported is the usage of strings and string comparison to tell what error was thrown, not the example error shown which it looks like #9405 and #3514 are about.

@smarterclayton

This comment has been minimized.

Copy link

commented Dec 22, 2014

Yes, I was the one who wrote the string comparison code and lavalamp was the reviewer. I was providing the example in case there was disagreement whether normal use of net/http would replicate the error such that it deserved a distinct type, which it sounds like there was not.

On Dec 22, 2014, at 4:20 AM, Ryan Thomas notifications@github.com wrote:

@smarterclayton my interpretation of the issue that @lavalamp reported is the usage of strings and string comparison to tell what error was thrown, not the example error shown which it looks like #9405 and #3514 are about.


Reply to this email directly or view it on GitHub.

@lavalamp

This comment has been minimized.

Copy link
Author

commented Dec 22, 2014

Right.

If I were king, I would make these rules for library writers:

  • errors.New() is always and only used to declare public error variables in var (...) blocks.
  • If you start to use fmt.Errorf(), don't. Instead make a custom, introspectable, error type.

Breaking either rule makes it pretty much impossible for library consumers to correctly handle errors.

@rsc rsc removed the repo-main label Apr 14, 2015

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2015

Too late for Go 1.5.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 24, 2018

Closing as duplicate of #9405 (which has more detail & discussion).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.