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

FSEvents: fix bug where rewatching path fails across channels #155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lsegal
Copy link

@lsegal lsegal commented Jun 22, 2018

The following code reproduces a bug where if you watch a parent and child directory across multiple channels and then call notify.Stop() on the either channel, any subsequent calls to notify.Watch() on a grandparent directory will fail:

package main

import (
	"fmt"
	"io/ioutil"
	"os"

	"github.com/rjeczalik/notify"
)

func watch(path string) chan notify.EventInfo {
	c := make(chan notify.EventInfo, 1)
	if err := notify.Watch(path+"...", c, notify.All); err != nil {
		panic(err)
	}
	return c
}

func main() {
	os.MkdirAll("./a/b/c", 0775)
	defer os.RemoveAll("./a")

	// watch a child and parent path across multiple channels.
	// this can happen in any order.
	ch1 := watch("./a/b/c")
	ch2 := watch("./a/b")

	// unwatch ./a/b -- this is what causes the panic on the next line.
	// note that this also fails if we notify.Stop(ch1) instead.
	notify.Stop(ch2)

	// watching ./a will now return errNotWatched.
	ch3 := watch("./a")

	// just a test to make sure watching still works when we get here.
	go func() { ioutil.WriteFile("a/b/c/d", []byte("X"), 0664) }()
	fmt.Println(<-ch1, <-ch3)
}

Fortunately we can fix this failure by simply not returning errNotWatched from unwatch(), which simply performs a no-op if there is no active stream for the path when Unwatch() is called.

@lsegal
Copy link
Author

lsegal commented Aug 30, 2018

Any updates on this? The test failure looks like a temporal issue.

The following code reproduces a bug where if you watch a parent
and child directory across multiple channels and then call
notify.Stop() on the either channel, any subsequent calls to
notify.Watch() on a grandparent directory will fail:

```go
package main

import (
	"fmt"
	"io/ioutil"
	"os"

	"github.com/rjeczalik/notify"
)

func watch(path string) chan notify.EventInfo {
	c := make(chan notify.EventInfo, 1)
	if err := notify.Watch(path+"...", c, notify.All); err != nil {
		panic(err)
	}
	return c
}

func main() {
	os.MkdirAll("./a/b/c", 0775)
	defer os.RemoveAll("./a")

	// watch a child and parent path across multiple channels.
	// this can happen in any order.
	ch1 := watch("./a/b/c")
	ch2 := watch("./a/b")

	// unwatch ./a/b -- this is what causes the panic on the next line.
	// note that this also fails if we notify.Stop(ch1) instead.
	notify.Stop(ch2)

	// watching ./a will now return errNotWatched.
	ch3 := watch("./a")

	// just a test to make sure watching still works when we get here.
	go func() { ioutil.WriteFile("a/b/c/d", []byte("X"), 0664) }()
	fmt.Println(<-ch1, <-ch3)
}
```

Fortunately we can fix this failure by simply not returning
errNotWatched from unwatch(), which simply performs a no-op
if there is no active stream for the path when Unwatch() is
called.
@rjeczalik
Copy link
Owner

Hey @lsegal! Sorry for letting this hang so long, but I'm not sure it's a proper fix - the tree implementation should not try to unwatch something that was already unwatched, thus patching fsevents only would workaround this problem for macOS. If there's a bug, it should be fixed one layer above, but I did not look into this so I can't tell for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants