Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go.exp/fsnotify: data race #8282

Closed
ostness opened this issue Jun 24, 2014 · 10 comments
Closed

go.exp/fsnotify: data race #8282

ostness opened this issue Jun 24, 2014 · 10 comments

Comments

@ostness
Copy link

ostness commented Jun 24, 2014

What does 'go version' print?
go version go1.3 darwin/amd64
Mac OS X 10.9.3

What steps reproduce the problem?
If possible, include a link to a program on play.golang.org.

1. http://play.golang.org/p/8WDnSAxoK2
2. run the code with -race to watch a FILE
3. sed -i '' -e 's,A,B,g' FILE # "sed -i" removes the FILE, then creates a new
with that name

What happened?
==================
WARNING: DATA RACE
Write by main goroutine:
  code.google.com/p/go.exp/fsnotify.(*Watcher).removeWatch()
      ~/.gvm/pkgsets/go1.3/fswatch/src/code.google.com/p/go.exp/fsnotify/fsnotify_bsd.go:245 +0x3bf
  code.google.com/p/go.exp/fsnotify.(*Watcher).Close()
      ~/.gvm/pkgsets/go1.3/fswatch/src/code.google.com/p/go.exp/fsnotify/fsnotify_bsd.go:124 +0x22f
  main.main()
      ~/watch.go:18 +0x10d

Previous read by goroutine 5:
  code.google.com/p/go.exp/fsnotify.(*Watcher).removeWatch()
      ~/.gvm/pkgsets/go1.3/fswatch/src/code.google.com/p/go.exp/fsnotify/fsnotify_bsd.go:250 +0x641
  code.google.com/p/go.exp/fsnotify.(*Watcher).readEvents()
      ~/.gvm/pkgsets/go1.3/fswatch/src/code.google.com/p/go.exp/fsnotify/fsnotify_bsd.go:379 +0xc0d

Goroutine 5 (running) created at:
  code.google.com/p/go.exp/fsnotify.NewWatcher()
      ~/.gvm/pkgsets/go1.3/fswatch/src/code.google.com/p/go.exp/fsnotify/fsnotify_bsd.go:102 +0x3d6
  main.main()
      ~/watch.go:9 +0x30
==================
Found 1 data race(s)
exit status 66

fsnotify correctly detects the deletion event, then the readEvents() goroutine and
Close() race for Watcher.watches

What should have happened instead?
My use case is to watch a file that may be deleted and recreated, thus
I have to wait for an event and immediately Close() the watcher and make a new one.
That would be a shame to time.Sleep or something.

Please provide any additional information below.
github.com/howeyc/fsnotify races exactly the same
@dvyukov
Copy link
Member

dvyukov commented Jun 24, 2014

Comment 1:

Labels changed: added threadsanitizer.

@nathany
Copy link
Contributor

nathany commented Jun 29, 2014

Comment 2:

Does this CL solve the data race for you? 
https://golang.org/cl/103300045/

@ostness
Copy link
Author

ostness commented Jun 29, 2014

Comment 3:

No, not really. These changes committed already into howeyc/fsnotify master, and using
it the data race is unchanged

@nathany
Copy link
Contributor

nathany commented Jun 29, 2014

Comment 4:

Could this be an issue with readEvents() blocking?
The example included with fsnotify reads events off the channel in a separate goroutine:
https://code.google.com/p/go/source/browse/fsnotify/example_test.go?repo=exp
Whereas your play example doesn't use a separate goroutine.
Also see: howeyc/fsnotify#7

@ostness
Copy link
Author

ostness commented Jun 29, 2014

Comment 5:

> Could this be an issue with readEvents() blocking?
I just may have spotted a wrong mutex use:
at fsnotify_bsd.go:120 the w.pmut is locked to get a copy of w.watches (which is fine)
but
the pmut "Protects access to paths and finfo" and
the wmut "Protects access to watches."
So the wmut should have been used unless I'm reading it all wrong.
The pmut use is present in howeyc/fsnotify as well.
> The example included with fsnotify reads events off the channel in a separate
goroutine:
Ok, it may have been an intended api and it doesn't make my day any different.
http://play.golang.org/p/UZr1ODb8ja
The code again does one thing: receives an event and closes and exits,
and I cannot comprehend the difference between the two plays BUT running the new one
with howeyc/fsnotify no data races observed.
with os/fsnotify the exactly same data race present.
So that's a difference between the two packages.
The issue indeed may have been fixed (with the CL changes) in howeyc/fsnotify.
For now using howeyc/fsnotify with a separate goroutine does it for me.

@nathany
Copy link
Contributor

nathany commented Jul 5, 2014

Comment 6:

Thanks for identifying the incorrect mutex. I've opened a pull request to
howeyc/fsnotify howeyc/fsnotify#100 
Eventually I will get a CL to go.exp as well (I'm not familiar enough to open multiple
CLs at once, so waiting for review of https://golang.org/cl/105370044/).

@ostness
Copy link
Author

ostness commented Jul 5, 2014

Comment 7:

Great.
Cheers.

@griesemer
Copy link
Contributor

Comment 8:

Labels changed: added repo-exp.

@nathany
Copy link
Contributor

nathany commented Oct 2, 2014

Comment 9:

This fix was merged into fsnotify some time ago:
https://github.com/go-fsnotify/fsnotify/blob/master/CHANGELOG.md#dev--2014-07-04
More recently I've reworked kqueue to only use a single mutex.
I'm no longer maintaining go.exp/fsnotify:
https://groups.google.com/forum/#!msg/golang-dev/-__vD-kOF5s/
Feel free to close this issue.

@ostness
Copy link
Author

ostness commented Dec 8, 2014

This issue is long gone.

@ostness ostness closed this as completed Dec 8, 2014
@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants