Is it necessary to consume response body before closing it (net/http client code)?

transcript

mholt [9:10 AM] 
When using http.Get(), is it really necessary to read the full response body just to close it later?

[9:10] 
The docs keep saying `Caller should close resp.Body when done reading from it.` and I keep seeing code like this:
```
io.Copy(ioutil.Discard, resp.Body)
resp.Body.Close()
```

nithin [9:12 AM] 
does this have to do with that too many open files bug?

[9:12] 
i thought that it was fixed in 1.4

hydroflame [9:13 AM] 
I wouldn’t think so

[9:13] 
Wouldn’t that be a bug ?

dgryski [9:14 AM] 
mholt: the io.Copy() drains the body meaning it can be reused via keepalive

[9:14] 
if there's still data pending, the Close() will actually close it and it can't be reused

mholt [9:14 AM] 
I thought that was only needed with servers

dgryski [9:14 AM] 
mholt: Your client has keep-alive and can reuse connections if possible

mholt [9:15 AM] 
Ohhh cool. So reading the whole body then closing will allow it to be re-used, even though it's closed... got it... :confused:

[9:15] 
Wait, did I miss something? :smile:

dgryski [9:15 AM] 
Close() is closing the request, not the connection.

mholt [9:16 AM] 
Okay. I guess I don't understand why it can be reused only if fully read

[9:16] 
if the request is closed either way

dgryski [9:16 AM] 
mholt because there's still data on the wire waiting to be read

mholt [9:18 AM] 
Ohhhh. Okay, so it keeps the TCP connection open but tells the server to flush its response buffer

[9:18] 
Thanks :simple_smile:

peterbourgon [9:21 AM] 
It's fine to close a body without reading it... right?

[9:22] 
@mholt: @dgryski: ^^ like this doesn't cause any problems AFAIK

mholt [9:22 AM] 
Maybe it's just a matter of efficiency?

dgryski [9:24 AM] 
If you close a body without reading it it won't be reused.

peterbourgon [9:24 AM] 
@dgryski: define "it"?

[9:24] 
(the second "it")

dgryski [9:25 AM] 
The TCP connection won't  be reused if the body hasn't been read until eof (edited)

patrickb [9:26 AM] 
Matt, it takes some digging but in most cases you don't heed to read the body... resp.Body.Close() will drain for you.
The key is that Close() has to be called if you want it reused (quickly)

dgryski [9:28 AM] 
Close draining the body doesn't make any sense. If there is a 4 gigabyte upload that I don't want I want that connection closed, not drained.

patrickb [9:29 AM] 
It drains up to a limit...

[9:30] 
..and we're talking about responses Damian..

peterbourgon [9:30 AM] 
@dgryski: uh, are you sure?

mholt [9:31 AM] 
Hmm, maybe we need to go dig in the std lib. I'm headed out the door in a minute but might look into it later

dgryski [9:54 AM] 
@peterbourgon now I'm second guessing :/

[9:54] 
Will need to read the code

peterbourgon [9:54 AM] 
@dgryski: I've never once intentionally drained a response body

[9:54] 
But maybe I've been skirting by on luck

dgryski [9:55 AM] 
Me neither; but you can also read to eof by normal use

stabbycutyou [9:57 AM] 
you can read n bytes and no eof, then 0 bytes and an eof, or n bytes and an eof

[9:57] 
it’s a fun little edgecase

patrickb [10:24 AM] 
back.  I'll dig up the stdlib code.. gimme a sec.

patrickb [10:34 AM] 
https://github.com/golang/go/blob/master/src/net/http/transfer.go#L777

[10:36] 
but ultimately, the most it will auto-consume from the  response body is:
```go
// maxPostHandlerReadBytes is the max number of Request.Body bytes not
// consumed by a handler that the server will read from the client
// in order to keep a connection alive.  If there are more bytes than
// this then the server to be paranoid instead sends a "Connection:
// close" response.
//
// This number is approximately what a typical machine's TCP buffer
// size is anyway.  (if we have the bytes on the machine, we might as
// well read them)
const maxPostHandlerReadBytes = 256 << 10
```

[10:40] 
iow: 256KB... so if you expect responses near that size than that then you should explicitly consume them.
I think the notion of having to consume the response body should be better documented though.  I guess the assumption (prob not a bad one I suppose) is that if you have large responses then it's something you actually care about and are going to be explicitly reading.  If it's a simple get request that you just need to know if it succeeds or fails then the response body is likely to be so small as to not matter and as long as you at least Close() the body you're fine.

patrickb [11:11 AM] 
ah, but it only does partial discard if doEarlyClose is set (which has to be done explicitly)
so... by default it will consume the entire response upon Close()

mholt [11:16 AM] 
@patrickb: But doesn't that only apply to servers?

patrickb [11:17 AM] 
from what I saw, they're what ​*set*​ that value, yes.

mholt [11:17 AM] 
Now I really wanna know more about this :smile: (edited)

patrickb [11:18 AM] 
(in http/server.go)
    req.RemoteAddr = c.remoteAddr
    req.TLS = c.tlsState
    if body, ok := req.Body.(*body); ok {
        body.doEarlyClose = true
    }

[11:20] 
it's these sort of nuggets that are pretty under-documented and frankly, even following through a simple get request call and determining its code path, I think, takes some gymnastics (and good tagging).

mholt [11:24 AM] 
hmm, interesting.

patrickb [11:24 AM] 
...like getting the point of finding even what was handling the Body (io.ReadCloser) interface took a while to find (to see what was happening when Close() was called on the response body)..
it's an interesting rabbit hole actually.

mholt [11:24 AM] 
So if I do a Close() it finishes downloading 256 KB of content

patrickb [11:29 AM] 
once you actually ​*find*​ the code, it's straight-forward, but http is not a 'simple' package.
ultimately you end up in readTransfer in http/transfer.go and it is what ultimately assigns to the Body member.. (with various conditions for dir reader types) but then you see the normal response body is really a 'body' struct in transfer.go and then the Close() impl is fairly explicit.
...but anyway, if you do a Close() then it will finish reading ALL content:
    default:
        // Fully consume the body, which will also lead to us reading
        // the trailer headers after the body, if present.
        _, err = io.Copy(ioutil.Discard, bodyLocked{b})
    }
unless doEarlyClose is set.

mholt [11:30 AM] 
@patrickb: Oh wow, so it will download all 4 GB of the response even if I don't want it.

[11:30] 
(Unless setting doEarlyClose I guess)

patrickb [11:31 AM] 
as I read it, yes.

mholt [11:31 AM] 
@patrickb: You should write a blog post about this. I think a lot of people will find it interesting/helpful.
1  

dgryski [11:31 AM] 
That sounds broken to me

new messages
patrickb [11:31 AM] 
It is a pretty twisted path to get there to be honest so I may be missing something...  Brad Fitzpatrick would be the best one to ask.

dgryski [11:32 AM] 
Yes;  definitely blog.  Get golang-dev to check

patrickb [11:32 AM] 
<-- not the blogging type.. sorry.

dgryski [11:33 AM] 
Just write up your findings in a gist

patrickb [11:35 AM] 
You can't get me to shut the heck up in person a lot of times but the blog thing never resonated with me...  just very, very time consuming and I'm always busy enough with my job [which I should be getting back to!!!]
I'm always eternally thankful for the amazing blogs others put up so the irony isn't lost on me but, well...   that time thing..

pcasaretto [11:36 AM] 
I had the feeling that this auto drain on Close had been implemented, and then rolled back

patrickb [11:37 AM] 
I just happened to go down this exact rabbit hole yesterday(?) when I had a 
io.Copy(ioutil.Discard, getResp.Body)
..Close() pair and was like..  this seems so silly to have to do that.. shouldn't Close do this for me if there's stuff left over!?  and it took entirely too long (for my tastes) to get the answer.

mholt [11:37 AM] 
I feel like this chat has been my most beneficial one today :simple_smile:

[11:38] 
So, thanks.
1  

[11:38] 
But I'm gonna copy this chat into a gist so somebody can write about it

TLDR;
No, you don't need to consume response from http client, Close() automatically do that.
For server, if request more than 256KiB, it won't reuse the connection unless you consume the request body

Experimentally, that's not the case - I have code that only closes the Response body (response bodies have a Content-Length and are about 57 bytes in size), and in go 1.11 the transport does close the connection instead of re-using it. Reading the whole response body makes it keep the idle connection around.

What is the resolution from @asf-stripe? Looks like another discussion on the same issue from a while ago. Presumably, it should be fixed but still not sure -> google/go-github#317