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: connection never closes #1967
Labels
Comments
Owner changed to @bradfitz. Status changed to Accepted. |
Indeed. A dozen Readers deep and we get to src/pkg/http/transfer.go's body.Close, which contains: if _, err := io.Copy(ioutil.Discard, b); err != nil { Down this deep, it could't close the TCP connection if it wanted to, as it only contains a io.Reader, which may be any of a half dozen types of readers. (hard to find the TCP connection buried deep inside). Two possible solutions: a) push down the actual TCP connection so response.Body.Close can close it (if that's viewed as the right thing to do. I actually think it should only be closed if the response body can't be fully consumed in some wall time/bandwidth threshold, optimizing for keeping the TCP connection alive) b) add a new method to http.Transport, similar to Transport.CloseIdleConnections, but called Transport.CloseOpenConnections. So if you need such an ability, you create your own Client with its own Transport, and you force kill the whole Transport if you want to. Russ, preference? Labels changed: added pkg-http. |
I think the OP's use case was legit. (a download manager that needs to be able to pause & resume later, presumably with a Range request) We should have some answer for this. Actually I just thought of one: http://tip.goneat.org/pkg/http/#Transport -- I added an optional Transport.Dial awhile back for SOCKS proxy dialers. It could be used here to make a net.Dial wrapper that lets you do force disconnect on the underlying TCP connections. Probably..., does that work for you? |
But will that mean that the http package will never support this? I think it's a flaw to assume that every request must be entirely downloaded before it can close. --- http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html 8.1.4 Practical Considerations ... When a client or server wishes to time-out it SHOULD issue a graceful close on the transport connection. Clients and servers SHOULD both constantly watch for the other side of the transport close, and respond to it as appropriate. If a client or server does not detect the other side's close promptly it could cause unnecessary resource drain on the network. |
You are fantastic. Thank you so much for that. I spent a good day rewriting the http package so that the body struct would have a ReadCloser instead of a Reader. I should point out that I had no success. I just did a quick test supplying a custom Dial function and it did close the connection. Before going I should point to this: --- http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html 8.1.4 Practical Considerations ... When a client or server wishes to time-out it SHOULD issue a graceful close on the transport connection. Clients and servers SHOULD both constantly watch for the other side of the transport close, and respond to it as appropriate. If a client or server does not detect the other side's close promptly it could cause unnecessary resource drain on the network. --- I think you should put in the documentation somewhere that connections won't be closed. I'm sure there are some other people that have gotten into difficulties by pre-emptively closing a connection. Again, thank you. |
I've had a similar problem today and after some heavy investigation I believe I have found that connections do NOT close if Request.Close or Response.Close is true. This means that if the user wants the connection to close, or if the server requests it via "Connection: close", the connection will not be closed. To see what I mean, look at net/http/transport.go, line 568, and look at how the code proceeds with alive set to false. Concerning the OP's issue, I would say that (without actually testing it) the connection does not close even if you read to EOF. I have attached a patch which should resolve the issue. Attachments:
|
I think Gustavo already fixed this, but it was reverted due to networking problems on Windows. sslice, note that we don't use the bug tracker for patches. If you want to submit a patch, see: http://golang.org/doc/contribute.html Notably, you'll need to submit a CLA and patches are generally only accepted along with new tests. |
Thanks Brad, I am trying to submit a patch now, but I can't seem to get my changes up. http://golang.org/cl/6196045/ (james@james4k.com is me) This is the output I get: http://privatepaste.com/6df9ff46a6 I originally tried to submit my changes from another box I was testing on, but its version of Mercurial (2.2) didn't play well with the codereview extension. Trying again locally with Mercurial 1.9 it works better (no python tracebacks!), but still doesn't seem to work. |
OK, I recreated the issue and all is good. http://golang.org/cl/6201044 Still worth noting that Mercurial 2.2 breaks codereview, though! |
This issue was closed by revision ccd63c3. Status changed to Fixed. |
bradfitz
pushed a commit
that referenced
this issue
May 11, 2015
…sfully ««« backport 820ffde8c396 net/http: non-keepalive connections close successfully Connections did not close if Request.Close or Response.Close was true. This meant that if the user wanted the connection to close, or if the server requested it via "Connection: close", the connection would not be closed. Fixes #1967. R=golang-dev, rsc, bradfitz CC=golang-dev https://golang.org/cl/6201044 »»»
bradfitz
added a commit
that referenced
this issue
May 11, 2015
…lure for now ««« backport c3cbd6798cc7 net/http: fix regression and mute known test failure for now Two tests added in 820ffde8c are expected to fail until the fix for Issue 3540 goes back in (pending Windows net fixes), so make those tests just Logf for now, with a TODO to re-enable. Add a new client test. Rearrange the transport code to be more readable, and fix the bug from 820ffde8c where the persistConn was being closed before the body was fully ready. Fixes #3644 Updates #1967 (not yet fixed, but should be after Issue 3540) R=golang-dev, adg CC=golang-dev https://golang.org/cl/6211069 »»»
bradfitz
added a commit
that referenced
this issue
May 11, 2015
…client connections ««« backport 4c333000f50b net/http: fix response Connection: close, close client connections Fixes #3663 Updates #3540 (fixes it more) Updates #1967 (fixes it more, re-enables a test) R=golang-dev, n13m3y3r CC=golang-dev https://golang.org/cl/6213064 »»»
This issue was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
by probablyadam:
Attachments:
The text was updated successfully, but these errors were encountered: