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: no Client API to close server connection based on Response #21978

Open
mborsz opened this issue Sep 22, 2017 · 8 comments
Open

net/http: no Client API to close server connection based on Response #21978

mborsz opened this issue Sep 22, 2017 · 8 comments
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Thinking
Milestone

Comments

@mborsz
Copy link

mborsz commented Sep 22, 2017

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

1.8

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

linux/amd64

What did you do?

I'm using http.Client that connects to HTTP/2 server. I would like to prevent reusing the connection if last response had error 500.

I tried to implement something like this, but I failed:

  • Transport.CancelRequest doesn't work for HTTP/2,
  • Request.Cancel works only on a single stream in HTTP/2 connection,
  • CloseIdleConnections won't work if the connection is in use.
  • I also tried to use httptrace.GetConn to steal net.Conn used by the request and close it, but this is against httptrace protocol.

I did small research and I can't find any way to implement such a thing.
This is a feature request to add it.

Background:
I have an environment where I have tcp load balancer with few replicas behind.
While new connections are directed only to healthy replicas, the existing ones are always attached to the same replica (even if it becomes unhealthy).

If http.Client is attached to replica which became unhealthy, reusing the same connection will cause directing request to the same unhealthy instance.
A more reasonable approach in this case is to reconnect so that load balancer can attach a healthy replica.

@ianlancetaylor ianlancetaylor changed the title No way in HTTP2 for http.Client to close its connection net/http: no way in HTTP2 for http.Client to close its connection Mar 30, 2018
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 30, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 30, 2018
@ianlancetaylor
Copy link
Contributor

CC @bradfitz @tombergan

@bradfitz
Copy link
Contributor

bradfitz commented Apr 3, 2018

I'm not sure this is even HTTP/2 specific. The net/http API doesn't really expose much or any details of the underlying connections. It's mostly a request/response-based API only.

We do have CloseIdleConnections, but that should work the same way between HTTP/1 and HTTP/2. If not, that's a bug and we should fix that, rather than add new API surface.

That said, even with CloseIdleConnections, there's no nice way to do what you're wanting with either HTTP/1 or HTTP/2, as you can't close the connection until it's idle, and by the time it's "idle", another request might've arrived on it.

@rs sent out a document about expanding the HTTP/2 connection pool interface while I was on vacation. See #17776 (comment) and that bug and linked CLs. Maybe this is another use case that design could address. @rs, thoughts?

I suppose one path could be taking advantage of the fact that net/http.(*Response).Body is an interface, and add support for some optional CloseConnection method on it that acts like Close but also nukes the connection.

@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 3, 2018
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Apr 3, 2018
@rs
Copy link
Contributor

rs commented Apr 12, 2018

If you implement your own connection pool (with the patches I proposed here), you could just remove the connection from the pool when you detect a 500 error. To close the connection, you then call CloseIfIdle in MarkComplete if the connection is not part the pool. This way you will gracefully close the discarded connections once all in-progress streams are completed.

@rsc
Copy link
Contributor

rsc commented Jun 11, 2018

Duplicate of #20977.

@rsc rsc closed this as completed Jun 11, 2018
@rs
Copy link
Contributor

rs commented Jun 11, 2018

It is not. #20977 is about server closing client connection. Here we are taking about client closing server connection.

@bradfitz
Copy link
Contributor

Oh, okay. We were triaging too quickly and I got confused about which bugs were which.

@bradfitz bradfitz reopened this Jun 11, 2018
@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jul 9, 2018
@bradfitz bradfitz changed the title net/http: no way in HTTP2 for http.Client to close its connection net/http: no Client API to close server connection based on Response Aug 6, 2018
@rsc rsc modified the milestones: Go1.12, Go1.13 Nov 14, 2018
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@jim-minter
Copy link

Bumping this thread -- it's old but still an issue.

As an HTTP client, I'm hitting a case where some HTTP server instance behind a load balancer breaks and starts returning 500s (FWIW with no body) and without the "Connection: close" header. I retry, but I end up reusing the same TCP connection to the same broken HTTP instance, so I never hit a different backend server and my retry policy is basically useless.

Obviously I need to get the server owner to fix its behavior, but it would be great if, as a client, there were a way to get net/http not to reuse the connection further, in order to be less beholden to the server's behavior.

This happens with both HTTP/1.1 and HTTP/2.

If appropriate, I could live with the request to close the connection racing with other new requests to the same endpoint. Getting to the point where 2 or 3 requests fail and then the connection is closed is way better than having requests fail ad infinitum.

http.Transport.CloseIdleConnections() doesn't solve the problem well (a) because it's a big hammer, and (b) because there's no guarantee that the connection is idle when CloseIdleConnections() is called.

FWIW I can see in func (pc *persistConn) readLoop() there's the following test:

if resp.Close || rc.req.Close || resp.StatusCode <= 199 || bodyWritable {
	// Don't do keep-alive on error if either party requested a close
	// or we get an unexpected informational (1xx) response.
	// StatusCode 100 is already handled above.
	alive = false
}

I imagine that extending that to if resp.Close || rc.req.Close || resp.StatusCode <= 199 || bodyWritable || resp.StatusCode >= 500 { might probably help this specific case, but I imagine that's an unacceptably large behavior change for the rest of the world.

I'm not sure how else this could neatly be done.

@jim-minter
Copy link

I created a thread on go-nuts about this ("net/http: no Client API to close server connection based on Response #21978") and got some useful advice.

For reference, https://go.dev/play/p/ijSN9CsQFbc is the sort of thing we're going to try out. I think it might help in our somewhat exceptional circumstance (500s like this are rare, haven't yet been able to diagnose and fix the server-side root cause, transport is bound to specific API client, etc.) but others' mileage may vary. A potentially useful hack but not a general purpose kind of thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Thinking
Projects
None yet
Development

No branches or pull requests

8 participants