net/http: clarify whether Requests can be reused in HTTP/2

[radeksimko]

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

$ go version
1.16.2

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/Users/radeksimko/gopath/bin"
GOCACHE="/Users/radeksimko/Library/Caches/go-build"
GOENV="/Users/radeksimko/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/radeksimko/gopath/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/radeksimko/gopath"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/radeksimko/.goenv/versions/1.16.2"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/radeksimko/.goenv/versions/1.16.2/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/fb/k_9ystyd08j934qcxmyv0wjc0000gp/T/go-build138280913=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package main

import (
	"net/http"
	"testing"
)

func TestRepro(t *testing.T) {
	req, err := http.NewRequest("GET", "https://http2.golang.org/", nil)
	if err != nil {
		t.Fatal(err)
	}

	client := http.DefaultClient
	headResp, err := client.Do(req)
	if err == nil {
		if headResp.StatusCode == 200 {
			req.Header.Set("Range", "bytes=0-")
		}
	}
}

go test -race ./...

What did you expect to see?

Either implementation that makes it safe to reuse the http.Request in the context outlined in the above snippet where a HEAD request is sent to check whether server accepts-range and then GET is sent with relevant range header to effectively resume previously interrupted download.

Or docs clarifying that http.Request is not safe to reuse in this context.

The current wording doesn't make it clear unfortunately:

go/src/net/http/client.go

Lines 130 to 132 in 135c9f4

	// read fields of the request in a separate goroutine. Callers
	// should not mutate or reuse the request until the Response's
	// Body has been closed.

which was added as part of #19653

Even if the body is closed in the above example, the race condition doesn't go away because it seems to have a different root cause. In HTTP/2 a goroutine stays around and accesses request headers as can be seen below from the stack trace.

go/src/net/http/h2_bundle.go

Lines 9189 to 9193 in 135c9f4

	// isConnectionCloseRequest reports whether req should use its own
	// connection for a single request and then close the connection.
	func http2isConnectionCloseRequest(req *Request) bool {
	return req.Close || httpguts.HeaderValuesContainsToken(req.Header["Connection"], "close")
	}

What did you see instead?

==================
WARNING: DATA RACE
Write at 0x00c00009cd80 by goroutine 7:
  runtime.mapassign_faststr()
      /Users/radeksimko/.goenv/versions/1.16.2/src/runtime/map_faststr.go:202 +0x0
  net/textproto.MIMEHeader.Set()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/textproto/header.go:22 +0x2a4
  net/http.Header.Set()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/header.go:37 +0x1ef
  github.com/radeksimko/test.TestRepro()
      /Users/radeksimko/gopath/src/github.com/radeksimko/test/main_test.go:19 +0x1ca
  testing.tRunner()
      /Users/radeksimko/.goenv/versions/1.16.2/src/testing/testing.go:1194 +0x202

Previous read at 0x00c00009cd80 by goroutine 14:
  runtime.mapaccess1_faststr()
      /Users/radeksimko/.goenv/versions/1.16.2/src/runtime/map_faststr.go:12 +0x0
  net/http.http2isConnectionCloseRequest()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:9175 +0x1d0
  net/http.(*http2clientConnReadLoop).endStreamError()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:8805 +0x68
  net/http.(*http2clientConnReadLoop).endStream()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:8796 +0x20c
  net/http.(*http2clientConnReadLoop).processData()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:8788 +0x1e6
  net/http.(*http2clientConnReadLoop).run()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:8360 +0x4ad
  net/http.(*http2ClientConn).readLoop()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:8244 +0xbd

Goroutine 7 (running) created at:
  testing.(*T).Run()
      /Users/radeksimko/.goenv/versions/1.16.2/src/testing/testing.go:1239 +0x5d7
  testing.runTests.func1()
      /Users/radeksimko/.goenv/versions/1.16.2/src/testing/testing.go:1512 +0xa6
  testing.tRunner()
      /Users/radeksimko/.goenv/versions/1.16.2/src/testing/testing.go:1194 +0x202
  testing.runTests()
      /Users/radeksimko/.goenv/versions/1.16.2/src/testing/testing.go:1510 +0x612
  testing.(*M).Run()
      /Users/radeksimko/.goenv/versions/1.16.2/src/testing/testing.go:1418 +0x3b3
  main.main()
      _testmain.go:43 +0x236

Goroutine 14 (running) created at:
  net/http.(*http2Transport).newClientConn()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:7208 +0xcc6
  net/http.(*http2Transport).NewClientConn()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:7140 +0x79
  net/http.(*http2addConnCall).run()
      /Users/radeksimko/.goenv/versions/1.16.2/src/net/http/h2_bundle.go:867 +0x55
==================
--- FAIL: TestRepro (0.48s)
    testing.go:1093: race detected during execution of test
FAIL
FAIL	github.com/radeksimko/test	1.171s
FAIL

[seankhliao]

cc @bradfitz @tombergan @empijei

[networkimprov]

cc @fraenkel

[fraenkel]

Requests are not safe to be reused for http or http/2.

[radeksimko]

@fraenkel that was eventually my conclusion from reading up on the history in the linked issue and debugging this race condition.

Should I send a patch to reflect this conlusion here?

go/src/net/http/client.go

Lines 130 to 132 in 135c9f4

	// read fields of the request in a separate goroutine. Callers
	// should not mutate or reuse the request until the Response's
	// Body has been closed.

to say that it's just never safe?

[bradfitz]

The rules for HTTP/2 are the same as the rules for HTTP/1, as documented previously in 3039bff for #19653.

Your test code is wrong; it's the response is still active, as you didn't close the body:

	client := http.DefaultClient
	headResp, err := client.Do(req)
	if err == nil {
		if headResp.StatusCode == 200 {
			req.Header.Set("Range", "bytes=0-")
		}
	}

[seankhliao]

I still see races after adding a headResp.Body.Close() in there, though less consistently

[fraenkel]

@bradfitz I am not so sure about previous statements given both contexts that are added or trailing headers that could be added. These are two items I noticed at a glance., One would have to look at all the changes that are made to a request when it is not copied.

[radeksimko]

@bradfitz You are right that the race is no longer reproducible with the same HTTP/2 host & method when the body is closed.

However I did manage to reproduce it even with body closure:

package main

import (
	"net/http"
	"testing"
)

func TestRepro(t *testing.T) {
	req, err := http.NewRequest("HEAD", "https://releases.hashicorp.com/", nil)
	if err != nil {
		t.Fatal(err)
	}

	client := http.DefaultClient
	headResp, err := client.Do(req)
	headResp.Body.Close() // <--- closing body here
	if err == nil {
		if headResp.StatusCode == 200 {
			req.Header.Set("Range", "bytes=0-")
		}
	}
}

I do not know what difference between the two servers/requests is triggering it, but it is getting triggered reliably.