From f81df212a468bea7f9b07d27c0abfbcafd0c5ea3 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Wed, 26 Mar 2025 13:29:35 +0100 Subject: [PATCH 1/7] Add test to expose bug that `IndexingRescanner` generates events outside the watch scope --- .../engineering/swat/watch/TestDirectory.java | 5 +- .../overflows/IndexingRescannerTests.java | 77 +++++++++++++++++++ 2 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 src/test/java/engineering/swat/watch/impl/overflows/IndexingRescannerTests.java diff --git a/src/test/java/engineering/swat/watch/TestDirectory.java b/src/test/java/engineering/swat/watch/TestDirectory.java index 6df4d47c..8628cc2a 100644 --- a/src/test/java/engineering/swat/watch/TestDirectory.java +++ b/src/test/java/engineering/swat/watch/TestDirectory.java @@ -37,12 +37,11 @@ import java.util.Comparator; import java.util.List; -class TestDirectory implements Closeable { +public class TestDirectory implements Closeable { private final Path testDirectory; private final List testFiles; - - TestDirectory() throws IOException { + public TestDirectory() throws IOException { testDirectory = Files.createTempDirectory("java-watch-test"); List testFiles = new ArrayList<>(); add3Files(testFiles, testDirectory); diff --git a/src/test/java/engineering/swat/watch/impl/overflows/IndexingRescannerTests.java b/src/test/java/engineering/swat/watch/impl/overflows/IndexingRescannerTests.java new file mode 100644 index 00000000..0eb24e7c --- /dev/null +++ b/src/test/java/engineering/swat/watch/impl/overflows/IndexingRescannerTests.java @@ -0,0 +1,77 @@ +package engineering.swat.watch.impl.overflows; + +import static org.awaitility.Awaitility.await; + +import java.io.IOException; +import java.util.concurrent.ForkJoinPool; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.awaitility.Awaitility; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import engineering.swat.watch.OnOverflow; +import engineering.swat.watch.TestDirectory; +import engineering.swat.watch.TestHelper; +import engineering.swat.watch.WatchEvent; +import engineering.swat.watch.WatchScope; +import engineering.swat.watch.Watcher; +import engineering.swat.watch.impl.EventHandlingWatch; + +class IndexingRescannerTests { + + private TestDirectory testDir; + + @BeforeEach + void setup() throws IOException { + testDir = new TestDirectory(); + } + + @AfterEach + void cleanup() { + if (testDir != null) { + testDir.close(); + } + } + + @BeforeAll + static void setupEverything() { + Awaitility.setDefaultTimeout(TestHelper.NORMAL_WAIT); + } + + @Test + void onlyEventsForFilesInScopeAreIssued() throws IOException, InterruptedException { + var path = testDir.getTestDirectory(); + + // Prepare a watch that monitors only the children (not all descendants) + // of `path` + var eventsOnlyForChildren = new AtomicBoolean(true); + var watchConfig = Watcher.watch(path, WatchScope.PATH_AND_CHILDREN) + .approximate(OnOverflow.NONE) // Disable the auto-handler here; we'll have an explicit one below + .on(e -> { + if (e.getRelativePath().getNameCount() > 1) { + eventsOnlyForChildren.set(false); + } + }); + + try (var watch = (EventHandlingWatch) watchConfig.start()) { + // Create a rescanner that initially indexes all descendants (not + // only the children) of `path` + var rescanner = new IndexingRescanner( + ForkJoinPool.commonPool(), path, + WatchScope.PATH_AND_ALL_DESCENDANTS); + + // Trigger a rescan. As only the children (not all descendants) of + // `path` are watched, the rescan should issue events only for those + // children. + var overflow = new WatchEvent(WatchEvent.Kind.OVERFLOW, path); + rescanner.accept(watch, overflow); + Thread.sleep(TestHelper.SHORT_WAIT.toMillis()); + + await("No events for non-children descendants should have been issued") + .until(eventsOnlyForChildren::get); + } + } +} From 5f7329da581aa9a8944c9162cb072dcd515365c5 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Wed, 26 Mar 2025 13:44:09 +0100 Subject: [PATCH 2/7] Fix bug that `IndexingRescanner` generates events outside the watch scope --- .../impl/overflows/IndexingRescanner.java | 43 ++++++++++++++----- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java index 1af09527..2d5684d0 100644 --- a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java +++ b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java @@ -32,6 +32,8 @@ import java.nio.file.Path; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.FileTime; +import java.util.ArrayDeque; +import java.util.Deque; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -82,20 +84,29 @@ protected MemorylessRescanner.Generator newGenerator(Path path, WatchScope scope } protected class Generator extends MemorylessRescanner.Generator { - // Field to keep track of the paths that are visited during the current - // rescan. After the visit, the `DELETED` events that happened since the - // previous rescan can be approximated. - private Set visited = new HashSet<>(); + // Field to keep track of (a stack of) the paths that are visited during + // the current rescan (one frame for each nested subdirectory), to + // approximate `DELETED` events that happened since the previous rescan. + // Instances of this class are supposed to be used non-concurrently, so + // no synchronization to access this field is needed. + private Deque> visited = new ArrayDeque<>(); public Generator(Path path, WatchScope scope) { super(path, scope); + this.visited.push(new HashSet<>()); + } + + private void addToPeeked(Deque> deque, T t) { + var peeked = deque.peek(); + if (peeked != null) { + peeked.add(t); + } } // -- MemorylessRescanner.Generator -- @Override protected void generateEvents(Path path, BasicFileAttributes attrs) { - visited.add(path); var lastModifiedTimeOld = index.get(path); var lastModifiedTimeNew = attrs.lastModifiedTime(); @@ -111,14 +122,26 @@ else if (lastModifiedTimeOld.compareTo(lastModifiedTimeNew) < 0) { } } + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { + addToPeeked(visited, dir); + visited.push(new HashSet<>()); + return super.preVisitDirectory(dir, attrs); + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + addToPeeked(visited, file); + return super.visitFile(file, attrs); + } + @Override public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { - // If the visitor is back at the root of the rescan, then the time - // is right to issue `DELETED` events based on the set of `visited` - // paths. - if (dir.equals(path)) { + // Issue `DELETED` events based on the set of paths visited in `dir` + var visitedInDir = visited.pop(); + if (visitedInDir != null) { for (var p : index.keySet()) { - if (p.startsWith(path) && !visited.contains(p)) { + if (dir.equals(p.getParent()) && !visitedInDir.contains(p)) { events.add(new WatchEvent(WatchEvent.Kind.DELETED, p)); } } From ecc8ee6981893d28e150775da1b7869cf58819e4 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Wed, 26 Mar 2025 13:47:58 +0100 Subject: [PATCH 3/7] Remove redundant initialization statement --- src/test/java/engineering/swat/watch/RecursiveWatchTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/engineering/swat/watch/RecursiveWatchTests.java b/src/test/java/engineering/swat/watch/RecursiveWatchTests.java index 5faa2122..ca3b4934 100644 --- a/src/test/java/engineering/swat/watch/RecursiveWatchTests.java +++ b/src/test/java/engineering/swat/watch/RecursiveWatchTests.java @@ -140,5 +140,4 @@ void deleteOfFileInDirectoryShouldBeVisible() throws IOException { .untilTrue(seen); } } - } From 214d0acabb38be6a4747942c1bddc7b08f1ea60f Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Wed, 26 Mar 2025 13:58:01 +0100 Subject: [PATCH 4/7] Make a few minor improvements to improve code quality, comments, and license --- .../impl/overflows/IndexingRescanner.java | 1 - .../overflows/IndexingRescannerTests.java | 40 ++++++++++++++++--- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java index 2d5684d0..bdff8047 100644 --- a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java +++ b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java @@ -93,7 +93,6 @@ protected class Generator extends MemorylessRescanner.Generator { public Generator(Path path, WatchScope scope) { super(path, scope); - this.visited.push(new HashSet<>()); } private void addToPeeked(Deque> deque, T t) { diff --git a/src/test/java/engineering/swat/watch/impl/overflows/IndexingRescannerTests.java b/src/test/java/engineering/swat/watch/impl/overflows/IndexingRescannerTests.java index 0eb24e7c..d3913881 100644 --- a/src/test/java/engineering/swat/watch/impl/overflows/IndexingRescannerTests.java +++ b/src/test/java/engineering/swat/watch/impl/overflows/IndexingRescannerTests.java @@ -1,3 +1,29 @@ +/* + * BSD 2-Clause License + * + * Copyright (c) 2023, Swat.engineering + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ package engineering.swat.watch.impl.overflows; import static org.awaitility.Awaitility.await; @@ -45,8 +71,8 @@ static void setupEverything() { void onlyEventsForFilesInScopeAreIssued() throws IOException, InterruptedException { var path = testDir.getTestDirectory(); - // Prepare a watch that monitors only the children (not all descendants) - // of `path` + // Configure a non-recursive directory watch that monitors only the + // children (not all descendants) of `path` var eventsOnlyForChildren = new AtomicBoolean(true); var watchConfig = Watcher.watch(path, WatchScope.PATH_AND_CHILDREN) .approximate(OnOverflow.NONE) // Disable the auto-handler here; we'll have an explicit one below @@ -58,14 +84,16 @@ void onlyEventsForFilesInScopeAreIssued() throws IOException, InterruptedExcepti try (var watch = (EventHandlingWatch) watchConfig.start()) { // Create a rescanner that initially indexes all descendants (not - // only the children) of `path` + // only the children) of `path`. The resulting initial index is an + // overestimation of the files monitored by the watch. var rescanner = new IndexingRescanner( ForkJoinPool.commonPool(), path, WatchScope.PATH_AND_ALL_DESCENDANTS); - // Trigger a rescan. As only the children (not all descendants) of - // `path` are watched, the rescan should issue events only for those - // children. + // Trigger a rescan. Because only the children (not all descendants) + // of `path` are watched, the rescan should issue events only for + // those children (even though the initial index contains entries + // for all descendants). var overflow = new WatchEvent(WatchEvent.Kind.OVERFLOW, path); rescanner.accept(watch, overflow); Thread.sleep(TestHelper.SHORT_WAIT.toMillis()); From 5893890acdcdc6bd139cf5d66699629cbb46f6fd Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Wed, 26 Mar 2025 14:02:38 +0100 Subject: [PATCH 5/7] Fix initialization of `IndexingRescanner.Generator` --- .../engineering/swat/watch/impl/overflows/IndexingRescanner.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java index bdff8047..d23cb910 100644 --- a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java +++ b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java @@ -93,6 +93,7 @@ protected class Generator extends MemorylessRescanner.Generator { public Generator(Path path, WatchScope scope) { super(path, scope); + visited.push(new HashSet<>()); // Initial set for content of `path` } private void addToPeeked(Deque> deque, T t) { From 98225e38ca73bc03193891e96aac98648a33ed26 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Wed, 26 Mar 2025 14:03:42 +0100 Subject: [PATCH 6/7] Fix initialization of `IndexingRescanner.Generator` --- .../swat/watch/impl/overflows/IndexingRescanner.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java index d23cb910..fb6c70c0 100644 --- a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java +++ b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java @@ -93,7 +93,7 @@ protected class Generator extends MemorylessRescanner.Generator { public Generator(Path path, WatchScope scope) { super(path, scope); - visited.push(new HashSet<>()); // Initial set for content of `path` + this.visited.push(new HashSet<>()); // Initial set for content of `path` } private void addToPeeked(Deque> deque, T t) { From 79312dba3b24f1c2a1fb46ac1ce037df9b9a6db4 Mon Sep 17 00:00:00 2001 From: Sung-Shik Jongmans Date: Wed, 26 Mar 2025 16:52:52 +0100 Subject: [PATCH 7/7] Make field final --- .../swat/watch/impl/overflows/IndexingRescanner.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java index fb6c70c0..f9a14cf3 100644 --- a/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java +++ b/src/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java @@ -89,7 +89,7 @@ protected class Generator extends MemorylessRescanner.Generator { // approximate `DELETED` events that happened since the previous rescan. // Instances of this class are supposed to be used non-concurrently, so // no synchronization to access this field is needed. - private Deque> visited = new ArrayDeque<>(); + private final Deque> visited = new ArrayDeque<>(); public Generator(Path path, WatchScope scope) { super(path, scope);