net/http, x/net/http2: support http2requestBody.Close() being called multiple times concurrently

[moshegood]

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

$ go version
go version go1.17.6 darwin/amd64

Does this issue reproduce with the latest release?

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/moshe/Library/Caches/go-build"
GOENV="/Users/moshe/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/moshe/code/go/pkg/mod"
GONOPROXY="git.sqcorp.co,git.corp.squareup.com,github.com/squareup"
GONOSUMDB="git.sqcorp.co,git.corp.squareup.com,github.com/squareup"
GOOS="darwin"
GOPATH="/Users/moshe/code/go"
GOPRIVATE="git.sqcorp.co,git.corp.squareup.com,github.com/squareup"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17.6"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/usr/local/Cellar/go/1.17.6/libexec/src/go.mod"
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/k0/pqfxzl896xg7g6vq_wby46380000gn/T/go-build4218274825=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Used net/http2 with net/httputil/ReverseProxy.
Then ran a test with the -race flag.

What did you expect to see?

No race conditions.

What did you see instead?

Race conditions.

Not quite sure how hard this will be to replicate cleanly to reproduce.
Basically, the http2 portion of net/http will close a request body when it’s done.
Also, the ProxyServer portion of net/http/httputil will close a request body when it’s done.
So the request body gets closed twice.
But the http2requestBody.Close() method, while obviously coded to support being called multiple times, will run into a race condition if Close() is called twice concurrently.

Very simple fix as well.
Current code:

// requestBody is the Handler's Request.Body type.
// Read and Close may be called concurrently.
type http2requestBody struct {
	_             http2incomparable
	stream        *http2stream
	conn          *http2serverConn
	closed        bool       // for use by Close only
	sawEOF        bool       // for use by Read only
	pipe          *http2pipe // non-nil if we have a HTTP entity message body
	needsContinue bool       // need to send a 100-continue
}

func (b *http2requestBody) Close() error {
	if b.pipe != nil && !b.closed {
		b.pipe.BreakWithError(http2errClosedBody)
	}
	b.closed = true
	return nil
}

Fixed code:

// requestBody is the Handler's Request.Body type.
// Read and Close may be called concurrently.
type http2requestBody struct {
	_             http2incomparable
	stream        *http2stream
	conn          *http2serverConn
	closeOnce     sync.Once       // for use by Close only
	sawEOF        bool       // for use by Read only
	pipe          *http2pipe // non-nil if we have a HTTP entity message body
	needsContinue bool       // need to send a 100-continue
}

func (b *http2requestBody) Close() error {
	b.closeOnce.Do(func(){
		if b.pipe != nil {
			b.pipe.BreakWithError(http2errClosedBody)
		}
	})
	return nil
}

[moshegood]

Example code to reproduce:

package main

import (
	"log"
	"net/http"
	"net/http/httputil"
	"net/url"
)

func main() {
	endpoint := "https://httpstat.us/"
	url, err := url.Parse(endpoint)
	if err != nil {
		panic(err)
	}

	// Create a server on port 8000
	// Exactly how you would run an HTTP/1.1 server
	srv := &http.Server{Addr: ":8000", Handler: httputil.NewSingleHostReverseProxy(url)}

	// Start the server with TLS, since we are running HTTP/2 it must be
	// run with TLS.
	// Exactly how you would run an HTTP/1.1 server with TLS connection.
	log.Printf("Serving stuff on https://0.0.0.0:8000")
	log.Fatal(srv.ListenAndServeTLS("server.crt", "server.key"))
}

Then I can run it as:

$ GORACE="log_path=stderr" go run -race server.go

And trigger the race with:

for i in $(seq 20); do curl -k -X POST -d@random.stl --max-time 0.$i  --http2 https://localhost:8000/; done

[gopherbot]

Change https://go.dev/cl/385874 mentions this issue: net/http, x/net/http2: support http2requestBody.Close() being called multiple times concurrently

[dmitshur]

CC @neild.

[neild]

The race is a side effect of the fix for #46866, which causes ReverseProxy to always close the body of outgoing requests before returning. When the ReverseProxy handler returns early, this can result in racing closes: One by ReverseProxy.ServeHTTP and one by RoundTrip.

While the http package doesn't document that it's okay to Close a request body multiple times or concurrently, making this safe seems reasonable and simpler than finding an alternate workaround for #46866.

[gopherbot]

Change https://go.dev/cl/386495 mentions this issue: net/http, x/net/http2: support http2requestBody.Close() being called multiple times concurrently

[moshegood]

How do we get this change submitted to upstream?
PR has been reviewed, but needs a googler.

[scathers]

Now that the change has been approved, any idea on when it will be merged in?