code review 6640057: net/http: fix race on bodyEOFSignal.isClosed

net/http: fix race on bodyEOFSignal.isClosed

Update issue 4191.

Fixes unreported race failure at
http://build.golang.org/log/61e43a328fb220801d3d5c88cd91916cfc5dc43c

[dave_cheney.net, 2012-10-11 04:04:39]

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

[bradfitz, 2012-10-11 04:09:41]

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...
>
> 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
>
>
>

[bradfitz, 2012-10-11 04:19:23]

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.g...
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.g...
src/pkg/net/http/transport.go:854: return atomic.LoadUint32(&es.isClosed) > 0
!= 0, to match comment above

[dvyukov, 2012-10-11 04:26:44]

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.g...
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?

[dave_cheney.net, 2012-10-11 04:31:40]

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.g...
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.g...
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.g...
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.

[dave_cheney.net, 2012-10-11 04:31:55]

Hello dvyukov@google.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-10-11 04:43:11]

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/6640...
>

[dvyukov, 2012-10-11 05:45:08]

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/6640...
>>
>

[bradfitz, 2012-10-11 16:40:56]

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/6640...
>>>
>>

[dave_cheney.net, 2012-10-11 20:44:48]

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/6640...
>>>>
>>>
>

[dave_cheney.net, 2012-10-11 21:33:14]

*** 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