code review 7103046: time: fix race

time: fix race

Fixes issue 4622

[bradfitz, 2013-01-11 22:21:21]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2013-01-11 22:23:10]

whoops, fixing the transpose.

Tested by hand.  Will try to write a reliable test.

On Fri, Jan 11, 2013 at 2:21 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> time: fix race
>
> Fixes issue 4622
>
> Please review this at
https://codereview.appspot.**com/7103046/<https://codereview.appspot.com/7103...
>
> Affected files:
>   M src/pkg/time/time.go
>
>
> Index: src/pkg/time/time.go
> ==============================**==============================**=======
> --- a/src/pkg/time/time.go
> +++ b/src/pkg/time/time.go
> @@ -257,12 +257,12 @@
>         return uint64(sec + (unixToInternal + internalToAbsolute))
>  }
>
> -// locabs is a combination of the Zone and abs methods,
> +// locbas is a combination of the Zone and abs methods,
>  // extracting both return values from a single zone lookup.
>  func (t Time) locabs() (name string, offset int, abs uint64) {
>         l := t.loc
> -       if l == nil {
> -               l = &utcLoc
> +       if l == nil || l == &localLoc {
> +               l = l.get()
>         }
>         // Avoid function call if we hit the local time cache.
>         sec := t.sec + internalToUnix
>
>
>

[dave_cheney.net, 2013-01-11 22:30:40]

Hi Brad, this didn't pass the test included with the error report on my machine.
I don't think it is possible to write a test case inside the time package
itself, as internal_test.go contains an init routine which will setup all the
location stuff before any test code can run. Maybe a sub package ?

https://codereview.appspot.com/7103046/diff/2002/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/7103046/diff/2002/src/pkg/time/time.go#newcode260
src/pkg/time/time.go:260: // locbas is a combination of the Zone and abs
methods,
typo ?

[bradfitz, 2013-01-11 22:34:59]

On Fri, Jan 11, 2013 at 2:30 PM, <dave@cheney.net> wrote:

> Hi Brad, this didn't pass the test included with the error report on my
> machine.


Fixes it reliably here, and reading the code, it seems correct.  The old
code was trying to access the local cache (l.cacheZone != nil &&
l.cacheStart <= sec && sec < l.cacheEnd) before it was initialized by
l.get(),

func (l *Location) get() *Location {
        if l == nil {
                return &utcLoc
        }
if l == &localLoc {
                localOnce.Do(initLocal)
        }
        return l
}

All the other code use the same l.get() pattern too.


I don't think it is possible to write a test case inside the
> time package itself, as internal_test.go contains an init routine which
> will setup all the location stuff before any test code can run. Maybe a
> sub package ?
>

I tried clearing the Onces too in my test.


>
>
>
https://codereview.appspot.**com/7103046/diff/2002/src/pkg/**time/time.go<htt...
> File src/pkg/time/time.go (right):
>
> https://codereview.appspot.**com/7103046/diff/2002/src/pkg/**
>
time/time.go#newcode260<https://codereview.appspot.com/7103046/diff/2002/src/pkg/time/time.go#newcode260>
> src/pkg/time/time.go:260: // locbas is a combination of the Zone and abs
> methods,
> typo ?
>

yes, noted above.

[bradfitz, 2013-01-11 22:40:43]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-01-11 22:42:26]

PTAL

This now has a test which fails under test -race without the fix and passes
with it.


On Fri, Jan 11, 2013 at 2:40 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, dave@cheney.net (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
https://codereview.appspot.**com/7103046/<https://codereview.appspot.com/7103...
>

[dave_cheney.net, 2013-01-11 22:43:49]

Thanks Brad, but now another test fails to pass under the race detector

lucky(~/go/src/pkg/time) % go test -race
warning: building out-of-date packages:
        bytes
        bufio
        os
        fmt
        encoding/gob
        encoding/base64
        unicode/utf16
        encoding/json
        encoding/binary
        math/rand
        math/big
        flag
        text/tabwriter
        runtime/pprof
        testing
        testing/quick
        regexp/syntax
        regexp
installing these packages with 'go test -race -i' will speed future tests.

--- FAIL: ExampleDate (0.00 seconds)
got:
Go launched at 2009-11-11 10:00:00 +1100 EST
want:
Go launched at 2009-11-10 15:00:00 -0800 PST
FAIL
exit status 1
FAIL    time    13.854s

https://codereview.appspot.com/7103046/diff/1003/src/pkg/time/time_test.go
File src/pkg/time/time_test.go (right):

https://codereview.appspot.com/7103046/diff/1003/src/pkg/time/time_test.go#ne...
src/pkg/time/time_test.go:1230: // golang.org/issue/4622
nice touch.

[bradfitz, 2013-01-11 22:53:56]

On Fri, Jan 11, 2013 at 2:43 PM, <dave@cheney.net> wrote:

> Thanks Brad, but now another test fails to pass under the race detector


I can't reproduce that one.

Can we leave that one to somebody who can?

[bradfitz, 2013-01-11 23:00:03]

On Fri, Jan 11, 2013 at 2:53 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

>
>
> On Fri, Jan 11, 2013 at 2:43 PM, <dave@cheney.net> wrote:
>
>> Thanks Brad, but now another test fails to pass under the race detector
>
>
> I can't reproduce that one.
>

I can't reproduce this with or without my patch, on Linux or OS X.

How are you triggering it?

[dave_cheney.net, 2013-01-11 23:05:29]

Maybe my machine is screwed. The -race builder is the ultimate arbiter,
thanks for adding the test.
On 12 Jan 2013 10:00, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:

>
>
> On Fri, Jan 11, 2013 at 2:53 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>>
>>
>> On Fri, Jan 11, 2013 at 2:43 PM, <dave@cheney.net> wrote:
>>
>>> Thanks Brad, but now another test fails to pass under the race detector
>>
>>
>> I can't reproduce that one.
>>
>
> I can't reproduce this with or without my patch, on Linux or OS X.
>
> How are you triggering it?
>
>

[bradfitz, 2013-01-12 17:18:53]

Is your machine still having an error on the tests?  Does it without my
patch?  I don't see how it could be related.  Did you "go install -race
std" first?

Thoughts on the CL otherwise?


On Fri, Jan 11, 2013 at 3:05 PM, Dave Cheney <dave@cheney.net> wrote:

> Maybe my machine is screwed. The -race builder is the ultimate arbiter,
> thanks for adding the test.
> On 12 Jan 2013 10:00, "Brad Fitzpatrick" <bradfitz@golang.org> wrote:
>
>>
>>
>> On Fri, Jan 11, 2013 at 2:53 PM, Brad Fitzpatrick
<bradfitz@golang.org>wrote:
>>
>>>
>>>
>>> On Fri, Jan 11, 2013 at 2:43 PM, <dave@cheney.net> wrote:
>>>
>>>> Thanks Brad, but now another test fails to pass under the race detector
>>>
>>>
>>> I can't reproduce that one.
>>>
>>
>> I can't reproduce this with or without my patch, on Linux or OS X.
>>
>> How are you triggering it?
>>
>>

[dave_cheney.net, 2013-01-13 04:51:51]

I think the problem is your machine may be native to PST, so resetting
the timezone for the test does not cause the test to fail.

On Sat, Jan 12, 2013 at 9:43 AM,  <dave@cheney.net> wrote:
> Thanks Brad, but now another test fails to pass under the race detector
>
> lucky(~/go/src/pkg/time) % go test -race
> warning: building out-of-date packages:
>         bytes
>         bufio
>         os
>         fmt
>         encoding/gob
>         encoding/base64
>         unicode/utf16
>         encoding/json
>         encoding/binary
>         math/rand
>         math/big
>         flag
>         text/tabwriter
>         runtime/pprof
>         testing
>         testing/quick
>         regexp/syntax
>         regexp
> installing these packages with 'go test -race -i' will speed future
> tests.
>
> --- FAIL: ExampleDate (0.00 seconds)
> got:
> Go launched at 2009-11-11 10:00:00 +1100 EST
> want:
> Go launched at 2009-11-10 15:00:00 -0800 PST
> FAIL
> exit status 1
> FAIL    time    13.854s
>
>
> https://codereview.appspot.com/7103046/diff/1003/src/pkg/time/time_test.go
> File src/pkg/time/time_test.go (right):
>
>
https://codereview.appspot.com/7103046/diff/1003/src/pkg/time/time_test.go#ne...
> src/pkg/time/time_test.go:1230: // golang.org/issue/4622
> nice touch.
>
> https://codereview.appspot.com/7103046/

[dave_cheney.net, 2013-01-14 00:18:03]

Hi Brad,

Sorry this is still failing for me so I don't think I can test it reliably.
Maybe we need to ask Dmitry for some of his time.

Cheers

Dave

[bradfitz, 2013-01-14 00:18:34]

On Sat, Jan 12, 2013 at 8:51 PM, Dave Cheney <dave@cheney.net> wrote:

> I think the problem is your machine may be native to PST, so resetting
> the timezone for the test does not cause the test to fail.


Oh, indeed.  Thanks!  The tests were already forcing US/Pacific, but my
test broke that for subsequent tests.  I just got lucky.  The race builder
(in Moscow) would've failed.

Fixed.  PTAL.

[bradfitz, 2013-01-14 00:18:49]

Hello golang-dev@googlegroups.com, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2013-01-14 10:12:10]

LGTM, but please wait for Dmitry

Thank you for fixing the race. I have a concern that there are other races in
this package. 

l.get() does not do any sort of memory barrier that would let other threads see
the value of localLoc that was initialised inside initLocal. It may be that the
repeated tests of l == localLoc are all failing, then falling through to the
do.Once which is acting as a memory barrier.

https://codereview.appspot.com/7103046/diff/3003/src/pkg/time/time.go
File src/pkg/time/time.go (right):

https://codereview.appspot.com/7103046/diff/3003/src/pkg/time/time.go#newcode264
src/pkg/time/time.go:264: if l == nil || l == &localLoc {
l.get() already does both of these tests, maybe line 265 should just be l =
l.get() without the test for l == &localLoc

[bradfitz, 2013-01-14 17:09:37]

On Mon, Jan 14, 2013 at 2:12 AM, <dave@cheney.net> wrote:

> LGTM, but please wait for Dmitry
>
> Thank you for fixing the race. I have a concern that there are other
> races in this package.
>
> l.get() does not do any sort of memory barrier that would let other
> threads see the value of localLoc that was initialised inside initLocal.
> It may be that the repeated tests of l == localLoc are all failing, then
> falling through to the do.Once which is acting as a memory barrier.
>

Um, &localLoc can't change.  localLoc is:

var localLoc Location

so checks that l == &localLoc are fine.


>
https://codereview.appspot.**com/7103046/diff/3003/src/pkg/**time/time.go<htt...
> File src/pkg/time/time.go (right):
>
> https://codereview.appspot.**com/7103046/diff/3003/src/pkg/**
>
time/time.go#newcode264<https://codereview.appspot.com/7103046/diff/3003/src/pkg/time/time.go#newcode264>
> src/pkg/time/time.go:264: if l == nil || l == &localLoc {
> l.get() already does both of these tests, maybe line 265 should just be
> l = l.get() without the test for l == &localLoc
>

The existing comment in abs says:

        // Avoid function calls when possible.



So I'm not calling get if it's unnecessary, copying the pattern from
elsewhere.

[dvyukov, 2013-01-14 17:22:01]

If the issue was that the code path skip "localOnce.Do(initLocal)", then LGTM.

[bradfitz, 2013-01-14 17:55:51]

It called it, but too late. It first accessed some cache fields from
localLoc and then found it was invalid and then called the Once. But if two
goroutines did that, one would see a racy cache while the other Once was
running.

This moves the init to the top.
On Jan 14, 2013 9:22 AM, <dvyukov@google.com> wrote:

> If the issue was that the code path skip "localOnce.Do(initLocal)", then
> LGTM.
>
>
>
https://codereview.appspot.**com/7103046/<https://codereview.appspot.com/7103...
>

[bradfitz, 2013-01-14 22:09:47]

*** Submitted as https://code.google.com/p/go/source/detail?r=39a404853d24 ***

time: fix race

Fixes issue 4622

R=golang-dev, dave, dvyukov
CC=golang-dev
https://codereview.appspot.com/7103046