golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.29k stars 17.58k forks source link

go.exp/fsnotify: data race #8282

Closed ostness closed 9 years ago

ostness commented 10 years ago
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 commented 10 years ago

Comment 1:

Labels changed: added threadsanitizer.

nathany commented 10 years ago

Comment 2:

Does this CL solve the data race for you? 
https://golang.org/cl/103300045/
ostness commented 10 years ago

Comment 3:

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

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: https://github.com/howeyc/fsnotify/issues/7
ostness commented 10 years ago

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 commented 10 years ago

Comment 6:

Thanks for identifying the incorrect mutex. I've opened a pull request to
howeyc/fsnotify https://github.com/howeyc/fsnotify/pull/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 commented 10 years ago

Comment 7:

Great.
Cheers.
griesemer commented 10 years ago

Comment 8:

Labels changed: added repo-exp.

nathany commented 10 years ago

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 commented 9 years ago

This issue is long gone.