code review 6553059: exp/inotify: cleanup ignored watch

exp/inotify: cleanup ignored watch

inotify sends the IN_IGNORED event whenever a watch was removed.
Fixes issue 2483

[mb0, 2012-09-23 12:21:00]

Hello golang-dev@googlegroups.com,

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

[dave_cheney.net, 2012-09-28 02:16:25]

On 2012/09/23 12:21:00, mb0 wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go

Thank you for this fix. I think the test needs a bit more work.

[dave_cheney.net, 2012-09-28 02:16:34]

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
File src/pkg/exp/inotify/inotify_linux.go (right):

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
src/pkg/exp/inotify/inotify_linux.go:200: if event.Mask&IN_IGNORED != 0 {
^ gofmt ?

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
File src/pkg/exp/inotify/inotify_linux_test.go (right):

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
src/pkg/exp/inotify/inotify_linux_test.go:112: }
defer watcher.Close()

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
src/pkg/exp/inotify/inotify_linux_test.go:123: for err := range watcher.Error {
Does this goroutine exit properly, or does it live on until the end of the test
run ?

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
src/pkg/exp/inotify/inotify_linux_test.go:124: t.Fatalf("error received: %s",
err)
you cannot use Fatalf inside a goroutine, use t.Errorf() instead

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
src/pkg/exp/inotify/inotify_linux_test.go:128: for event := range watcher.Event
{
Same as above

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
src/pkg/exp/inotify/inotify_linux_test.go:134: _, ok := watcher.watches[dir]
Is this safe to observe a map that may be mutated by another goroutine ?

[mb0, 2012-10-07 17:39:18]

http://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_l...
File src/pkg/exp/inotify/inotify_linux.go (right):

http://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_l...
src/pkg/exp/inotify/inotify_linux.go:156: n, err := syscall.Read(w.fd, buf[0:])
^ this will block until a new event is received.
this is only a problem if there are no watches when closing the watcher.
(otherwise the syscall.InotifyRmWatch call triggers a new IN_IGNORED event that
unblocks the syscall.Read).

could you advise me if i should use a syscall.Select or a lazy readEvents go
routine to tackle that problem?

both solutions are drafted in github.com/mb0/inotify (latest two commits).
i would like two fix both issues in the next patch. thank you!

[rsc, 2012-11-01 18:46:05]

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
File src/pkg/exp/inotify/inotify_linux.go (right):

https://codereview.appspot.com/6553059/diff/5001/src/pkg/exp/inotify/inotify_...
src/pkg/exp/inotify/inotify_linux.go:156: n, err := syscall.Read(w.fd, buf[0:])
If this only happens when closing the watcher, I would solve this by doing an
InotifyAddWatch to unblock the read. :-)

[mb0, 2012-11-01 21:01:51]

Hello golang-dev@googlegroups.com, dave@cheney.net, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-11-01 21:11:48]

seems plausible. anyone else?

https://codereview.appspot.com/6553059/diff/14001/src/pkg/exp/inotify/inotify...
File src/pkg/exp/inotify/inotify_linux.go (right):

https://codereview.appspot.com/6553059/diff/14001/src/pkg/exp/inotify/inotify...
src/pkg/exp/inotify/inotify_linux.go:88: w.AddWatch("/", IN_DELETE_SELF)
// Add a spurious watch so that the watching goroutine will return
// from syscall.Read when we remove watches below.

[mb0, 2012-11-01 21:21:53]

Hello dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2013-02-16 02:41:39]

Sorry this took three months to be reviewed. Please <ping /> in the future.

https://codereview.appspot.com/6553059/diff/15003/src/pkg/exp/inotify/inotify...
File src/pkg/exp/inotify/inotify_linux.go (right):

https://codereview.appspot.com/6553059/diff/15003/src/pkg/exp/inotify/inotify...
src/pkg/exp/inotify/inotify_linux.go:87: if len(w.watches) == 0 {
This is racy, do you need to hold w.mu ? even then, you can race after w.mu is
unlocked to call w.AddWatch

[dave_cheney.net, 2013-02-16 02:45:41]

==================
WARNING: DATA RACE
Read by goroutine 11:
  runtime.mapaccess2()
      /home/dfc/go/src/pkg/runtime/hashmap.c:944 +0x0
  exp/inotify.(*Watcher).RemoveWatch()
      /home/dfc/go/src/pkg/exp/inotify/inotify_linux.go:142 +0x7d
  exp/inotify.TestInotifyIgnored()
      /home/dfc/go/src/pkg/exp/inotify/inotify_linux_test.go:140 +0x4fe
  testing.tRunner()
      /home/dfc/go/src/pkg/testing/testing.go:315 +0xe8

Previous write by goroutine 12:
  runtime.mapdelete()
      /home/dfc/go/src/pkg/runtime/hashmap.c:1063 +0x0
  exp/inotify.(*Watcher).readEvents()
      /home/dfc/go/src/pkg/exp/inotify/inotify_linux.go:207 +0x5de

Goroutine 11 (running) created at:
  testing.RunTests()
      /home/dfc/go/src/pkg/testing/testing.go:430 +0xc7a
  testing.Main()
      /home/dfc/go/src/pkg/testing/testing.go:327 +0xcd
  main.main()
      exp/inotify/_test/_testmain.go:47 +0xda
  runtime.main()
      /home/dfc/go/src/pkg/runtime/proc.c:253 +0x8e

Goroutine 12 (running) created at:
  exp/inotify.NewWatcher()
      /home/dfc/go/src/pkg/exp/inotify/inotify_linux.go:76 +0x332
  exp/inotify.TestInotifyIgnored()
      /home/dfc/go/src/pkg/exp/inotify/inotify_linux_test.go:109 +0x76
  testing.tRunner()
      /home/dfc/go/src/pkg/testing/testing.go:315 +0xe8

==================
PASS
Found 1 data race(s)
FAIL    exp/inotify     2.074s

[mb0, 2013-02-16 11:56:24]

i pointed out the data race in the discussion mail 5 month ago. it was not
introduced by me. it only started to show because RemoveWatch is now called by
the test.

in this path i also put the isClosed check inside the lock. i think we should
not expect the user to call AddWatch and Close from the same go routine.

[dave_cheney.net, 2013-02-17 00:54:53]

LGTM modulo these comments.

https://codereview.appspot.com/6553059/diff/24001/src/pkg/exp/inotify/inotify...
File src/pkg/exp/inotify/inotify_linux.go (right):

https://codereview.appspot.com/6553059/diff/24001/src/pkg/exp/inotify/inotify...
src/pkg/exp/inotify/inotify_linux.go:92: w.add("/", IN_DELETE_SELF)
What about handling the error here ?

https://codereview.appspot.com/6553059/diff/24001/src/pkg/exp/inotify/inotify...
src/pkg/exp/inotify/inotify_linux.go:109: w.mu.Unlock()
it is fine to use defer here, the cost is minimal.

https://codereview.appspot.com/6553059/diff/24001/src/pkg/exp/inotify/inotify...
src/pkg/exp/inotify/inotify_linux.go:112: 
// add adds path to the watched file set. The caller must have already locked
w.mu.

https://codereview.appspot.com/6553059/diff/24001/src/pkg/exp/inotify/inotify...
src/pkg/exp/inotify/inotify_linux.go:149: w.mu.Unlock()
ditto

https://codereview.appspot.com/6553059/diff/24001/src/pkg/exp/inotify/inotify...
src/pkg/exp/inotify/inotify_linux.go:152: 
see comment on // add

https://codereview.appspot.com/6553059/diff/24001/src/pkg/exp/inotify/inotify...
src/pkg/exp/inotify/inotify_linux.go:156: return errors.New(fmt.Sprintf("can't
remove non-existent inotify watch for: %s", path))
fmt.Errorf might be simpler here.

[mb0, 2013-02-17 01:25:56]

Hello dave@cheney.net, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-12-18 20:22:17]

R=close

code does not exist (here) anymore