net/http: Docs update for connection reuse

[FireFart]

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

go version go1.10.2 windows/amd64

Does this issue reproduce with the latest release?

yes

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

set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\firefart\AppData\Local\go-build
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\firefart\go
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\firefart\AppData\Local\Temp\go-build959778686=/tmp/go-build -gno-record-gcc-switches

Suggested Change

Currently the docs on https://golang.org/pkg/net/http/ say that you need to close the response Body after you are done with it so golang can reuse the connection. In my tests it turned out, that you have to actually consume the body before closing it otherwise the connection will not be reused.
I think it should be stated everywhere in the referenced docs where Close is mentioned that you have to consume the body too. If you do not consume the body golang will create new connections on every request.
If you aren't interested in the body, you need to do smth like

io.Copy(ioutil.Discard, resp.Body)
defer resp.Body.Close()

This blog post also describes the problem:
https://awmanoj.github.io/tech/2016/12/16/keep-alive-http-requests-in-golang/

Here is a sample program to demonstrate the problem:

package main

import (
	"flag"
	"io"
	"io/ioutil"
	"net/http"
	"sync"
	"time"
)

var (
	client = &http.Client{
		Timeout: 30 * time.Second,
	}
	discard bool
	threads int
	url     string
	wg      sync.WaitGroup
	c       chan struct{}
)

func main() {
	flag.BoolVar(&discard, "discard", false, "Consume body")
	flag.IntVar(&threads, "threads", 8, "threads")
	flag.StringVar(&url, "url", "", "url")
	flag.Parse()
	wg.Add(threads)
	c = make(chan struct{}, threads)
	for i := 0; i < threads; i++ {
		go func() {
			for range c {
				req, _ := http.NewRequest(http.MethodGet, url, nil)
				resp, err := client.Do(req)
				if err == nil {
					if discard {
						io.Copy(ioutil.Discard, resp.Body)
					}
					resp.Body.Close()
				}
			}
			wg.Done()
		}()
	}

	for i := 0; i < 200; i++ {
		c <- struct{}{}
	}
	close(c)
	wg.Wait()
}

Here is the timing output from only closing the body, and the other time with consuming it before. Each test was executed twice to make sure it's no network based problem. Also a https URL was used to demonstrate the problem because the TLS handshake takes a lot of time.

PS C:\Users\firefart\go\src\> Measure-Command { .\test.exe -url https://firefart.at }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 3
Milliseconds      : 487
Ticks             : 34877955
TotalDays         : 4,03680034722222E-05
TotalHours        : 0,000968832083333333
TotalMinutes      : 0,058129925
TotalSeconds      : 3,4877955
TotalMilliseconds : 3487,7955



PS C:\Users\firefart\go\src\> Measure-Command { .\test.exe -url https://firefart.at }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 3
Milliseconds      : 511
Ticks             : 35116646
TotalDays         : 4,06442662037037E-05
TotalHours        : 0,000975462388888889
TotalMinutes      : 0,0585277433333333
TotalSeconds      : 3,5116646
TotalMilliseconds : 3511,6646



PS C:\Users\firefart\go\src\> Measure-Command { .\test.exe -url https://firefart.at -discard }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 948
Ticks             : 9482683
TotalDays         : 1,09753275462963E-05
TotalHours        : 0,000263407861111111
TotalMinutes      : 0,0158044716666667
TotalSeconds      : 0,9482683
TotalMilliseconds : 948,2683



PS C:\Users\firefart\go\src\> Measure-Command { .\test.exe -url https://firefart.at -discard }


Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 904
Ticks             : 9042128
TotalDays         : 1,04654259259259E-05
TotalHours        : 0,000251170222222222
TotalMinutes      : 0,0150702133333333
TotalSeconds      : 0,9042128
TotalMilliseconds : 904,2128

You can also verify this behaviour using wireshark and have a look at the source port. If it changes on every request there is no connection reuse.

So it would be great if you can add this case to the net/http docs

[agnivade]

https://golang.org/pkg/net/http/#Response

The default HTTP client's Transport may not
// reuse HTTP/1.x "keep-alive" TCP connections if the Body is
// not read to completion and closed.

I believe this is sufficient, and pretty clearly mentioned.

Please let us know if you had anything else in mind or maybe you missed this part ?

[FireFart]

Here https://golang.org/pkg/net/http/#Client.Do:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

I think it would also be good on top in the examples section to put a example with io.Copy(ioutil.Discard, resp.Body) if you just do a GET request to check the return code.

[meirf]

@agnivade the text you've highlighted is clear on what needs to be done (read to completion and closed) to avoid something bad (may not reuse connection), but I think the problem is the other places where closing the body is mentioned. This is a very common complaint so I think it's worth digging deeper on this.

• Overview: "The client must close the response body when finished with it" The code that follows has ioutil.ReadAll, but "finished with it" is vague. "finished with it" could be interpreted as used it to completion OR not interested in using it all and ignoring it. So someone might think that if they aren't using the response body, then this rule doesn't apply to them.
• Client.Get: "Caller should close resp.Body when done reading from it." "done reading from it" is less vague than "finished with it", but for some reason the word "should" is used instead of "must", which weakens the point. But even if it had "must" someone might think that if they don't want to read the body, this rule doesn't apply to them. Also this rule doesn't explain the consequences of ignoring it.
• Client.Post: "Caller should close resp.Body when done reading from it."
• Client.PostForm: "Caller should close resp.Body when done reading from it."

Perhaps we've been trying to avoid duplicating:

The default HTTP client's Transport may not reuse HTTP/1.x "keep-alive" TCP connections if the Body is not read to completion and closed.

all over the docs because it's verbose and because some people might not care about connection reuse. One alternative is to to be more concise and say "the caller must read the body to completion and close" without saying why, but that's dishonest since they don't really need to in all cases.

@bradfitz am I missing any nuance here?

[agnivade]

I see.

Then this line If the Body is not closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request. in https://golang.org/pkg/net/http/#Client.Do is technically incorrect I believe. Because you need to close and read body.

[FireFart]

Exactly. I would love to see it documented in every function but it's a lot copy paste. Are there references in godocs which we can use so the phrase is only documented once?

Another option would be to change the Body.Close() method to automatically read all data before closing it but I think this would be a bigger change

[iaburton]

Didn't this "read body to completion" come up as a security issue at some point? I don't remember if it was just server or client related (or both), but basically if Go automatically read the body to completion than it would be trivial for the program to be DoS'd (cpu/memory consumption). The counterpart could just send an infinite body and the Go program would sit there consuming it all, and if you're not consuming it explicitly in your code (ioutil.Discard or otherwise) then the developer of the program might not know it was happening.

I thought I saw a commit a while back that had Go read part of the body to a certain point but if it kept going then it would close the connection abruptly, to avoid this potiental DoS. If thats the case, and you make another request while some goroutine is doing this, it would make sense that a new connection would be made (what you're seeing here and why reading the body to completion is the "solution"). In fact, if you don't read the body to completion from both a security and "HTTP" standpoint doesn't it make more sense to close the connection on (or near) body close and reopen a new connection when needed? That way it's only one connection at a time.

[as]

The default HTTP client's Transport may not reuse HTTP/1.x "keep-alive" TCP connections if the Body is not read to completion and closed.

To be precise, what does it mean for a Body to be read to completion?

If the Body is not closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection

The RFC verbiage doesn't help me understand under what conditions it may or may not reuse the connection. I suspect this was chosen because the request might return an error, but this information is already fuzzy when reading the package documentation:

# note: godoc is gone in 1.12
# it now has to be installed separately
# go get github.com/golang/tools/cmd/godoc
; godoc net/http 

package http
    import "net/http"

    Package http provides HTTP client and server implementations.

    Get, Head, Post, and PostForm make HTTP (or HTTPS) requests:

        resp, err := http.Get("http://example.com/")
        ...
        resp, err := http.Post("http://example.com/upload", "image/jpeg", &buf)
        ...
        resp, err := http.PostForm("http://example.com/form",
                url.Values{"key": {"Value"}, "id": {"123"}})

    The client must close the response body when finished with it:

        resp, err := http.Get("http://example.com/")
        if err != nil {
                // handle error
        }
        defer resp.Body.Close()
        body, err := ioutil.ReadAll(resp.Body)
        // ...

Above, the example fails to convey the careful dance required to make connections reusable. The error
handling and return is replaced by a // handle error comment that falls through to the defer.

Second, the Body is consumed and assigned to a variable called body. This doesn't show what the user needs to do if they don't care about the Body.

The number of users that do this correctly in private repositories is none. I have never seen it, nor can I remember the last time I've seen it in open source projects. I suspect it's because of the term finished, which does not imply that the body must be read in full. I don't think saying must be read is clear either, because an io.Reader isn't guaranteed to consume the Body with one call to read. The term is likely to be lost in translation.

I think the term consume is more appropriate than finished or read.

[agnivade]

paging @bradfitz. Thoughts on this ? We need some clarity on how do we make the docs more clear. In particular @meirf's comment here outlines the various places where things are not uniform.