Support for path contexts + misc improvements and fixes #58

mdproctor · 2020-11-12T22:48:59Z

Bug
-Carbon returns real paths, this will not map added paths which are symbolic. To address this it turns the real path back into the user provided path.
-Fixed some tests, by making them await longer

Improvements
-When underlying root paths were deleted, it would not detect and close the Carbon deamon thread or clean up the paths or invalidate the WatchKey. This is now addressed
-Can now check if DirectoryWatcher is open or closed.
-Added pom.xml so maven sort of works, but it's a quick hack and can be improved.-

Feature
-Allow a context value to be associated with a given subtree (i.e. from root path and all child paths), and provided as part of the Event. This avoids having to search for matching roots, to lookup a context value.

-Carbon returns real paths, this will not map added paths which are symbolic. To address this it turns the real path back into the user provided path. -Fixed some tests, by making them await longer Improvements -When underlying root paths were deleted, it would not detect and close the Carbon deamon thread or clean up the paths or invalidate the WatchKey. This is now addressed -Can now check if DirectoryWatcher is open or closed. -Added pom.xml so maven sort of works, but it's a quick hack and can be improved.- Feature -Allow a context value to be associated with a given subtree (i.e. from root path and all child paths), and provided as part of the Event. This avoids having to search for matching roots, to lookup a context value.

mdproctor · 2020-11-13T02:13:58Z

I should add I only checked that the added maven pom.xml and test work. I notice the SBT scala better stuff fails, it probably needs updating.

gmethvin

Thanks for the pull request! I like the ideas but I had a few concerns.

gmethvin · 2020-11-13T01:26:21Z

core/pom.xml

@@ -0,0 +1,67 @@
+<?xml version="1.0" encoding="UTF-8"?>


Why do we need a maven build?

I'm happy to discuss alternative build systems, but I think it makes more sense to discuss this in a separate issue before we make any changes. Ideally we should choose a single build system and stick with it for everything, and not try to build the project with multiple build systems.

You don't, you can exclude it. It's just easier for me, as it was what I was familiar with, and works well in my IDE. I put it there, as it may make things easier for other people too, who don't have all the SBT/Scala stuff.

Yeah, I see your point. It just hard to make sure things are consistent between the maven and sbt builds, so it ultimately ends up creating more issues for us to fix.

I am eventually thinking about splitting out the better-files extension into a completely separate library (with separate release cycle) so maybe that's the right time to consider the maven stuff.

core/src/main/java/io/methvin/watcher/DirectoryWatcher.java

core/src/main/java/io/methvin/watchservice/MacOSXListeningWatchService.java

mdproctor · 2020-11-13T12:43:00Z

Btw I'm on gitter, if you want to chat. Left some messages for you there.

gmethvin

A few more suggestions that might simplify things

core/src/main/java/io/methvin/watcher/DirectoryChangeEvent.java

core/src/main/java/io/methvin/watcher/DirectoryWatcher.java

core/src/main/java/io/methvin/watchservice/MacOSXListeningWatchService.java

gmethvin · 2020-11-13T21:33:02Z

core/pom.xml

@@ -0,0 +1,67 @@
+<?xml version="1.0" encoding="UTF-8"?>


Yeah, I see your point. It just hard to make sure things are consistent between the maven and sbt builds, so it ultimately ends up creating more issues for us to fix.

I am eventually thinking about splitting out the better-files extension into a completely separate library (with separate release cycle) so maybe that's the right time to consider the maven stuff.

core/src/main/java/io/methvin/watchservice/MacOSXListeningWatchService.java

mdproctor · 2020-11-14T00:29:19Z

All suggestions seem reasonable. Rather than reply individually, i'll rework them over next few days and we can re-review them.

Btw one thing I don't think was necessary, was the thread I used to close the Carbon API thread. I only did it, as I wasn't sure if it would deadlock if it was done inplace. If you feel the code won't deadlock, I can do it without needing a thread.

gmethvin · 2020-11-16T04:46:22Z

I don't think the extra thread is necessary based on my understanding of where the locking is happening. Was there a specific place you were concerned it would deadlock?

mdproctor · 2020-12-10T03:01:42Z

OK, so I just removed that thread used to close the carbon API, hopefully it's ok. I wasn't sure how this was working, so was being cautious.

gmethvin · 2020-12-11T00:21:44Z

It looks like there's now a test failing on linux and windows builds:

Any idea what might be causing that?

gmethvin · 2020-12-11T00:52:48Z

core/src/main/java/io/methvin/watchservice/MacOSXListeningWatchService.java

@@ -145,6 +148,8 @@ public AbstractWatchKey register(
            flags);

    final CFRunLoopThread thread = new CFRunLoopThread(streamRef, file.toFile());
+    callback.onClose(() -> {watchKey.watchService().close(thread, callback, file); return null;});


We don't use the return value, so you can change the type to Runnable to avoid the ugly return null here.

mdproctor · 2020-12-12T14:41:12Z

It looks like there's now a test failing on linux and windows builds:

https://travis-ci.org/github/gmethvin/directory-watcher/jobs/748678776#L474

https://ci.appveyor.com/project/gmethvin/directory-watcher/builds/36753887#L279

Any idea what might be causing that?
I don't have a linux machine, only MacOS, but I'll ask a colleague to look into it.

gmethvin

Sorry I've had limited time to look at this. I really appreciate the improvements but I also want to make sure we're confident in the changes, since several major projects rely on this library.

Looking at your tests, I see you have the line:

await().atMost(3, TimeUnit.SECONDS).until(() -> recorder.events.size() == 33);

So you're expecting 33 events from the three calls to createStructure2. But it's not obvious to me why you're expecting that number of events.

You might also want to try with hashing, since some platforms fire duplicate events. The cross-platform guarantees of this library can be relaxed when it comes to duplicate events, as long as they make sense. For example, sometimes you'll get multiple modify events instead of one, as the file is being written out.

Also, my understanding is that registeredContexts is an internal data structure to map each path registered in the WatchService to the root path that was registered in DirectoryWatcher. Actually if that's the case, maybe we should name it something more obvious like registeredPathToRootPath. The term context is actually a little confusing to me because there's something called context on the WatchEvent which only refers to the registered path in the WatchService, which will be different on Linux, for example, or any other platform that doesn't support recursive watching on the WatchService.

If you'd like, you could split off the macOS-only improvements to another PR so we can get those out sooner.

core/src/main/java/io/methvin/watcher/DirectoryWatcher.java

linux tests fix

-removed getRegisteredContexts -changed registeredContexts to registeredPathToRootPath

core/src/main/java/io/methvin/watcher/DirectoryWatcher.java

core/src/main/java/io/methvin/watchservice/MacOSXListeningWatchService.java

gmethvin · 2020-12-27T08:46:17Z

@mdproctor I made some changes that appear to fix the test failures.

The main change is that I moved the tests that delete the root directory into an isMac conditional, so they will only be run for the macOS watcher. There appears to be an inconsistency between platforms in how delete events are reported for the watched roots, so I created issue #59 to track that.

I also removed uses of assumeTrue on the number of events, since those will skip the rest of the test if the condition isn't true. I don't quite understand why you used assumeTrue there, but it seems like those values were based on observed behavior rather than specific guarantees the library is trying to provide, so they aren't really that useful anyway.

Anyway, let me know if those changes look good. If so I'll merge and release.

mdproctor force-pushed the master branch from 14528c4 to 2b231b0 Compare November 13, 2020 02:10

gmethvin reviewed Nov 13, 2020

View reviewed changes

core/src/main/java/io/methvin/watchservice/MacOSXListeningWatchService.java Outdated Show resolved Hide resolved

core/src/main/java/io/methvin/watchservice/MacOSXListeningWatchService.java Outdated Show resolved Hide resolved

Addressed all comments from PR

9dc020c

gmethvin reviewed Dec 11, 2020

View reviewed changes

-Changed interface from Callable to Runnable

de30ae5

gmethvin reviewed Dec 13, 2020

View reviewed changes

core/src/main/java/io/methvin/watcher/DirectoryWatcher.java Outdated Show resolved Hide resolved

treblereel and others added 3 commits December 19, 2020 19:47

linux tests fix

076fbeb

Merge pull request #1 from treblereel/linux_tests

22fae33

linux tests fix

-hashing added to test and events check changed from 33 to 15.

9bc2c4d

-removed getRegisteredContexts -changed registeredContexts to registeredPathToRootPath

gmethvin reviewed Dec 22, 2020

View reviewed changes

core/src/main/java/io/methvin/watcher/DirectoryWatcher.java Outdated Show resolved Hide resolved

core/src/main/java/io/methvin/watcher/DirectoryWatcher.java Outdated Show resolved Hide resolved

gmethvin reviewed Dec 22, 2020

View reviewed changes

-minor corrections

c886a38

gmethvin force-pushed the master branch 3 times, most recently from 9cf59fc to 99017cc Compare December 27, 2020 08:09

Fix tests

9f09a9c

gmethvin force-pushed the master branch from 99017cc to 9f09a9c Compare December 27, 2020 08:39

gmethvin added 2 commits December 27, 2020 03:32

Merge branch 'master' into mdproctor/master

0ef8454

remove pom.xml

7690026

gmethvin merged commit 168cd31 into gmethvin:master Dec 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for path contexts + misc improvements and fixes #58

Support for path contexts + misc improvements and fixes #58

mdproctor commented Nov 12, 2020

mdproctor commented Nov 13, 2020

gmethvin left a comment

gmethvin Nov 13, 2020

mdproctor Nov 13, 2020 •

edited

Loading

gmethvin Nov 13, 2020

mdproctor commented Nov 13, 2020

gmethvin left a comment

gmethvin Nov 13, 2020

mdproctor commented Nov 14, 2020 •

edited

Loading

gmethvin commented Nov 16, 2020

mdproctor commented Dec 10, 2020

gmethvin commented Dec 11, 2020

gmethvin Dec 11, 2020

mdproctor Dec 12, 2020

mdproctor commented Dec 12, 2020

gmethvin left a comment •

edited

Loading

gmethvin commented Dec 27, 2020 •

edited

Loading

Support for path contexts + misc improvements and fixes #58

Support for path contexts + misc improvements and fixes #58

Conversation

mdproctor commented Nov 12, 2020

mdproctor commented Nov 13, 2020

gmethvin left a comment

Choose a reason for hiding this comment

gmethvin Nov 13, 2020

Choose a reason for hiding this comment

mdproctor Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

gmethvin Nov 13, 2020

Choose a reason for hiding this comment

mdproctor commented Nov 13, 2020

gmethvin left a comment

Choose a reason for hiding this comment

gmethvin Nov 13, 2020

Choose a reason for hiding this comment

mdproctor commented Nov 14, 2020 • edited Loading

gmethvin commented Nov 16, 2020

mdproctor commented Dec 10, 2020

gmethvin commented Dec 11, 2020

gmethvin Dec 11, 2020

Choose a reason for hiding this comment

mdproctor Dec 12, 2020

Choose a reason for hiding this comment

mdproctor commented Dec 12, 2020

gmethvin left a comment • edited Loading

Choose a reason for hiding this comment

gmethvin commented Dec 27, 2020 • edited Loading

mdproctor Nov 13, 2020 •

edited

Loading

mdproctor commented Nov 14, 2020 •

edited

Loading

gmethvin left a comment •

edited

Loading

gmethvin commented Dec 27, 2020 •

edited

Loading