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

x/net/http2: data race in request body buffer sharing #14960

Closed
bradfitz opened this issue Mar 25, 2016 · 2 comments
Closed

x/net/http2: data race in request body buffer sharing #14960

bradfitz opened this issue Mar 25, 2016 · 2 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@bradfitz
Copy link
Contributor

I suspect I introduced a data race in https://go-review.googlesource.com/20542 when I introduced the reuse of the 64k request body buffers.

From a grpc-go travis failure (grpc/grpc-go#604):

WARNING: DATA RACE
Read by goroutine 1272:
  runtime.slicecopy()
      /home/travis/.gimme/versions/go1.6.linux.amd64/src/runtime/slice.go:113 +0x0
  golang.org/x/net/http2.(*fixedBuffer).Read()
      /home/travis/gopath/src/golang.org/x/net/http2/fixed_buffer.go:29 +0x19b
  golang.org/x/net/http2.(*pipe).Read()
      /home/travis/gopath/src/golang.org/x/net/http2/pipe.go:45 +0x2a7
  golang.org/x/net/http2.(*requestBody).Read()
      /home/travis/gopath/src/golang.org/x/net/http2/server.go:1824 +0x14e
  google.golang.org/grpc/transport.(*serverHandlerTransport).HandleStreams.func3()
      /home/travis/gopath/src/google.golang.org/grpc/transport/handler_server.go:318 +0x111

Previous write by goroutine 1180:
  runtime.slicecopy()
      /home/travis/.gimme/versions/go1.6.linux.amd64/src/runtime/slice.go:113 +0x0
  golang.org/x/net/http2.(*fixedBuffer).Write()
      /home/travis/gopath/src/golang.org/x/net/http2/fixed_buffer.go:54 +0x2db
  golang.org/x/net/http2.(*pipe).Write()
      /home/travis/gopath/src/golang.org/x/net/http2/pipe.go:72 +0x268
  golang.org/x/net/http2.(*serverConn).processData()
      /home/travis/gopath/src/golang.org/x/net/http2/server.go:1307 +0x6f8
  golang.org/x/net/http2.(*serverConn).processFrame()
      /home/travis/gopath/src/golang.org/x/net/http2/server.go:1087 +0x746
  golang.org/x/net/http2.(*serverConn).processFrameFromReader()
      /home/travis/gopath/src/golang.org/x/net/http2/server.go:1039 +0xb81
  golang.org/x/net/http2.(*serverConn).serve()
      /home/travis/gopath/src/golang.org/x/net/http2/server.go:712 +0xd46
  golang.org/x/net/http2.(*Server).ServeConn()
      /home/travis/gopath/src/golang.org/x/net/http2/server.go:334 +0x1009
  google.golang.org/grpc.(*Server).serveUsingHandler()
      /home/travis/gopath/src/google.golang.org/grpc/server.go:354 +0x24f
  google.golang.org/grpc.(*Server).handleRawConn()
      /home/travis/gopath/src/google.golang.org/grpc/server.go:287 +0x577

The Read and Write calls are both protected by a mutex, but at this point it's a different mutex I suspect, with each caller having a different *pipe value.

We can't return the buffer when the stream closes, as a caller might still be using it in their ServeHTTP goroutine. Instead, we can have the ServeHTTP goroutine's exit itself return it, after first acquiring the lock and niling it out, in case the ServeHTTP goroutine retains the request.body somehow.

/cc @iamqizhao @broady
@bradfitz bradfitz self-assigned this Mar 25, 2016
@bradfitz bradfitz added this to the Unreleased milestone Mar 25, 2016
@iamqizhao
Copy link

Hi Brad,

Can we roll back this change if the fix cannot be instrumented quickly?

gopherbot pushed a commit to golang/net that referenced this issue Mar 25, 2016
@bradfitz
http2: revert part of e7da8ed to fix data race it introduced
1600a4c 
Git rev e7da8ed (CL 20542) introduced an optimization to reuse the
64k request body buffers across multiple requests. But ServeHTTP
handlers could retain them too long and cause races.

Temporarily revert the main part of that CL until a proper fix is in.

Updates golang/go#14960
Updates grpc/grpc-go#604

Change-Id: I28450e797a1d3122868214700b6ef345a0a1a47c
Reviewed-on: https://go-review.googlesource.com/21160
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

CL https://golang.org/cl/31447 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Oct 19, 2016
@bradfitz
http2: make Server reuse 64k request body buffer between requests
3bafa33 
name           old time/op    new time/op    delta
ServerPosts-4     192µs ± 1%     164µs ± 1%  -14.16%  (p=0.000 n=17+19)

name           old alloc/op   new alloc/op   delta
ServerPosts-4    69.8kB ± 0%     2.8kB ± 0%  -95.95%  (p=0.000 n=18+18)

name           old allocs/op  new allocs/op  delta
ServerPosts-4      42.0 ± 0%      40.0 ± 0%   -4.76%  (p=0.000 n=20+20)

This is a redo of git rev e7da8ed (golang.org/cl/20542) which had a race
and was later reverted in golang.org/cl/21160.

Updates golang/go#14960
Updates grpc/grpc-go#604

Change-Id: Ie216e45730dce4fc0c58f295bcbc669973145599
Reviewed-on: https://go-review.googlesource.com/31447
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Oct 19, 2017
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
Unreleased
Development

No branches or pull requests
3 participants
@bradfitz @gopherbot @iamqizhao