Some fixes regarding unwatching and rewatching a path #129

imsodin · 2017-08-31T09:43:57Z

This started in issue #124 and replaces the previously split up PRs. This is bundled as the fixed problems come up in the same tests, but are in separate commits to make each change-diff about a single problem. The commits should explain what is going on.

imsodin · 2017-10-01T15:38:52Z

@rjeczalik Any feedback?
I need this downstream in Syncthing as tests expose some of these bugs. It would be nice if I didn't have to deviate from this repo.

rjeczalik · 2017-10-02T17:37:05Z

watcher_fsevents.go

@@ -189,9 +187,6 @@ func newWatcher(c chan<- EventInfo) watcher {
 }

 func (fse *fsevents) watch(path string, event Event, isrec int32) (err error) {
-	if path, err = canonical(path); err != nil {


I don't think we can remove this yet, until the functionality is moved to tree.

Morever I think it'd need to stay here because of fse.watches - if we put a watch on a dir and a symlink to it, fse.watches must not hold two watches but only one.

Isn't it already in the tree:
https://github.com/rjeczalik/notify/blob/master/tree_recursive.go#L164

Cleanpath subsequently calls canonical on the path and no other watcher implementation uses canonical.

It is, but fse.watches maintains its own path -> watches mapping, if we get rid of canonical, we may have duplicated watches.

I still don't understand it: The paths in fse.watches that were previously passed through canonical are those given as arguments to the exported (Recursive-)Re-/Watch functions, which are called from the tree, where the paths were already canonicalized in the tree - how else can duplicated paths enter fse.watches?

Other watchers maintains such a map of paths to structs as well, but don't canonicalize paths (e.g. readdcw in r.m, trigger in pthLkp).

Also the new tests fail when canonical is reintroduced, as it won't remove a non-existing path, because canonical stats the path and thus fails.

which are called from the tree, where the paths were already canonicalized in the tree

You're right, I take back my comment about duplicates - canonical is not needed here.

Also the new tests fail when canonical is reintroduced, as it won't remove a non-existing path, because canonical stats the path and thus fails.

Good point.

rjeczalik · 2017-10-02T17:37:26Z

watcher_inotify.go

+
+// if path was removed, notify already removed the watch and returns EINVAL error
+func removeInotifyWatch(fd int32, iwd int32) (err error) {
+	if _, err = unix.InotifyRmWatch(int(fd), uint32(iwd)); err != nil && err != unix.EINVAL {


rjeczalik · 2017-10-02T17:37:47Z

watcher_readdcw.go

 			r.loopevent(n, overEx)
 			if err = overEx.parent.readDirChanges(); err != nil {
 				// TODO: error handling
 			}
 		}
+		r.loopstate(overEx)


/cc @ppknap

rjeczalik · 2017-10-02T17:38:06Z

watcher_readdcw.go

+			if g != nil {
+				dbgprint("unwatch: posting")
+				if err = syscall.PostQueuedCompletionStatus(r.cph, 0, 0, (*syscall.Overlapped)(unsafe.Pointer(g.ovlapped))); err != nil {
+					wd.filter &^= stateUnwatch


/cc @ppknap

rjeczalik · 2017-10-02T17:38:37Z

watcher_trigger.go

@@ -56,6 +57,19 @@ type trigger interface {
 	IsStop(n interface{}, err error) bool
 }

+// trgWatched is a the base data structure representing watched file/directory.
+// The platform specific full data structure (watched) must embed this type.
+type trgWatched struct {


/cc @pblaszczyk

I think Pawel's review is not needed here since this struct is just a "base" for watched type from Kqueue's and FEN impls.

rjeczalik · 2017-10-02T17:39:28Z

@imsodin Changes look good to me, except the canonical part in FSEvents watcher. How about we can revert the deletion of it for now?

rjeczalik · 2017-10-02T17:44:54Z

And sorry it took so long 😇

ppknap

LGTM, @imsodin thank you for spending time on this!

ppknap · 2017-10-02T17:50:08Z

watcher_inotify.go

+
+// if path was removed, notify already removed the watch and returns EINVAL error
+func removeInotifyWatch(fd int32, iwd int32) (err error) {
+	if _, err = unix.InotifyRmWatch(int(fd), uint32(iwd)); err != nil && err != unix.EINVAL {


ppknap · 2017-10-02T17:50:28Z

watcher_readdcw.go

 			r.loopevent(n, overEx)
 			if err = overEx.parent.readDirChanges(); err != nil {
 				// TODO: error handling
 			}
 		}
+		r.loopstate(overEx)


ppknap · 2017-10-02T17:51:49Z

watcher_readdcw.go

 	}
 }

 // TODO(pknap) : doc
 func (r *readdcw) loopstate(overEx *overlappedEx) {
+	r.Lock()
+	defer r.Unlock()
 	filter := atomic.LoadUint32(&overEx.parent.parent.filter)


probably we won't need atomic here anymore.

Probably the same in nonStateWatched, as it is only called when the lock is held, right?
https://github.com/rjeczalik/notify/pull/129/files/45213bd71c5145dd70d258df0acaf721b59a7d90#diff-ac809f36d763355597370ab566b6d71aR478

indeed 👍 🥇

ppknap · 2017-10-02T17:53:23Z

watcher_readdcw.go

+			if g != nil {
+				dbgprint("unwatch: posting")
+				if err = syscall.PostQueuedCompletionStatus(r.cph, 0, 0, (*syscall.Overlapped)(unsafe.Pointer(g.ovlapped))); err != nil {
+					wd.filter &^= stateUnwatch


imsodin · 2017-10-03T17:00:36Z

@rjeczalik Don't worry about the time - I am just glad to get feedback!

About the canonical stuff see my response to your comment on the code:
#129 (comment)

… timeout

…moved

imsodin · 2017-10-04T09:15:42Z

About the rebase: I just spotted and undid a changed comment that belongs to #128, everything else did not change.

rjeczalik · 2017-10-04T16:12:34Z

Thanks for the fixes and improvements @imsodin!

Zillode · 2017-10-05T14:08:03Z

👍

This was referenced Aug 31, 2017

readdcw: Check state on every loop iteration #127

Closed

Add test for stopping on non-existent dir (ref #124) #125

Closed

imsodin added 2 commits August 31, 2017 13:37

tests: pass root dir of testing watcher through cleanpath

ca15b52

trigger: Check that no watches remain on close

7321520

rjeczalik reviewed Oct 2, 2017

View reviewed changes

ppknap approved these changes Oct 2, 2017

View reviewed changes

rjeczalik approved these changes Oct 4, 2017

View reviewed changes

imsodin added 12 commits October 4, 2017 11:12

Add tests for unwatching and then watching the same path and set test…

07c566e

… timeout

readdcw: check state on every loop iteration

cea90c3

readdcw: Fix race (missing lock)

de41129

readdcw: Manually send unwatch events when watched file/folder was re…

1687b00

…moved

fsevents: Don't canonicalize paths, tree does it already

40b317d

inotify: Handle removing of watches on non-existing paths

734d890

trigger: When unwatching, rewatch in the correct mode (dir/ndir)

9c33e90

trigger: typo when watching fails (return nil -> err)

a55fe76

trigger: Specify common watched structure in watcher_trigger.go (dedup)

e2ec277

readdcw: polish (defer r.Unlock() where appropriate)

46fab92

readdcw: Some useful debug logging

a65f5f9

readdcw: Don't use atomic when lock is held

9a90a47

rjeczalik approved these changes Oct 4, 2017

View reviewed changes

rjeczalik merged commit 1aa3b9d into rjeczalik:master Oct 4, 2017

xenoscopic mentioned this pull request Nov 17, 2017

Windows file watches can't be re-established. mutagen-io/mutagen#1

Closed

imsodin mentioned this pull request Nov 20, 2017

watcher_fsevents: Fails to remove watch when directory does no longer exist #124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some fixes regarding unwatching and rewatching a path #129

Some fixes regarding unwatching and rewatching a path #129

imsodin commented Aug 31, 2017

imsodin commented Oct 1, 2017

rjeczalik Oct 2, 2017

imsodin Oct 2, 2017

rjeczalik Oct 4, 2017

imsodin Oct 4, 2017

rjeczalik Oct 4, 2017

rjeczalik Oct 2, 2017

ppknap Oct 2, 2017

rjeczalik Oct 2, 2017

ppknap Oct 2, 2017

rjeczalik Oct 2, 2017

ppknap Oct 2, 2017

rjeczalik Oct 2, 2017

ppknap Oct 4, 2017

rjeczalik commented Oct 2, 2017

rjeczalik commented Oct 2, 2017

ppknap left a comment

ppknap Oct 2, 2017

ppknap Oct 2, 2017

ppknap Oct 2, 2017

imsodin Oct 2, 2017 •

edited

Loading

ppknap Oct 2, 2017

ppknap Oct 2, 2017

imsodin commented Oct 3, 2017

imsodin commented Oct 4, 2017

rjeczalik commented Oct 4, 2017

Zillode commented Oct 5, 2017

Some fixes regarding unwatching and rewatching a path #129

Some fixes regarding unwatching and rewatching a path #129

Conversation

imsodin commented Aug 31, 2017

imsodin commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjeczalik commented Oct 2, 2017

rjeczalik commented Oct 2, 2017

ppknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin commented Oct 3, 2017

imsodin commented Oct 4, 2017

rjeczalik commented Oct 4, 2017

Zillode commented Oct 5, 2017

imsodin Oct 2, 2017 •

edited

Loading