FAQ
Reviewers: dvyukov, gri,

Message:
Hello dvyukov@google.com, gri@golang.org (cc: fullung@gmail.com,
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go


Description:
go/token: add test for concurrent use of FileSet.Pos

Update issue 4354.

Add a test to expose the race in the FileSet position cache.

Please review this at https://codereview.appspot.com/6940078/

Affected files:
M src/pkg/go/token/position_test.go


Index: src/pkg/go/token/position_test.go
===================================================================
--- a/src/pkg/go/token/position_test.go
+++ b/src/pkg/go/token/position_test.go
@@ -6,7 +6,10 @@

import (
"fmt"
+ "math/rand"
+ "runtime"
"testing"
+ "time"
)

func checkPos(t *testing.T, msg string, p, q Position) {
@@ -179,3 +182,30 @@
}
}
}
+
+// issue 4345. Test concurrent use of FileSet.Pos does not trigger a
+// race in the FileSet position cache.
+func TestFileSetRace(t *testing.T) {
+ defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(runtime.NumCPU()))
+ fset := NewFileSet()
+ for i := 0; i < 100; i++ {
+ fset.AddFile(fmt.Sprintf("file-%d", i), fset.Base(), 1031)
+ }
+ max := int32(fset.Base())
+ var done = make(chan struct{})
+ for i := 0; i < runtime.GOMAXPROCS(0)*2; i++ {
+ go func() {
+ for {
+ select {
+ case <-done:
+ return
+ default:
+ p := Pos(rand.Int31n(max))
+ fset.Position(p)
+ }
+ }
+ }()
+ }
+ <-time.After(200 * time.Millisecond)
+ close(done)
+}

Search Discussions

  • Dvyukov at Dec 18, 2012 at 10:43 am
    https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_test.go
    File src/pkg/go/token/position_test.go (right):

    https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_test.go#newcode189
    src/pkg/go/token/position_test.go:189: defer
    runtime.GOMAXPROCS(runtime.GOMAXPROCS(runtime.NumCPU()))
    Is it required?
    Race detector does not need GOMAXPROCS to find races. It can also be
    executed on a machine with 1 core.

    https://codereview.appspot.com/6940078/
  • Dave Cheney at Dec 18, 2012 at 11:12 am

    Is it required?
    Race detector does not need GOMAXPROCS to find races. It can also be
    executed on a machine with 1 core.
    The race detector does not report this version races unless GOMAXPROCS > 1

    func TestFileSetRace(t *testing.T) {
    fset := NewFileSet()
    for i := 0; i < 100; i++ {
    fset.AddFile(fmt.Sprintf("file-%d", i), fset.Base(), 1031)
    }
    max := int32(fset.Base())
    var wg sync.WaitGroup
    for i := 0; i < 16; i++ {
    wg.Add(1)
    go func() {
    defer wg.Done()
    for i := 0 ; i < 10000 ; i++ {
    p := Pos(rand.Int31n(max))
    fset.Position(p)
    }
    }()
    }
    wg.Wait()
    }
  • Dmitry Vyukov at Dec 18, 2012 at 11:17 am
    Humm... I think it's possible. First, the goroutines may run till
    completion w/o blocking calls, and the second is that I suspect
    rand.Int31n() contains internal mutex and so synchronizes goroutines.

    Perhaps set GOMAXPROCS to 4 then.



    On Tue, Dec 18, 2012 at 3:12 PM, Dave Cheney wrote:

    Is it required?
    Race detector does not need GOMAXPROCS to find races. It can also be
    executed on a machine with 1 core.
    The race detector does not report this version races unless GOMAXPROCS > 1

    func TestFileSetRace(t *testing.T) {
    fset := NewFileSet()
    for i := 0; i < 100; i++ {
    fset.AddFile(fmt.Sprintf("file-%d", i), fset.Base(), 1031)
    }
    max := int32(fset.Base())
    var wg sync.WaitGroup
    for i := 0; i < 16; i++ {
    wg.Add(1)
    go func() {
    defer wg.Done()
    for i := 0 ; i < 10000 ; i++ {
    p := Pos(rand.Int31n(max))
    fset.Position(p)
    }
    }()
    }
    wg.Wait()
    }
  • Dvyukov at Dec 18, 2012 at 10:43 am
  • Gri at Dec 18, 2012 at 6:10 pm
    some minor comments

    otherwise looks great; just what I was waiting for


    https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_test.go
    File src/pkg/go/token/position_test.go (right):

    https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_test.go#newcode196
    src/pkg/go/token/position_test.go:196: for i := 0; i <
    runtime.GOMAXPROCS(0)*2; i++ {
    maybe count down? (no need to call GOMAXPROCS repeatedly).

    for i := runtime.GOMAXPROCS(0)*2; i > 0; i--

    https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_test.go#newcode204
    src/pkg/go/token/position_test.go:204: fset.Position(p)
    fset.Position(Pos(rand.Int31n(max)))

    https://codereview.appspot.com/6940078/diff/1003/src/pkg/go/token/position_test.go#newcode209
    src/pkg/go/token/position_test.go:209: <-time.After(200 *
    time.Millisecond)
    time.Sleep(200 * time.Millisecond)

    https://codereview.appspot.com/6940078/
  • Dave at Dec 18, 2012 at 10:03 pm
    Dmitry, Robert, please take another look. I have removed the blocking on
    the shared math.Rand and the race detector no longer needs GOMAXPROCS >
    1 to fire.

    Dmitry: there is one issue

    ==================
    WARNING: DATA RACE
    Read by goroutine 8:
    go/token.(*FileSet).file()
    /home/dfc/go/src/pkg/go/token/position.go:371 +0x38
    go/token.(*FileSet).Position()
    /home/dfc/go/src/pkg/go/token/position.go:403 +0x70
    go/token.func·004()
    /home/dfc/go/src/pkg/go/token/position_test.go:201 +0xbd

    Previous write by goroutine 7:
    [failed to restore the stack]

    Goroutine 8 (running) created at:
    go/token.TestFileSetRace()
    /home/dfc/go/src/pkg/go/token/position_test.go:203 +0x46e
    testing.tRunner()
    /home/dfc/go/src/pkg/testing/testing.go:302 +0xe8

    Goroutine 7 (finished) created at:
    go/token.TestFileSetRace()
    /home/dfc/go/src/pkg/go/token/position_test.go:203 +0x46e
    testing.tRunner()
    /home/dfc/go/src/pkg/testing/testing.go:302 +0xe8

    ==================

    Note the missing stack trace in the 2nd goroutine.

    https://codereview.appspot.com/6940078/
  • Dmitry Vyukov at Dec 19, 2012 at 6:24 am

    On Wed, Dec 19, 2012 at 2:03 AM, wrote:

    Dmitry, Robert, please take another look. I have removed the blocking on
    the shared math.Rand and the race detector no longer needs GOMAXPROCS >
    1 to fire.

    Dmitry: there is one issue

    ==================
    WARNING: DATA RACE
    Read by goroutine 8:
    go/token.(*FileSet).file()
    /home/dfc/go/src/pkg/go/token/**position.go:371 +0x38
    go/token.(*FileSet).Position()
    /home/dfc/go/src/pkg/go/token/**position.go:403 +0x70
    go/token.func·004()
    /home/dfc/go/src/pkg/go/token/**position_test.go:201 +0xbd

    Previous write by goroutine 7:
    [failed to restore the stack]

    Goroutine 8 (running) created at:
    go/token.TestFileSetRace()
    /home/dfc/go/src/pkg/go/token/**position_test.go:203 +0x46e
    testing.tRunner()
    /home/dfc/go/src/pkg/testing/**testing.go:302 +0xe8

    Goroutine 7 (finished) created at:
    go/token.TestFileSetRace()
    /home/dfc/go/src/pkg/go/token/**position_test.go:203 +0x46e
    testing.tRunner()
    /home/dfc/go/src/pkg/testing/**testing.go:302 +0xe8

    ==================

    Note the missing stack trace in the 2nd goroutine.

    Let's see whether documentation is usable :)

    http://code.google.com/p/go-wiki/wiki/RaceDetector
  • Dave at Dec 18, 2012 at 10:05 pm
    Hello dvyukov@google.com, gri@golang.org (cc: fullung@gmail.com,
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6940078/
  • Dave at Dec 18, 2012 at 10:11 pm
    Hello dvyukov@google.com, gri@golang.org (cc: fullung@gmail.com,
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6940078/
  • Gri at Dec 18, 2012 at 10:55 pm
    FYI


    https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_test.go
    File src/pkg/go/token/position_test.go (right):

    https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_test.go#newcode199
    src/pkg/go/token/position_test.go:199: defer stop.Done()
    just call stop.Done() at the end - defer seems overkill

    https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_test.go#newcode205
    src/pkg/go/token/position_test.go:205: stop.Wait()
    if the race shows up very quickly, just use time.Sleep() here (with a
    short time) and then this test's runt-time is independent of machine.
    And then you don't need the WaitGroup and the inner loop can just be
    endless. Even less code overall.

    https://codereview.appspot.com/6940078/
  • Dave at Dec 19, 2012 at 12:10 am
    Thanks for your comments.


    https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_test.go
    File src/pkg/go/token/position_test.go (right):

    https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_test.go#newcode199
    src/pkg/go/token/position_test.go:199: defer stop.Done()
    On 2012/12/18 22:55:00, gri wrote:
    just call stop.Done() at the end - defer seems overkill
    Done.

    https://codereview.appspot.com/6940078/diff/14001/src/pkg/go/token/position_test.go#newcode205
    src/pkg/go/token/position_test.go:205: stop.Wait()
    On 2012/12/18 22:55:00, gri wrote:
    if the race shows up very quickly, just use time.Sleep() here (with a short
    time) and then this test's runt-time is independent of machine. And then you
    don't need the WaitGroup and the inner loop can just be endless. Even less code
    overall.
    I'm concerned about leaving these goroutines running after the Test is
    finished. I've reduced the loop count to 1000 which is sufficient for
    triggering the race. The run time on the slowest arm host that I have
    for go test go/token is 300ms, much less than the time it takes to
    compile and link the test binary.

    https://codereview.appspot.com/6940078/
  • Gri at Dec 19, 2012 at 12:22 am
  • Gri at Dec 19, 2012 at 12:38 am
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=e7cd0a82d669 ***

    go/token: add test for concurrent use of FileSet.Pos

    Update issue 4354.

    Add a test to expose the race in the FileSet position cache.

    R=dvyukov, gri
    CC=fullung, golang-dev
    https://codereview.appspot.com/6940078

    Committer: Robert Griesemer <gri@golang.org>


    https://codereview.appspot.com/6940078/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 18, '12 at 10:27a
activeDec 19, '12 at 6:24a
posts14
users3
websitegolang.org

3 users in discussion

Dave: 6 posts Dmitry Vyukov: 4 posts Gri: 4 posts

People

Translate

site design / logo © 2022 Grokbase