net/http: connection never closes

[gopherbot]

by probablyadam:

What steps will reproduce the problem?
1. Create http.Request with Close set to true.
2. Download file.
3. Close http.Response.Body before reaching EOF.

What is the expected output?

Connection closes with http.Response.Body.Close().

What do you see instead?

Connection remains open.

Which compiler are you using (5g, 6g, 8g, gccgo)?

8g

Which operating system are you using?

linux 386

Which revision are you using?  (hg identify)

001f5f776015

Please provide any additional information below.

Run the attached code and use netstat to verify that the connection remains open.

Attachments:

1. x.go (1795 bytes)

[rsc]

Comment 1:

Owner changed to @bradfitz.

Status changed to Accepted.

[bradfitz]

Comment 2:

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.

[rsc]

Comment 3:

I think it's okay to say WorkingAsIntended.

[bradfitz]

Comment 4:

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?

[gopherbot]

Comment 5 by probablyadam:

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.

[gopherbot]

Comment 6 by probablyadam:

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.

[rsc]

Comment 7:

Labels changed: added priority-later.

[james4k]

Comment 9:

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:

1. issue1967.patch (487 bytes)

[bradfitz]

Comment 10:

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.

[james4k]

Comment 11:

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.

[james4k]

Comment 12:

OK, I recreated the issue and all is good. http://golang.org/cl/6201044
Still worth noting that Mercurial 2.2 breaks codereview, though!

[bradfitz]

Comment 13:

This issue was closed by revision ccd63c3.

Status changed to Fixed.