Code review - Issue 6640057: code review 6640057: net/http: fix race on bodyEOFSignal.isClosedhttps://codereview.appspot.com/2012-10-11T21:33:14+00:00rietveld
Message from unknown
2012-10-11T04:02:10+00:00dfcurn:md5:dcb7477c7fb96ddb9f791ccb7a2a3e88
Message from unknown
2012-10-11T04:02:15+00:00dfcurn:md5:1f2137c1adeda627f6040547f5ff9d9b
Message from unknown
2012-10-11T04:04:33+00:00dfcurn:md5:ee980dee989266052098a780ecfb3d39
Message from dave@cheney.net
2012-10-11T04:04:39+00:00dfcurn:md5:96825169d786defd3f75eb13516be5c4
Hello dvyukov@google.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go
Message from bradfitz@golang.org
2012-10-11T04:09:41+00:00bradfitzurn:md5:f106f3bc106c813e88c9b96954fe643a
The more concerning thing is that Close was called on two goroutines to
begin with.
I would think that if the original design expected that, it would've used a
mutex or atomic already. Which makes me think somebody is calling Close
who shouldn't.
I don't remember this code well, though. Let me stare at this more
tomorrow.
On Wed, Oct 10, 2012 at 9:04 PM, <dave@cheney.net> wrote:
> Reviewers: dvyukov, bradfitz,
>
> Message:
> Hello dvyukov@google.com, bradfitz@golang.org (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net/http: fix race on bodyEOFSignal.isClosed
>
> Fixes unreported race failure at
> http://build.golang.org/log/**61e43a328fb220801d3d5c88cd9191**6cfc5dc43c<http://build.golang.org/log/61e43a328fb220801d3d5c88cd91916cfc5dc43c>
>
> Please review this at http://codereview.appspot.com/**6640057/<http://codereview.appspot.com/6640057/>
>
> Affected files:
> M src/pkg/net/http/transport.go
>
>
> Index: src/pkg/net/http/transport.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/transport.**go
> +++ b/src/pkg/net/http/transport.**go
> @@ -24,6 +24,7 @@
> "os"
> "strings"
> "sync"
> + "sync/atomic"
> "time"
> )
>
> @@ -816,13 +817,13 @@
> type bodyEOFSignal struct {
> body io.ReadCloser
> fn func()
> - isClosed bool
> + isClosed uint32 // atomic bool, non zero if true
> once sync.Once
> }
>
> func (es *bodyEOFSignal) Read(p []byte) (n int, err error) {
> n, err = es.body.Read(p)
> - if es.isClosed && n > 0 {
> + if es.closed() && n > 0 {
> panic("http: unexpected bodyEOFSignal Read after Close;
> see issue 1725")
> }
> if err == io.EOF {
> @@ -832,10 +833,10 @@
> }
>
> func (es *bodyEOFSignal) Close() (err error) {
> - if es.isClosed {
> + if !es.setClosed() {
> + // already closed
> return nil
> }
> - es.isClosed = true
> err = es.body.Close()
> if err == nil {
> es.condfn()
> @@ -849,6 +850,14 @@
> }
> }
>
> +func (es *bodyEOFSignal) closed() bool {
> + return atomic.LoadUint32(&es.**isClosed) > 0
> +}
> +
> +func (es *bodyEOFSignal) setClosed() bool {
> + return atomic.CompareAndSwapUint32(&**es.isClosed, 0, 1)
> +}
> +
> type readFirstCloseBoth struct {
> io.ReadCloser
> io.Closer
>
>
>
Message from bradfitz@golang.org
2012-10-11T04:19:23+00:00bradfitzurn:md5:46eeb969fc6664e438606fb3b9e76eb6
LGTM
okay, I looked. I'm happy with this.
It's just as reasonable for line 575 (lastBody.Close() // assumed idempotent) and a user goroutine to race as it is for two user goroutines to race. And we should be correct in either case, even if two concurrent Closes from user goroutines doesn't make any sense. The one on line 575 does make sense... it's just making sure the user called theirs.
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode820
src/pkg/net/http/transport.go:820: isClosed uint32 // atomic bool, non zero if true
non-zero
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode854
src/pkg/net/http/transport.go:854: return atomic.LoadUint32(&es.isClosed) > 0
!= 0, to match comment above
Message from dvyukov@google.com
2012-10-11T04:26:44+00:00dvyukovurn:md5:b446ac65dbf2deb21aa8f35b39b5031a
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode837
src/pkg/net/http/transport.go:837: // already closed
Is it OK that a goroutine returns from Close() when the es is not yet closed?
Message from dave@cheney.net
2012-10-11T04:31:40+00:00dfcurn:md5:c2d5bb40001d884280da63077cc53b3b
Thank you for your comments.
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode820
src/pkg/net/http/transport.go:820: isClosed uint32 // atomic bool, non zero if true
On 2012/10/11 04:19:24, bradfitz wrote:
> non-zero
Done.
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode837
src/pkg/net/http/transport.go:837: // already closed
On 2012/10/11 04:26:45, dvyukov wrote:
> Is it OK that a goroutine returns from Close() when the es is not yet closed?
If setClosed was false, you lost the race to close, which means someone else won, and is closing es.body.Close.
https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode854
src/pkg/net/http/transport.go:854: return atomic.LoadUint32(&es.isClosed) > 0
On 2012/10/11 04:19:24, bradfitz wrote:
> != 0, to match comment above
Done.
Message from unknown
2012-10-11T04:31:49+00:00dfcurn:md5:105bf73863febf7cee4d5d28259baaa9
Message from dave@cheney.net
2012-10-11T04:31:55+00:00dfcurn:md5:5ca6962c0cceb55a3c96aa303ff1814f
Hello dvyukov@google.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from bradfitz@golang.org
2012-10-11T04:43:11+00:00bradfitzurn:md5:425b1bae21b939fd5bb499e78e24aea4
But I think Dmitry means that the loser still should wait for the winner's
function to complete.
On Oct 10, 2012 9:31 PM, <dave@cheney.net> wrote:
> Thank you for your comments.
>
>
> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
> net/http/transport.go<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go>
> File src/pkg/net/http/transport.go (right):
>
> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
> net/http/transport.go#**newcode820<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode820>
> src/pkg/net/http/transport.go:**820: isClosed uint32 // atomic bool, non
> zero if true
> On 2012/10/11 04:19:24, bradfitz wrote:
>
>> non-zero
>>
>
> Done.
>
> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
> net/http/transport.go#**newcode837<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode837>
> src/pkg/net/http/transport.go:**837: // already closed
> On 2012/10/11 04:26:45, dvyukov wrote:
>
>> Is it OK that a goroutine returns from Close() when the es is not yet
>>
> closed?
>
> If setClosed was false, you lost the race to close, which means someone
> else won, and is closing es.body.Close.
>
> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
> net/http/transport.go#**newcode854<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode854>
> src/pkg/net/http/transport.go:**854: return
> atomic.LoadUint32(&es.**isClosed) > 0
> On 2012/10/11 04:19:24, bradfitz wrote:
>
>> != 0, to match comment above
>>
>
> Done.
>
> https://codereview.appspot.**com/6640057/<https://codereview.appspot.com/6640057/>
>
Message from dvyukov@google.com
2012-10-11T05:45:08+00:00dvyukovurn:md5:de4197d776bd6663950a00bf9eed9fe4
Yes,that's what I mean. But I don't know whether its required.
On Oct 11, 2012 8:43 AM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
> But I think Dmitry means that the loser still should wait for the winner's
> function to complete.
> On Oct 10, 2012 9:31 PM, <dave@cheney.net> wrote:
>
>> Thank you for your comments.
>>
>>
>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>> net/http/transport.go<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go>
>> File src/pkg/net/http/transport.go (right):
>>
>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>> net/http/transport.go#**newcode820<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode820>
>> src/pkg/net/http/transport.go:**820: isClosed uint32 // atomic bool, non
>> zero if true
>> On 2012/10/11 04:19:24, bradfitz wrote:
>>
>>> non-zero
>>>
>>
>> Done.
>>
>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>> net/http/transport.go#**newcode837<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode837>
>> src/pkg/net/http/transport.go:**837: // already closed
>> On 2012/10/11 04:26:45, dvyukov wrote:
>>
>>> Is it OK that a goroutine returns from Close() when the es is not yet
>>>
>> closed?
>>
>> If setClosed was false, you lost the race to close, which means someone
>> else won, and is closing es.body.Close.
>>
>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>> net/http/transport.go#**newcode854<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode854>
>> src/pkg/net/http/transport.go:**854: return
>> atomic.LoadUint32(&es.**isClosed) > 0
>> On 2012/10/11 04:19:24, bradfitz wrote:
>>
>>> != 0, to match comment above
>>>
>>
>> Done.
>>
>> https://codereview.appspot.**com/6640057/<https://codereview.appspot.com/6640057/>
>>
>
Message from bradfitz@golang.org
2012-10-11T16:40:56+00:00bradfitzurn:md5:b975bf1920a7cfbfecc52eb72ce72525
I just looked. Doesn't seem to matter.
If the user goroutine calls Close() first and then the http package calls
it, the transport will still want for the condfn to finish:
var waitForBodyRead chan bool
if hasBody {
lastbody = resp.Body
waitForBodyRead = make(chan bool, 1)
resp.Body.(*bodyEOFSignal).fn = func() {
if alive && !pc.t.putIdleConn(pc) {
alive = false
}
if !alive || pc.isBroken() {
pc.close()
}
waitForBodyRead <- true
}
....
// Wait for the just-returned response body to be fully consumed
// before we race and peek on the underlying bufio reader.
if waitForBodyRead != nil {
<-waitForBodyRead
}
So I think we're fine with this CL as-is.
Thanks, Dave!
On Wed, Oct 10, 2012 at 10:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Yes,that's what I mean. But I don't know whether its required.
> On Oct 11, 2012 8:43 AM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
>
>> But I think Dmitry means that the loser still should wait for the
>> winner's function to complete.
>> On Oct 10, 2012 9:31 PM, <dave@cheney.net> wrote:
>>
>>> Thank you for your comments.
>>>
>>>
>>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>>> net/http/transport.go<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go>
>>> File src/pkg/net/http/transport.go (right):
>>>
>>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>>> net/http/transport.go#**newcode820<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode820>
>>> src/pkg/net/http/transport.go:**820: isClosed uint32 // atomic bool, non
>>> zero if true
>>> On 2012/10/11 04:19:24, bradfitz wrote:
>>>
>>>> non-zero
>>>>
>>>
>>> Done.
>>>
>>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>>> net/http/transport.go#**newcode837<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode837>
>>> src/pkg/net/http/transport.go:**837: // already closed
>>> On 2012/10/11 04:26:45, dvyukov wrote:
>>>
>>>> Is it OK that a goroutine returns from Close() when the es is not yet
>>>>
>>> closed?
>>>
>>> If setClosed was false, you lost the race to close, which means someone
>>> else won, and is closing es.body.Close.
>>>
>>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>>> net/http/transport.go#**newcode854<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode854>
>>> src/pkg/net/http/transport.go:**854: return
>>> atomic.LoadUint32(&es.**isClosed) > 0
>>> On 2012/10/11 04:19:24, bradfitz wrote:
>>>
>>>> != 0, to match comment above
>>>>
>>>
>>> Done.
>>>
>>> https://codereview.appspot.**com/6640057/<https://codereview.appspot.com/6640057/>
>>>
>>
Message from dave@cheney.net
2012-10-11T20:44:48+00:00dfcurn:md5:6c991b0196f775404d770521633ea263
Awesome. Thanks for the confirmation.
On 12 Oct 2012 03:40, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
> I just looked. Doesn't seem to matter.
>
> If the user goroutine calls Close() first and then the http package calls
> it, the transport will still want for the condfn to finish:
>
> var waitForBodyRead chan bool
> if hasBody {
> lastbody = resp.Body
> waitForBodyRead = make(chan bool, 1)
> resp.Body.(*bodyEOFSignal).fn = func() {
> if alive && !pc.t.putIdleConn(pc) {
> alive = false
> }
> if !alive || pc.isBroken() {
> pc.close()
> }
> waitForBodyRead <- true
> }
> ....
> // Wait for the just-returned response body to be fully consumed
>
> // before we race and peek on the underlying bufio reader.
>
> if waitForBodyRead != nil {
> <-waitForBodyRead
> }
>
>
> So I think we're fine with this CL as-is.
>
> Thanks, Dave!
>
>
> On Wed, Oct 10, 2012 at 10:45 PM, Dmitry Vyukov <dvyukov@google.com>wrote:
>
>> Yes,that's what I mean. But I don't know whether its required.
>> On Oct 11, 2012 8:43 AM, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
>>
>>> But I think Dmitry means that the loser still should wait for the
>>> winner's function to complete.
>>> On Oct 10, 2012 9:31 PM, <dave@cheney.net> wrote:
>>>
>>>> Thank you for your comments.
>>>>
>>>>
>>>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>>>> net/http/transport.go<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go>
>>>> File src/pkg/net/http/transport.go (right):
>>>>
>>>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>>>> net/http/transport.go#**newcode820<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode820>
>>>> src/pkg/net/http/transport.go:**820: isClosed uint32 // atomic bool,
>>>> non
>>>> zero if true
>>>> On 2012/10/11 04:19:24, bradfitz wrote:
>>>>
>>>>> non-zero
>>>>>
>>>>
>>>> Done.
>>>>
>>>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>>>> net/http/transport.go#**newcode837<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode837>
>>>> src/pkg/net/http/transport.go:**837: // already closed
>>>> On 2012/10/11 04:26:45, dvyukov wrote:
>>>>
>>>>> Is it OK that a goroutine returns from Close() when the es is not yet
>>>>>
>>>> closed?
>>>>
>>>> If setClosed was false, you lost the race to close, which means someone
>>>> else won, and is closing es.body.Close.
>>>>
>>>> https://codereview.appspot.**com/6640057/diff/4001/src/pkg/**
>>>> net/http/transport.go#**newcode854<https://codereview.appspot.com/6640057/diff/4001/src/pkg/net/http/transport.go#newcode854>
>>>> src/pkg/net/http/transport.go:**854: return
>>>> atomic.LoadUint32(&es.**isClosed) > 0
>>>> On 2012/10/11 04:19:24, bradfitz wrote:
>>>>
>>>>> != 0, to match comment above
>>>>>
>>>>
>>>> Done.
>>>>
>>>> https://codereview.appspot.**com/6640057/<https://codereview.appspot.com/6640057/>
>>>>
>>>
>
Message from unknown
2012-10-11T21:32:16+00:00dfcurn:md5:2c91af95740da372f2970f8e7a9bdb18
Message from unknown
2012-10-11T21:32:46+00:00dfcurn:md5:31232ae78aa3b03a5bc5aa95741c441d
Message from dave@cheney.net
2012-10-11T21:33:14+00:00dfcurn:md5:c15142949c2549f7f448b0013c47734b
*** Submitted as http://code.google.com/p/go/source/detail?r=cb38e940c38a ***
net/http: fix race on bodyEOFSignal.isClosed
Update issue 4191.
Fixes unreported race failure at
http://build.golang.org/log/61e43a328fb220801d3d5c88cd91916cfc5dc43c
R=dvyukov, bradfitz
CC=golang-dev
http://codereview.appspot.com/6640057