http2: avoid racy access to clientStream.requestedGzip

neild · neild · commit 410d19ee5650 · 2024-05-21T22:20:24.000Z
clientStream.requestedGzip is set from clientStream.writeRequest, and examined by clientConn.readLoop. I'm not sure if there's any possible way for an actual data race to happen here in practice, since the read loop should only examine the field after the request is sent by writeRequest, but it's enough for the race detector to complain. Set the field in ClientConn.roundTrip instead, before the clientStream has become accessible to any other goroutines. No test, but a following CL has race detector failures without this change. Change-Id: Id30f1b95bcfcc35c213440e0e47cce3feaaff06d Reviewed-on: https://go-review.googlesource.com/c/net/+/586245 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/http2/transport.go b/http2/transport.go
@@ -1301,6 +1301,26 @@ func (cc *ClientConn) roundTrip(req *http.Request, streamf func(*clientStream))
 		donec:                make(chan struct{}),
 	}
 
+	// TODO(bradfitz): this is a copy of the logic in net/http. Unify somewhere?
+	if !cc.t.disableCompression() &&
+		req.Header.Get("Accept-Encoding") == "" &&
+		req.Header.Get("Range") == "" &&
+		!cs.isHead {
+		// Request gzip only, not deflate. Deflate is ambiguous and
+		// not as universally supported anyway.
+		// See: https://zlib.net/zlib_faq.html#faq39
+		//
+		// Note that we don't request this for HEAD requests,
+		// due to a bug in nginx:
+		//   http://trac.nginx.org/nginx/ticket/358
+		//   https://golang.org/issue/5522
+		//
+		// We don't request gzip if the request is for a range, since
+		// auto-decoding a portion of a gzipped document will just fail
+		// anyway. See https://golang.org/issue/8923
+		cs.requestedGzip = true
+	}
+
 	go cs.doRequest(req, streamf)
 
 	waitDone := func() error {
@@ -1449,26 +1469,6 @@ func (cs *clientStream) writeRequest(req *http.Request, streamf func(*clientStre
 		streamf(cs)
 	}
 
-	// TODO(bradfitz): this is a copy of the logic in net/http. Unify somewhere?
-	if !cc.t.disableCompression() &&
-		req.Header.Get("Accept-Encoding") == "" &&
-		req.Header.Get("Range") == "" &&
-		!cs.isHead {
-		// Request gzip only, not deflate. Deflate is ambiguous and
-		// not as universally supported anyway.
-		// See: https://zlib.net/zlib_faq.html#faq39
-		//
-		// Note that we don't request this for HEAD requests,
-		// due to a bug in nginx:
-		//   http://trac.nginx.org/nginx/ticket/358
-		//   https://golang.org/issue/5522
-		//
-		// We don't request gzip if the request is for a range, since
-		// auto-decoding a portion of a gzipped document will just fail
-		// anyway. See https://golang.org/issue/8923
-		cs.requestedGzip = true
-	}
-
 	continueTimeout := cc.t.expectContinueTimeout()
 	if continueTimeout != 0 {
 		if !httpguts.HeaderValuesContainsToken(req.Header["Expect"], "100-continue") {
diff --git a/http2/transport_test.go b/http2/transport_test.go
@@ -2895,6 +2895,7 @@ func TestRoundTripDoesntConsumeRequestBodyEarly(t *testing.T) {
 	cc := &ClientConn{
 		closed:      true,
 		reqHeaderMu: make(chan struct{}, 1),
+		t:           &Transport{},
 	}
 	_, err := cc.RoundTrip(req)
 	if err != errClientConnUnusable {
