Initial pure JDK implementation #1

DavyLandman · 2024-09-13T19:40:41Z

On-and-off I've been working on a nice reference implementation of a file changes watcher. Both the raw api, and the tests I could come up with.

The future will bring windows & linux (and maybe osx) specific implementation that can benefit from the native kernel api.

todo:

add license
setup deployment to somewhere like maven.
add code examples to the readme
review log levels

This better reflects other recursive implementations

DavyLandman · 2024-09-25T14:26:39Z

@sungshik thanks for the thorough review, I think I've address all comments.

DavyLandman · 2024-09-25T14:34:14Z

I'm still doubting the exact interface of the Watcher class though, curious on your thoughts.

sungshik

I think the interface of the Watcher class is much clearer now; well done! My only suggestion is to rename the enum values a bit (but this is really a matter of taste, so just see how you feel about it). Longer term (not this PR), it could be nice to consider abstracting away also the usage of java.nio.file.Path in the public methods of Watcher and use, for instance, Rascal(-style) locations (if there's a use case for this, but I imagine there could be).

I also had a look at the other changes, and it all looks fine, except I think there are still races in BundledSubscription. I tried to come up with a fix, but it's tricky to do it without bigger synchronization blocks. Here's a proposal. The correctness guarantee I can give is: I'm not convinced it's wrong 😉. Also, with more effort, it might be possible to make the synchronized blocks a bit smaller (e.g., pull active.remove and active.add outside).

src/main/java/engineering/swat/watch/Watcher.java

src/main/java/engineering/swat/watch/impl/BundledSubscription.java

src/main/java/engineering/swat/watch/impl/JDKPoller.java

src/main/java/engineering/swat/watch/impl/JDKFileWatcher.java

src/main/java/engineering/swat/watch/WatchScope.java

src/main/java/engineering/swat/watch/impl/BundledSubscription.java

…d lock

Looks like it might be a bug in the JDK registry

…rker pools

…stry api

sungshik

Had a look at the tests. Just a few small things 👍

sungshik · 2024-10-14T09:58:45Z

src/test/java/engineering/swat/watch/DeleteLockTests.java

+        void run(Path target) throws IOException;
+    }
+
+    private static void recursiveDelete(Path target) throws IOException {


There's also a method deleteAllFiles in class TestDirectory

src/test/java/engineering/swat/watch/RecursiveWatchTests.java

src/test/java/engineering/swat/watch/SingleDirectoryTests.java

src/test/java/engineering/swat/watch/SingleFileTests.java

src/test/java/engineering/swat/watch/TestHelper.java

src/test/java/engineering/swat/watch/TortureTests.java

sungshik

And a review of the non-test code since last review. Looks good!

src/main/java/engineering/swat/watch/impl/BundledSubscription.java

Co-authored-by: sungshik <[email protected]>

…s without breaking binary compatibility

DavyLandman added 30 commits November 20, 2023 16:59

Started initial implementation

17f0cf7

First working test of directory watcher

bb6c56f

Improved tests by removing sleeps

860ac75

Implemented initial recursive support

d3a47b4

Simplified recursive watcher

e6fc78a

Simplified JDKPoller singleton and avoided extra thread

4313b3e

Added recursive tests

82615a3

Implemented virtual events for new nested directories and files

a0ea7db

Got checker framework to properly work

a12b50e

Added initial support for overflow event

d58eff4

Rewrote tests to start with a fresh directory

4f22795

Added full event support to the watcher interface

4b3c167

Making sure not to leak watches

2194412

Added overflow support for the recursive watcher

515a56a

Refactoring the functions to make them smaller

6812d78

Rewrote recursive watcher to report to relative root

aff2b72

This better reflects other recursive implementations

Execute syntatic events in the correct order

829e506

Added torture tests to make sure everything works, even in very busy IO

c0a7f3f

Cleanup of code

0858ca2

Bundle registered watches to avoid duplicate FS watches

46acd38

Implemented single file support

de3ea33

Improve how long we wait for failure condition

939e57b

Made the poller loop a bit more resistant to misbehaving callbacks

03ab088

Added test for delete behavior

54d4d88

Wrote javadocs

8cc2c39

Improved handling of deletes

dd18b9b

[ci] starting with ci

6a41f57

[ci] also run checker-framework

1d4ced3

Fixed broken tests

0ccca4c

[ci] also test under different jdks

800d1b0

Applied all the comments from @sungshik

e4f87a9

Added a bit more comment

538b045

sungshik reviewed Sep 30, 2024

View reviewed changes

DavyLandman added 12 commits October 1, 2024 20:39

Added registration test

39a8b74

Extra test around registration

e1c7186

Renamed enums and processes review comments

096a6f9

Working on the fix for the race Sung found

de22a03

Removed races around registering and unregistering by adding a big ol…

7006548

…d lock

Tweaked the test a bit

80628ab

Improved torture tests

d6b1f28

Fixed bug around fast registration and unregistration in JDKPoller

4f8cf59

Looks like it might be a bug in the JDK registry

Fixed test that would be triggered by events spread over different wo…

85efd25

…rker pools

Remove JDK watch only after a delay, just to avoid hammering the regi…

5bcf1ce

…stry api

Extended readme with example and description

ac9c73e

Slight wording fix

69870e2

sungshik reviewed Oct 15, 2024

View reviewed changes

src/main/java/engineering/swat/watch/impl/BundledSubscription.java Show resolved Hide resolved

DavyLandman and others added 7 commits October 15, 2024 11:29

Apply suggestions from code review

9095927

Co-authored-by: sungshik <[email protected]>

Improve documentation of PATH_ONLY watch scopes

fb01d39

Refine test (deleteOfFileInDirectoryShouldBeVisible)

23b239b

Tweaked documentation a bit

88170a2

Moved JDK classes to its own namespace

87c63e6

Improved impl directory

d392f1c

Make sure to have a dedicated interface, to allow for future addition…

982ce65

…s without breaking binary compatibility

DavyLandman force-pushed the initial-implementation branch from 2b3ee52 to 982ce65 Compare November 11, 2024 10:27

Missed refactor of test

d1b6e9e

DavyLandman merged commit 378f29a into main Nov 11, 2024
10 checks passed

DavyLandman deleted the initial-implementation branch December 10, 2024 13:58

Initial pure JDK implementation #1

Initial pure JDK implementation #1

Uh oh!

Conversation

DavyLandman commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DavyLandman commented Sep 25, 2024

Uh oh!

DavyLandman commented Sep 25, 2024

Uh oh!

sungshik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sungshik left a comment

Choose a reason for hiding this comment

Uh oh!

sungshik Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sungshik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DavyLandman commented Sep 13, 2024 •

edited

Loading