Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/http, x/net/http2: http.readTrackingBody data race after received GOAWAY frame #41234

Closed
answer1991 opened this issue Sep 5, 2020 · 6 comments
Closed

net/http, x/net/http2: http.readTrackingBody data race after received GOAWAY frame #41234

answer1991 opened this issue Sep 5, 2020 · 6 comments
Labels
FrozenDueToAge

Comments

@answer1991
Copy link

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

$ go version
go version go1.15 linux/amd64

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=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/go/src/k8s.io/kubernetes/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 -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build142705141=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. Set up a HTTP2 server with a handler which will send GOAWAY frame
  2. Use a HTTP client to request HTTP2 server with POST method and data in multi goroutines in concurrency
  3. http.readTrackingBody cause data race

Example codes: https://github.com/answer1991/go-workspace/blob/master/pkg/goaway/goaway_test.go , run with go test -race failed:

WARNING: DATA RACE
Read at 0x00c00031ed38 by goroutine 181:
  bytes.(*Reader).Read()
      /usr/local/go/src/bytes/reader.go:41 +0x5e
  io/ioutil.(*nopCloser).Read()
      <autogenerated>:1 +0x87
  net/http.(*readTrackingBody).Read()
      /usr/local/go/src/net/http/transport.go:621 +0x8a
  golang.org/x/net/http2.(*clientStream).writeRequestBody()
      /Users/Joe/go/src/golang.org/x/net/http2/transport.go:1293 +0x761
  golang.org/x/net/http2.(*Transport).getBodyWriterState.func1()
      /Users/Joe/go/src/golang.org/x/net/http2/transport.go:2564 +0x13d

Previous write at 0x00c00031ed38 by goroutine 218:
  bytes.(*Reader).Read()
      /usr/local/go/src/bytes/reader.go:46 +0x14b
  io/ioutil.(*nopCloser).Read()
      <autogenerated>:1 +0x87
  net/http.(*readTrackingBody).Read()
      /usr/local/go/src/net/http/transport.go:621 +0x8a
  golang.org/x/net/http2.(*clientStream).writeRequestBody()
      /Users/Joe/go/src/golang.org/x/net/http2/transport.go:1293 +0x761
  golang.org/x/net/http2.(*Transport).getBodyWriterState.func1()
      /Users/Joe/go/src/golang.org/x/net/http2/transport.go:2564 +0x13d

Root Cause

Error timeline:

  1. http2 Transport read request body in a goroutine(let's named body reading goroutine):

https://github.com/golang/net/blob/62affa334b73ec65ed44a326519ac12c421905e3/http2/transport.go#L2630

  1. http2 Transport.RoundTrip return PROTOCOL_ERROR error after received GOAWAY frame, but the body reading goroutine is still on going and the request.Body(wrapped as http.readTrackingBody) has not been read.
  2. http Transport retry the request, found http.readTrackingBody.didRead == false, then re-use the http.readTrackingBody
  3. then there are 2 body reading goroutine try to read the same http.readTrackingBody instance.
@answer1991
Copy link
Author

cc @rsc @bradfitz

This was referenced Sep 5, 2020
Merged
Closed
Closed
@answer1991 answer1991 changed the title net/http, x/net/http2: http.readTrackingBody cause data race after received GOAWAY frame net/http, x/net/http2: http.readTrackingBody data race after received GOAWAY frame Sep 5, 2020
@networkimprov
Copy link

cc @fraenkel

@fraenkel
Copy link
Contributor

fraenkel commented Sep 6, 2020

dupe of #31192

There are variations of this issue even when running this test case.

@gopherbot
Copy link

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

@answer1991
Copy link
Author

@fraenkel thanks!

@andybons
Copy link
Member

andybons commented Sep 8, 2020

Duplicate of #31192

@andybons andybons marked this as a duplicate of #31192 Sep 8, 2020
@andybons andybons closed this as completed Sep 8, 2020
gopherbot pushed a commit to golang/net that referenced this issue Oct 31, 2020
@fraenkel @odeke-em
net/http2: wait until the request body has been written
ff519b6 
When the clientConn is done with a request, if there is a request body,
we must wait until the body is written otherwise there can be a race
when attempting to rewind the body.

Fixes golang/go#31192
Fixes golang/go#41234

Change-Id: I77606cc19372eea8bbd8995102354f092b8042be
Reviewed-on: https://go-review.googlesource.com/c/net/+/253259
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
@answer1991 answer1991 mentioned this issue Nov 12, 2020
Closed
@gopherbot gopherbot mentioned this issue Nov 12, 2020
Closed
@dmitshur dmitshur mentioned this issue Nov 13, 2020
Closed
@golang golang locked and limited conversation to collaborators Sep 8, 2021
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
@fraenkel @odeke-em
net/http2: wait until the request body has been written
c9647c0 
When the clientConn is done with a request, if there is a request body,
we must wait until the body is written otherwise there can be a race
when attempting to rewind the body.

Fixes golang/go#31192
Fixes golang/go#41234

Change-Id: I77606cc19372eea8bbd8995102354f092b8042be
Reviewed-on: https://go-review.googlesource.com/c/net/+/253259
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Emmanuel Odeke <emmanuel@orijtech.com>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Go Bot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@andybons @networkimprov @fraenkel @answer1991 @gopherbot