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: connection never closes #1967

Closed
gopherbot opened this issue Jun 17, 2011 · 12 comments
Closed

net/http: connection never closes #1967

gopherbot opened this issue Jun 17, 2011 · 12 comments

Comments

@gopherbot
Copy link

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
Copy link
Contributor

rsc commented Jun 17, 2011

Comment 1:

Owner changed to @bradfitz.

Status changed to Accepted.

@bradfitz
Copy link
Contributor

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
Copy link
Contributor

rsc commented Jun 21, 2011

Comment 3:

I think it's okay to say WorkingAsIntended.

@bradfitz
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

rsc commented Dec 9, 2011

Comment 7:

Labels changed: added priority-later.

@james4k
Copy link
Contributor

james4k commented May 4, 2012

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
Copy link
Contributor

bradfitz commented May 4, 2012

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
Copy link
Contributor

james4k commented May 4, 2012

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
Copy link
Contributor

james4k commented May 4, 2012

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
Copy link
Contributor

Comment 13:

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

»»»
@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
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

4 participants