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: Too hard to tell if a RoundTrip error came from reading from the Body or from talking to the target server #18272

Closed
glasser opened this issue Dec 10, 2016 · 3 comments

Comments

@glasser
Copy link
Contributor

glasser commented Dec 10, 2016

Please answer these questions before submitting your issue. Thanks!

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

1.7.4.

What operating system and processor architecture are you using (go env)?

Linux, Mac.

What did you do?

I'm using net/http/httputil's ReverseProxy to implement a proxy that sends requests to various backends. I give the ReverseProxy a custom transport which decides which backend to use, calls RoundTrip on a normal http.Transport, retries against other backends on errors, etc. (We do most of our work inside a custom rp.Transport so that we can retry against other backends, but I believe the issue described would apply if we used a Director instead, and wrapped the ReverseProxy inside something else that checked errors coming out of it.)

If the inner RoundTrip call returns an error, I'd like to decide whether or not this means that the backend in question is broken or not.

The problem is, in this proxying context, there are a lot of errors I can get that indicate nothing about the backend at all, but merely indicate that the incoming client closed the connection.

Specifically, I can get any of the errors "client disconnected" (http2's errClientDisconnected), "body closed by handler" (http2's errClosedBody), "http: invalid Read on closed Body" (http.ErrBodyReadAfterClose), or "stream error: stream ID 1234; CANCEL" (http2.StreamError with Code == http2.ErrCodeCancel). All of these, as far as I can tell, come from reading the Body from the client's Request as part of writing it to the backend inside my nested RoundTrip call.

In addition, I can get the error "net/http: request canceled" (http's errRequestCanceled); this happens when ReverseProxy's "notice that CloseNotify has triggered on incoming address, cancel outgoing requests" kicks in.

I currently have a really ugly block of code in my custom Transport that I pass to the ReverseProxy:

		res, err = l.transport.RoundTrip(request)
		if err == nil {
			break
		}

		// Did the client close the connection or otherwise cancel the HTTP request?
		// If so (a) It really probably doesn't matter what we return because
		// there's no client listening. (b) This is not the fault of the backend
		// we're contacting, so we shouldn't log as if it is and we *CERTAINLY*
		// shouldn't decide that the backend is unhealthy.
		switch err.Error() {
		case
			// This means that *we* canceled the outgoing request, which only happens
			// if reverseproxy.ServeHTTP does this because it notices that the
			// incoming request has closed.
			"net/http: request canceled",
			// These are HTTP/2-only errors, only created by server code, so it must
			// have come from an error on reading the request.
			"client disconnected",
			"body closed by handler",
			// HTTP/1.1 error, only created by server code, so it must have come from
			// an error on reading the request.
			"http: invalid Read on closed Body":
			return noClientResponse(request), nil
		}
		// HTTP/2 error, sent by a disconnecting client in a GOAWAY or RST_STREAM
		// message (or possibly generated synthetically inside the server itself?)
		// We're assuming here that we never use HTTP/2 when connecting to
		// backends --- ie, that this error can only have come from us reading from
		// the client's body.  But that's not actually true in general!
		// XXX If we vendor in golang.org/x/net/http2 and run http2.ConfigureServer
		//     ourself, then we have access to the http2.StreamError type and can do
		//     a type check instead of a regex match.
		if http2ErrStreamCancelRegexp.MatchString(err.Error()) {
			return noClientResponse(request), nil
		}

But this is a mess.

I'd really like there to be a straightforward way to discover whether an error from RoundTrip related to talking to the server you're trying to contact or reading from Body. And I'd like there to be a straightforward way when using httputil.ReverseProxy to tell if an issue was primarily related to talking to the backend server or communicating with the incoming message.

I am considering replacing the Body on the incoming request with an io.ReadCloser that saves any non-io.EOF that it returns to somewhere visible to my proxy, so that I can compare that error to the error that comes back from RoundTrip. (And still check for errRequestCanceled separately.) But wouldn't it be great if http.Transport.RoundTrip did it for you — if it wrapped errors from reading the body in a special error indicating that? Maybe that's not the best way to solve my problem, but it's the one that comes to mind first.

@bradfitz
Copy link
Contributor

Is this a dup of #15935 or #14203?

@bradfitz
Copy link
Contributor

Or #13667 ?

@glasser
Copy link
Contributor Author

glasser commented Dec 10, 2016

Yes, I was using the wrong keywords for my search :(

Yes, I think this is a dup of the first two here (though #13667 is also something I run into, it's still about "problems with the target server"). I'll add a note to them saying that it would be helpful to be able to differentiate between req.Body-reading errors and errors relating to the target server.

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

3 participants