@bradfitz @tombergan

@bradfitz This might be the same issue with http. There is no guarantee that the bodyWriter is complete when roundtrip exits.

Here is a standalone adapted from @vadmeste's original program but here one doesn't have to manually create certificates but also it is reduced down

package main

import (
	"bytes"
	"fmt"
	"io"
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"

	"golang.org/x/net/http2"
)

func main() {
	cst := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprintf(w, "Pong")
	}))
	if err := http2.ConfigureServer(cst.Config, new(http2.Server)); err != nil {
		log.Fatalf("Failed to configure http2 server: %v", err)
	}
	cst.TLS = cst.Config.TLSConfig

	tr := &http.Transport{TLSClientConfig: cst.Config.TLSConfig}
	if err := http2.ConfigureTransport(tr); err != nil {
		log.Fatalf("Failed to configure http2 transport: %v", err)
	}
	tr.TLSClientConfig.InsecureSkipVerify = true
	cst.StartTLS()
	defer cst.Close()

	// Then finally create the HTTP client with the HTTP/2 transport.
	client := &http.Client{Transport: tr}

	for i := 0; i < 3; i++ {
		// Fill buf with data
		buf := bytes.NewBuffer(bytes.Repeat([]byte("A"), 128*1024))

		// Make a POST http call and read and discard response
		req, err := http.NewRequest("POST", cst.URL, buf)
		resp, err := client.Do(req)
		if err != nil {
			log.Fatalln(err)
		}
                if _, err := io.Copy(ioutil.Discard, resp.Body); err != nil {
			log.Fatalln(err)
		}
		resp.Body.Close()

		// Accessing buf here causes a RACE WARNING
		buf.Reset()
		fmt.Printf(".")
	}
}

@fraenkel Modifying @odeke-em 's example to use http like so:

package main

import (
	"bytes"
	"fmt"
	"io"
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
)

func main() {
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprintf(w, "Pong")
	}))
	defer srv.Close()

	client := srv.Client()

	for i := 0; i < 100; i++ {
		// Fill buf with data
		buf := bytes.NewBuffer(bytes.Repeat([]byte("A"), 128*1024))

		// Make a POST http call, read and discard response
		req, err := http.NewRequest("POST", srv.URL, buf)
		resp, err := client.Do(req)
		if err != nil {
			log.Fatalln(err)
		}
		if _, err := io.Copy(ioutil.Discard, resp.Body); err != nil {
			log.Fatalln(err)
		}
		resp.Body.Close()

		buf.Reset()
		fmt.Printf(".")
	}
}

I do not observe a data race.

The original issue is still present in gotip from go version devel +bd486c3 Sat Jun 13 16:03:19 2020 +0000 linux/amd64 and in 1.14.2.

There is no guarantee that the bodyWriter is complete when roundtrip exits.
I'm wondering what might be the fix for this. Could we make it so that the bodyWriter is complete when roundtrip exits?

@odeke-em My bad. But the TLS is also required to trigger this bug so both ts.EnableHTTP2 = true and ts.StartTLS() instead of ts.Start() in my simplified example will trigger the data race.

It took me a bit to reproduce the issue and then find the statement that covers this situation.

https://golang.org/pkg/net/http/#RoundTripper
callers wanting to reuse the body for subsequent requests must arrange to wait for the Close call before doing so.

The body isn't closed until net/http.(*http2clientStream).writeRequestBody() returns.

...must arrange to wait for the Close call...

That's interesting because as far as I can see there's no way of telling outside of the e.g. http.Post or http.Do call, whether the Close was called and that it returned.

Looking at this further, the issue is when a GOAWAY is sent from the server all outstanding streams are sent a response errClientConnGotGoAway. However, cs.writeRequestBody() may still be executing.

Good news, its easy to fix. Just have to create a test that always fails.

Change https://golang.org/cl/253259 mentions this issue: net/http2: wait until the request body has been written

@andybons

Could you please add this fix to next milestone, as Kubernetes affected.

Could you please add this fix to next milestone, as Kubernetes affected.

@rsc

@odeke-em are you available to review the above CL?

/cc @neild

We had a request to backport this, as it impacts Kubernetes, per @answer1991.

@dmitshur what do you think about the requested backport from @answer1991 and @networkimprov per #31192 (comment)?

Glad to see the fix merged.
As Kubernetes now is using golang 1.15.x, it would be fine if we backport the fix to 1.15.

I don't have enough information to know the severity of impact caused by this problem and whether there's a reasonable workaround. We need that information to balance it against the risk of backporting a commit when making a decision.

@answer1991 The process for requesting a backport is described at https://golang.org/wiki/MinorReleases. If after reading it you think this issue should be considered for backporting, please use gopherbot to make a request and provide a brief rationale for it. I know you've said this impacts Kubernetes, but please provide a little more detail about how serious it is. Thanks.

CC @golang/release.

Kubernetes team had already announced GOAWAY feature in Kubernetes v1.19 release notes. However, if Kubernetes enable GOAWAY feature, the client will receive unexpected 500 status code errors which I described at #41234, and there is no workaround. I think backport to golang 1.15 is necessary.

@gopherbot please backport, as Kubernetes is affected and there is no workaround.

Backport issue(s) opened: #42539 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@dmitshur @toothrot I think this needs backport to 1.14 as well?

Yep, since the issue affects both Go 1.15 and 1.14, it needs to be considered for backport to both, or neither. (We typically double-check that as part reviewing backport requests, but it's helpful to also catch it earlier.)

I don't know why the bot didn't create a cherry-pick to Go 1.14. Because I mentioned 1.15 in my comment? Re-try

Kubernetes team had already announced GOAWAY feature in Kubernetes v1.19 release notes. However, if Kubernetes enable GOAWAY feature, the client will receive unexpected 500 status code errors which I described at #41234, and there is no workaround. I think backport is necessary.

@gopherbot please backport, as Kubernetes is affected and there is no workaround.

Backport issue opened: #42586 (for 1.14).

(Follow up requests are not supported, so they need to be created manually until #25574 is resolved.)

thanks!

Reopening as the net/http issue isn't fixed in Go 1.16 yet.

This issue got closed because CL 253259 was submitted with the phrase "Fixes #31192". However that CL is for x/net. The bundled copy of golang.org/x/net/http2 inside net/http still needs to be updated to include the fix.

Doing the bundle update was attempted, but it caused an existing net/http test to fail. This is #42498. That issue needs to be resolved.

#42498 has been resolved (I believe), and should no longer block doing the bundle update.

Should we backport #42498 too?

Change https://golang.org/cl/275446 mentions this issue: all: update dependencies with changes in scope for Go 1.16 freeze

Change https://golang.org/cl/277012 mentions this issue: std: update golang.org/x/net to 20201209123823-ac852fbbde11