http2: avoid racy access to clientStream.requestedGzip
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
index 040d932..2cd3c6e 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -1301,6 +1301,26 @@
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 @@
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
index d875338..2171359 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -2895,6 +2895,7 @@
cc := &ClientConn{
closed: true,
reqHeaderMu: make(chan struct{}, 1),
+ t: &Transport{},
}
_, err := cc.RoundTrip(req)
if err != errClientConnUnusable {
