code review 103300045: fsnotify: fix data race on kevent buffer.

fsnotify: fix data race on kevent buffer.

In the BSD implementation of fsnotify, the watcher's kbuf buffers a
kevent between syscall.SetKevent (which prepares the kevent) and
syscall.Kevent (which registers the kevent). The implementation intends
to protect access to kbuf, but fails to do so in addWatch and removeWatch.

This change fixes the data race by allocating a new kevent buffer for
every method invocation.

Run the following command (as suggested by nathany) to test the change on OS X:
go test -test.run=TestConcurrentRemovalOfWatch -test.cpu=1,1,1,1,1 -race

[tilaks, 2014-06-11 20:22:59]

Hello golang-dev@googlegroups.com (cc: golang-codereviews@googlegroups.com),

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

[gobot, 2014-06-11 20:24:43]

Replacing golang-dev with golang-codereviews.

To the author of this CL: 

If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail'
instead.

If you did not name golang-dev explicitly and it was still added to the CL,
it means your working copy of the repo has a stale codereview.cfg
(or lib/codereview/codereview.cfg).
Please run 'hg sync' to update your client to the most recent codereview.cfg.
If the most recent codereview.cfg has defaultcc set to golang-dev instead of
golang-codereviews, please send a CL correcting it.

Thanks very much.

[nathany, 2014-06-13 03:02:50]

On 2014/06/11 20:24:43, gobot wrote:
> Replacing golang-dev with golang-codereviews.
> 
> To the author of this CL: 
> 
> If you are using 'hg mail -r golang-dev' to mail the CL, use simply 'hg mail'
> instead.
> 
> If you did not name golang-dev explicitly and it was still added to the CL,
> it means your working copy of the repo has a stale codereview.cfg
> (or lib/codereview/codereview.cfg).
> Please run 'hg sync' to update your client to the most recent codereview.cfg.
> If the most recent codereview.cfg has defaultcc set to golang-dev instead of
> golang-codereviews, please send a CL correcting it.
> 
> Thanks very much.

I tested this out under OS X 10.9.3 and FreeBSD 10.0

LGTM

[tilaks, 2014-06-13 06:47:04]

Hello golang-codereviews@googlegroups.com, nj@nathany.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[nathany, 2014-06-20 02:25:41]

On 2014/06/13 06:47:04, tilaks wrote:
> Hello mailto:golang-codereviews@googlegroups.com, mailto:nj@nathany.com (cc:
> mailto:golang-codereviews@googlegroups.com),
> 
> Please take another look.

Hi. If I run your test without the changes you made, I would expect test
failures. However that doesn't appear to be the case (at least for me). I tried
running go test -race on OS X. FreeBSD doesn't support the the race detector, so
I only used go test.

[tilaks, 2014-06-20 23:18:15]

On 2014/06/20 02:25:41, nathany wrote:
> On 2014/06/13 06:47:04, tilaks wrote:
> > Hello mailto:golang-codereviews@googlegroups.com, mailto:nj@nathany.com (cc:
> > mailto:golang-codereviews@googlegroups.com),
> > 
> > Please take another look.
> 
> Hi. If I run your test without the changes you made, I would expect test
> failures. However that doesn't appear to be the case (at least for me). I
tried
> running go test -race on OS X. FreeBSD doesn't support the the race detector,
so
> I only used go test.

Nathan, thanks for running the test. You're right - it doesn't seem to hit the
race condition every time, although you should see the failure if you run it a
few times.
One way to fix this is to simply run the test for a few tens of iterations - I
don't have any better ideas.

[nathany, 2014-06-20 23:25:00]

I don't know a better way either. Are you testing on BSD or OS X or both? I
can try running it until it fails...

On Friday, June 20, 2014, <tilak89@gmail.com> wrote:
>
> Nathan, thanks for running the test. You're right - it doesn't seem to
> hit the race condition every time, although you should see the failure
> if you run it a few times.
> One way to fix this is to simply run the test for a few tens of
> iterations - I don't have any better ideas.
>
> https://codereview.appspot.com/103300045/
>


-- 
Nathan Youngman
Email: nj@nathany.com
Web: http://www.nathany.com

[nathany, 2014-06-22 05:21:48]

Running the test several times on OS X 10.9.3 does result in a data race for me
prior to the fix in this CL. I don't see any data races with the fix applied.

go test -test.run=TestConcurrentRemovalOfWatch -test.cpu=1,1,1,1,1 -race

As FreeBSD doesn't have the race detector, I can't confirm whether it was an
issue there.

* The test does pass on Windows and Linux, but maybe this test should be in a
darwin/bsd specific file?
* I think the description of this CL should include the test run line above that
made it fail.
* A comment to "See https://codereview.appspot.com/103300045/" may be
appropriate in this case.

Thanks!


> On Friday, June 20, 2014, <mailto:tilak89@gmail.com> wrote:
> >
> > Nathan, thanks for running the test. You're right - it doesn't seem to
> > hit the race condition every time, although you should see the failure
> > if you run it a few times.
> > One way to fix this is to simply run the test for a few tens of
> > iterations - I don't have any better ideas.
> >
> > https://codereview.appspot.com/103300045/
> >
> 
> 
> -- 
> Nathan Youngman
> Email: mailto:nj@nathany.com
> Web: http://www.nathany.com

[tilaks, 2014-06-23 18:59:10]

Thanks for taking the time to verify this test. I've made the changes you
suggested; the test is now darwin-specific.

On 2014/06/22 05:21:48, nathany wrote:
> Running the test several times on OS X 10.9.3 does result in a data race for
me
> prior to the fix in this CL. I don't see any data races with the fix applied.
> 
> go test -test.run=TestConcurrentRemovalOfWatch -test.cpu=1,1,1,1,1 -race
> 
> As FreeBSD doesn't have the race detector, I can't confirm whether it was an
> issue there.
> 
> * The test does pass on Windows and Linux, but maybe this test should be in a
> darwin/bsd specific file?
> * I think the description of this CL should include the test run line above
that
> made it fail.
> * A comment to "See https://codereview.appspot.com/103300045/%22 may be
> appropriate in this case.
> 
> Thanks!
> 
> 
> > On Friday, June 20, 2014, <mailto:tilak89@gmail.com> wrote:
> > >
> > > Nathan, thanks for running the test. You're right - it doesn't seem to
> > > hit the race condition every time, although you should see the failure
> > > if you run it a few times.
> > > One way to fix this is to simply run the test for a few tens of
> > > iterations - I don't have any better ideas.
> > >
> > > https://codereview.appspot.com/103300045/
> > >
> > 
> > 
> > -- 
> > Nathan Youngman
> > Email: mailto:nj@nathany.com
> > Web: http://www.nathany.com

[nathany, 2014-06-28 19:55:45]

It looks like your file needs to be named fsnotify_darwin_test.go, otherwise
that test doesn't run at all.

I found this out by adding t.Fatal("oh oh") to the test.

On 2014/06/23 18:59:10, tilaks wrote:
> Thanks for taking the time to verify this test. I've made the changes you
> suggested; the test is now darwin-specific.

[nathany, 2014-08-18 04:12:16]

On 2014/06/28 19:55:45, nathany wrote:
> It looks like your file needs to be named fsnotify_darwin_test.go, otherwise
> that test doesn't run at all.
> 
> I found this out by adding t.Fatal("oh oh") to the test.
> 
> On 2014/06/23 18:59:10, tilaks wrote:
> > Thanks for taking the time to verify this test. I've made the changes you
> > suggested; the test is now darwin-specific.

tilaks. just for your information, I long ago merged this change at
https://github.com/go-fsnotify/fsnotify and made the necessary filename change
for the darwin test.

[nathany, 2014-08-18 05:42:16]

> tilaks. just for your information, I long ago merged this change at
> https://github.com/go-fsnotify/fsnotify and made the necessary filename change
> for the darwin test.

If you wish to abandon this CL, use: hg change -d 103300045.

See https://groups.google.com/forum/#!topic/golang-dev/-__vD-kOF5s for more on
the transition.

[gobot, 2014-12-19 05:08:16]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/103300045 is best) in the description in your
new CL.

Thanks very much.