Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(21)

Issue 6940078: code review 6940078: go/token: add test for concurrent use of FileSet.Pos (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 6 months ago by dfc
Modified:
9 years, 6 months ago
Reviewers:
CC:
dvyukov, gri, albert.strasheim, golang-dev
Visibility:
Public.

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.

Patch Set 1 #

Patch Set 2 : diff -r 3684de5292bf https://code.google.com/p/go #

Patch Set 3 : diff -r 3684de5292bf https://code.google.com/p/go #

Total comments: 4

Patch Set 4 : diff -r 3684de5292bf https://code.google.com/p/go #

Patch Set 5 : diff -r 3684de5292bf https://code.google.com/p/go #

Patch Set 6 : diff -r 3684de5292bf https://code.google.com/p/go #

Total comments: 4

Patch Set 7 : diff -r 3684de5292bf https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -0 lines) Patch
M src/pkg/go/token/position_test.go View 1 2 3 4 5 6 2 chunks +25 lines, -0 lines 0 comments Download

Messages

Total messages: 14
dfc
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
9 years, 6 months ago (2012-12-18 10:27:56 UTC) #1
dvyukov
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 ...
9 years, 6 months ago (2012-12-18 10:43:24 UTC) #2
dvyukov
leaving the rest for gri
9 years, 6 months ago (2012-12-18 10:43:46 UTC) #3
dfc
> Is it required? > Race detector does not need GOMAXPROCS to find races. It ...
9 years, 6 months ago (2012-12-18 11:12:02 UTC) #4
dvyukov
Humm... I think it's possible. First, the goroutines may run till completion w/o blocking calls, ...
9 years, 6 months ago (2012-12-18 11:17:47 UTC) #5
gri
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 ...
9 years, 6 months ago (2012-12-18 18:10:29 UTC) #6
dfc
Dmitry, Robert, please take another look. I have removed the blocking on the shared math.Rand ...
9 years, 6 months ago (2012-12-18 22:03:55 UTC) #7
dfc
Hello dvyukov@google.com, gri@golang.org (cc: fullung@gmail.com, golang-dev@googlegroups.com), Please take another look.
9 years, 6 months ago (2012-12-18 22:05:06 UTC) #8
dfc
Hello dvyukov@google.com, gri@golang.org (cc: fullung@gmail.com, golang-dev@googlegroups.com), Please take another look.
9 years, 6 months ago (2012-12-18 22:11:27 UTC) #9
gri
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 ...
9 years, 6 months ago (2012-12-18 22:55:00 UTC) #10
dfc
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, ...
9 years, 6 months ago (2012-12-19 00:10:28 UTC) #11
gri
LGTM. Thanks.
9 years, 6 months ago (2012-12-19 00:22:12 UTC) #12
gri
*** Submitted as https://code.google.com/p/go/source/detail?r=e7cd0a82d669 *** go/token: add test for concurrent use of FileSet.Pos Update issue ...
9 years, 6 months ago (2012-12-19 00:38:07 UTC) #13
dvyukov
9 years, 6 months ago (2012-12-19 06:24:07 UTC) #14
On Wed, Dec 19, 2012 at 2:03 AM, <dave@cheney.net> 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
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b