Support watching independent files #20

jvican · 2018-08-29T17:03:49Z

As explained in the docs, JDK's watch service cannot watch independent
files and instead can only watch directories. To simulate watching
independent files, we watch their parent directories in a non-recursive
manner. This PR implements the bits to simulate this behaviour.

The strategy here is to first register all the paths that are
non-recursive, and then proceed to the recursive ones. If any of the
directories that must be watched non-recursively are subsumed by any of the
recursive directories, they automatically become recursive.

All events about directories contained in non-recursive directories are
ignored by default.

jvican

@gmethvin Let me know what you think of this approach.

jvican · 2018-08-29T17:05:13Z

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

+
+          // If a change in a directory of a non-recursive directory happens, ignore it
+          if (Files.isDirectory(childPath, NOFOLLOW_LINKS)) {
+            Path childParentPath = childPath.getParent();


Is it safe to assume here that if I do mkdirs foo/bar/baz and foo is a non-recursive directory, I'll get the event about the creation of foo/bar before the event about foo/bar/baz? Otherwise this implementation is flawed.

jvican · 2018-08-29T17:05:53Z

core/src/test/java/io/methvin/watchservice/DirectoryWatcherTest.java

+    WatchService watchService = FileSystems.getDefault().newWatchService();
+
+    int waitInMs = 500;
+    FileSystem fileSystem = new FileSystem(directoryPath)


Let's see if Travis's Xcode instance likes this test, but I don't know why it doesn't seem to pass in my Mac OS High Sierra.

jvican

I completely changed the approach and added better tests, CI is passing now. I have one question left though.

jvican · 2018-08-29T20:11:02Z

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

          Path childPath = eventPath == null ? null : keyRoots.get(key).resolve(eventPath);
-          logger.debug("{} [{}]", kind, childPath);
+          if (shouldIgnoreEvent(childPath)) {


I don't understand why we need this. My assumption is that register(_, false) will register a directory in a non-recursive way. Is that not the case? If it isn't, can we disable recursive file watching at all for these directories in particular so that we don't need to pollute the logic of the watch method?

register(_, false) technically should watch non-recursively, but currently the macOS WatchService implementation always watches recursively regardless of what modifier is passed. That's because originally the goal of this library was only to watch directories recursively. One solution is to add this same filtering logic to the MacOSXListeningWatchService, since that's the only place it should be needed.

gmethvin · 2018-08-30T02:49:01Z

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

          Path childPath = eventPath == null ? null : keyRoots.get(key).resolve(eventPath);
-          logger.debug("{} [{}]", kind, childPath);
+          if (shouldIgnoreEvent(childPath)) {


register(_, false) technically should watch non-recursively, but currently the macOS WatchService implementation always watches recursively regardless of what modifier is passed. That's because originally the goal of this library was only to watch directories recursively. One solution is to add this same filtering logic to the MacOSXListeningWatchService, since that's the only place it should be needed.

gmethvin · 2018-08-30T02:51:35Z

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

@@ -105,7 +136,7 @@ public DirectoryWatcher build() throws IOException {
      if (logger == null) {
        staticLogger();
      }
-      return new DirectoryWatcher(paths, listener, watchService, enableFileHashing, logger);
+      return new DirectoryWatcher(paths, files, listener, watchService, enableFileHashing, logger);


What if we just support regular files in paths? Is the goal just to make the API more explicit?

Yes, I think having an explicit API helps reasoning about the overhead of watching independent files.

My main motivation to make a distinction between files and directories is that the user of this API needs to split the paths to watch in directories and files. To do that, the files/dirs need to exist, and therefore by construction the user filters out non existing directories (in OSX, I have to do this in bloop because the watcher will fail with an exception if a non-existing directory is passed -- the check is in WatchablePath IIRC).

gmethvin · 2018-08-30T02:52:06Z

project/build.properties

@@ -1 +1 @@
-sbt.version=1.0.2
+sbt.version=1.2.1


jvican · 2018-08-30T09:21:31Z

Alright, CI has finally passed on OSX with the pertinent changes in its watch service. To avoid the default recursive file watching on OSX, I wanted to use FSEventStreamSetExclusionPaths but realized that's not feasible because the limit of excluded paths is set to 8 (!). Reference: https://github.com/facebook/watchman/blob/master/watcher/fsevents.cpp

So, we'll live with the current approach. This makes #22 more important for the case of watching independent files efficiently.

jvican · 2018-08-30T09:27:00Z

@gmethvin There seems to be an error in the Linux CI related to the sbt-sonatype upgrade:

[warn] 	module not found: org.xerial.sbt#sbt-sonatype;2.3
[warn] ==== typesafe-ivy-releases: tried
[warn]   https://repo.typesafe.com/typesafe/ivy-releases/org.xerial.sbt/sbt-sonatype/scala_2.12/sbt_1.0/2.3/ivys/ivy.xml
[warn] ==== sbt-plugin-releases: tried
[warn]   https://repo.scala-sbt.org/scalasbt/sbt-plugin-releases/org.xerial.sbt/sbt-sonatype/scala_2.12/sbt_1.0/2.3/ivys/ivy.xml
[warn] ==== local: tried
[warn]   /home/travis/.ivy2/local/org.xerial.sbt/sbt-sonatype/scala_2.12/sbt_1.0/2.3/ivys/ivy.xml
[warn] ==== public: tried
[warn]   https://repo1.maven.org/maven2/org/xerial/sbt/sbt-sonatype_2.12_1.0/2.3/sbt-sonatype-2.3.pom
[warn] ==== local-preloaded-ivy: tried
[warn]   /home/travis/.sbt/preloaded/org.xerial.sbt/sbt-sonatype/2.3/ivys/ivy.xml
[warn] ==== local-preloaded: tried
[warn]   file:////home/travis/.sbt/preloaded/org/xerial/sbt/sbt-sonatype_2.12_1.0/2.3/sbt-sonatype-2.3.pom
[warn] 	::::::::::::::::::::::::::::::::::::::::::::::
[warn] 	::          UNRESOLVED DEPENDENCIES         ::
[warn] 	::::::::::::::::::::::::::::::::::::::::::::::
[warn] 	:: org.xerial.sbt#sbt-sonatype;2.3: not found
[warn] 	::::::::::::::::::::::::::::::::::::::::::::::
[warn] 
[warn] 	Note: Some unresolved dependencies have extra attributes.  Check that these dependencies exist with the requested attributes.
[warn] 		org.xerial.sbt:sbt-sonatype:2.3 (scalaVersion=2.12, sbtVersion=1.0)

jvican · 2018-08-30T09:51:15Z

It seems the resolution error disappeared in the last commit. CI is green again here and PR is ready for review.

As explained in the docs, JDK's watch service cannot watch independent files and instead can only watch directories. To simulate watching independent files, we watch their parent directories in a non-recursive manner. This PR implements the bits to simulate this behaviour. We only register non-recursive paths if they have not been registered before or if they are not contained in a path that is already been watched.

The zinc implementation had some misbehaviours after the merge commits with the pipelined implementation + changes upstream in Zinc 1.x. This commit fixes those misbehaviours, keeps us updated with the latest Zinc 1.x implementation and improves the compilation task tests. Aside from that, we migrate to directory-watcher too to make sure we have a fast and reliable independent file watching, using the changes proposed in gmethvin/directory-watcher#20

jvican · 2018-09-01T07:42:37Z

@gmethvin How does this look?

gmethvin

What do you think about determining whether the watch path is recursive in the OSX service based on whether the FILE_TREE modifier is passed, rather than having an extra isRecursive on the WatchablePath? Seems like that would be more in line with the "standard" implementation and would make it easier to use the service on its own outside the context of directory-watcher.

gmethvin · 2018-09-01T22:53:16Z

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

+    /**
+     * Set multiple files to watch.
+     *
+     * Note that the watch services interface does not have the power to watch independent


I would say "Note that the JDK WatchService interface..."

gmethvin · 2018-09-01T22:54:52Z

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

+     *
+     * @param files Paths to files (they cannot be directories).
+     */
+    public Builder files(List<Path> files) {


Maybe independentFiles or regularFiles? Also let's explicitly mention directories in the documentation for paths and path, e.g. "Set a single directory to watch"

Should we change paths to directories instead?

Yeah, that's probably a good idea, to make it clear it should only be directories.

gmethvin · 2018-09-01T22:56:58Z

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

    this.logger = logger;

+    /* Register all recursive paths after the non recursive paths to invalidate


I'm confused by the comment. We are registering the files after the recursive paths here.

This comment is out of date.

gmethvin · 2018-09-01T23:02:27Z

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

+    Path parentPath = directory.getParent();
+    for (Path nonRecursivePath : nonRecursivePathRoots) {
+      if (parentPath.equals(nonRecursivePath) && directory.startsWith(nonRecursivePath)) {
+        ignoreEvent = true;


Any reason why you used a variable as opposed to return true here?

Nope, I can do return true here instead

gmethvin · 2018-09-01T23:03:57Z

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

+  private void registerRootsForFiles(List<Path> files) throws IOException {
+    for (Path file: files) {
+      Path parentFile = file.getParent();
+     if (!watchingPathRoots.contains(parentFile)) {


weird indentation (I should probably set up a Java formatter so we don't have to worry about this)

Yeah, I missed this. A java formatter or an style we can use inside IntelliJ would be ideal.

jvican · 2018-09-02T11:18:08Z

What do you think about determining whether the watch path is recursive in the OSX service based on whether the FILE_TREE modifier is passed, rather than having an extra isRecursive on the WatchablePath? Seems like that would be more in line with the "standard" implementation and would make it easier to use the service on its own outside the context of directory-watcher.

Do you mean adding isRecursive to the constructor of the OSX watch service? I think it's better as it is because you usually want to control this at the path level. With regards to reusability, watchable path already appears in the public API of the watch service, so it should be reusable.

I'll act on the rest of your feedback soon.

gmethvin · 2018-09-02T18:36:07Z

Do you mean adding isRecursive to the constructor of the OSX watch service? I think it's better as it is because you usually want to control this at the path level.

No, it should be on a path level, but we can simply check for the ExtendedWatchEventModifier.FILE_TREE modifier, which we are already passing to the Watchable#register method. That requires us to do three things:

Update AbstractWatchService#register to take an additional WatchEvent.Modifier... varargs at the end.
Update WatchablePath#register to pass along the modifiers we give it (which are currently ignored).
Update MacOSXListeningWatchService#register to check for the FILE_TREE modifier, and keep track of which watchables are registered as recursive. This could be as simple as maintaining another Set<Path> of the recursive paths (or separate sets of recursive and non-recursive paths).

I like this approach because it brings our implementation more in line with the default JDK implementation. It also makes it easier to support additional modifiers in the future if we choose to.

jvican · 2018-09-03T08:49:38Z

Makes sense, I'll have a look later this week.

gmethvin · 2021-01-23T03:42:01Z

@jvican I'm going to close this for now since it's been inactive for a while and the changes are out of date, but feel free to pick it up again later if you're still interested.

jvican · 2021-01-23T10:12:18Z

Sounds good, thanks Greg.

jvican commented Aug 29, 2018

View reviewed changes

jvican force-pushed the topic/watch-independent-files branch 3 times, most recently from 43cf8bf to 88e1ee5 Compare August 29, 2018 20:03

jvican commented Aug 29, 2018

View reviewed changes

gmethvin reviewed Aug 30, 2018

View reviewed changes

jvican force-pushed the topic/watch-independent-files branch 5 times, most recently from d035b0a to 0129ee0 Compare August 30, 2018 09:13

Update to sbt 1.2.1

7de933f

jvican force-pushed the topic/watch-independent-files branch from 0129ee0 to efbe696 Compare August 30, 2018 09:21

jvican force-pushed the topic/watch-independent-files branch from efbe696 to f584fcb Compare August 30, 2018 09:28

jvican added 2 commits August 30, 2018 11:52

Add usage to README

4c6a9d0

jvican force-pushed the topic/watch-independent-files branch from 594e377 to 4c6a9d0 Compare August 30, 2018 09:52

gmethvin reviewed Sep 1, 2018

View reviewed changes

gmethvin closed this Jan 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support watching independent files #20

Support watching independent files #20

jvican commented Aug 29, 2018

jvican left a comment

jvican Aug 29, 2018

jvican Aug 29, 2018

jvican left a comment

jvican Aug 29, 2018

gmethvin Aug 30, 2018

gmethvin Aug 30, 2018

gmethvin Aug 30, 2018

jvican Aug 30, 2018

gmethvin Aug 30, 2018

jvican commented Aug 30, 2018 •

edited

Loading

jvican commented Aug 30, 2018

jvican commented Aug 30, 2018

jvican commented Sep 1, 2018

gmethvin left a comment •

edited

Loading

gmethvin Sep 1, 2018

gmethvin Sep 1, 2018

jvican Sep 2, 2018

gmethvin Sep 2, 2018

gmethvin Sep 1, 2018

jvican Sep 2, 2018

gmethvin Sep 1, 2018

jvican Sep 2, 2018

gmethvin Sep 1, 2018

jvican Sep 2, 2018

jvican commented Sep 2, 2018

gmethvin commented Sep 2, 2018

jvican commented Sep 3, 2018

gmethvin commented Jan 23, 2021

jvican commented Jan 23, 2021

		this.logger = logger;

		/* Register all recursive paths after the non recursive paths to invalidate

		@@ -1 +1 @@
		sbt.version=1.0.2
		sbt.version=1.2.1

Support watching independent files #20

Support watching independent files #20

Conversation

jvican commented Aug 29, 2018

jvican left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvican commented Aug 30, 2018 • edited Loading

jvican commented Aug 30, 2018

jvican commented Aug 30, 2018

jvican commented Sep 1, 2018

gmethvin left a comment • edited Loading

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

jvican commented Sep 2, 2018

gmethvin commented Sep 2, 2018

jvican commented Sep 3, 2018

gmethvin commented Jan 23, 2021

jvican commented Jan 23, 2021

jvican commented Aug 30, 2018 •

edited

Loading

gmethvin left a comment •

edited

Loading