Improved macOS support: `java.nio.file` Watch Service by sungshik · Pull Request #41 · SWAT-engineering/java-watch

sungshik · 2025-04-21T08:35:01Z

This PR adds an implementation of java.nio.file's Watch Service API that uses NativeEventStreams (added in #40) to access the native APIs on macOS. It's an alternative to JDK's FileSystems.getDefault().newWatchService().

(The additional comments below contain copy/pastes of the JDK docs for convenience.)

…at fully implements the `java.nio.file.WatchService` interface

codecov · 2025-04-21T08:40:59Z

Codecov Report

Attention: Patch coverage is 70.00000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 80.6%. Comparing base (73b0238) to head (5b04d16).
Report is 19 commits behind head on improved-macos-support-main.

Files with missing lines	Patch %	Lines
...a/engineering/swat/watch/impl/mac/MacWatchKey.java	80.6%	4 Missing and 8 partials ⚠️
.../engineering/swat/watch/impl/mac/MacWatchable.java	56.5%	8 Missing and 2 partials ⚠️
...ava/engineering/swat/watch/impl/jdk/JDKPoller.java	63.6%	6 Missing and 2 partials ⚠️
...gineering/swat/watch/impl/mac/MacWatchService.java	53.8%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##             improved-macos-support-main     #41      +/-   ##
================================================================
+ Coverage                           65.3%   80.6%   +15.2%     
- Complexity                           136     163      +27     
================================================================
  Files                                 20      23       +3     
  Lines                                705     822     +117     
  Branches                              80      95      +15     
================================================================
+ Hits                                 461     663     +202     
+ Misses                               205      97     -108     
- Partials                              39      62      +23

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…/nio-file-watch-service

…t/nio-file-watch-service

sungshik

Comments

src/main/java/engineering/swat/watch/impl/mac/MacBlockingWatchService.java

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

src/main/java/engineering/swat/watch/impl/mac/MacWatchService.java

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

DavyLandman

Some small comments

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

src/main/java/engineering/swat/watch/impl/mac/MacBlockingWatchService.java

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

DavyLandman · 2025-04-25T06:58:10Z

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

+    public MacWatchKey(MacWatchable watchable, MacWatchService service) throws IOException {
+        this.watchable = watchable;
+        this.service = service;
+        this.pendingEvents = new LinkedBlockingQueue<>();


Maybe we should batch add a whole range of events? Such that the queue contains lists of events? To avoid filling up the queue a lot and then cleaning it out again?

For the sake of finishing this feature in a "good enough" state asap, for now, I'll add this as a possible performance optimization to the list of tasks in the feature PR (#39). Let's decide later if we want/need to do this as part of that PR, or move it to a separate issue.

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

DavyLandman · 2025-04-25T07:05:00Z

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

+    private final NativeEventStream stream;
+
+    private volatile Configuration config = new Configuration();
+    private volatile boolean signalled = false; // `!signalled` means "ready"


Why do we need the signal variable? Is it not just : whenever the queue is not empty?

As I see all kinds of subtle races between clearing it and setting it in this class.

According to the WatchKey spec, a watch key remains signalled until it's reset, even if the queue becomes (temporarily) nonempty, so a separate field is needed to do this bookkeeping.

The interaction between signalled and pendingEvents is quite tricky, indeed. To make it easier to understand what happens, I've isolated those fields and all code that accesses them in a separate helper inner class (including comments to argue that no harmful races arise).

src/main/java/engineering/swat/watch/impl/mac/MacWatchService.java

DavyLandman · 2025-04-25T07:08:53Z

src/main/java/engineering/swat/watch/impl/mac/MacWatchable.java

+    }
+
+    public void unregister(MacWatchService watcher) {
+        registrations.remove(watcher);


Shouldn't this also cause some kind of call to the wachter?

This method is used only as part of WatchKey.cancel(), which is called by the poller when the watcher for the path of that watch key is closed. I.e., the watcher triggers the unregistration, so it already knows about it. I'll make it package-private, though, because it's just an internal bookkeeping method that shouldn't be used directly by third parties.

src/main/java/engineering/swat/watch/impl/mac/MacWatchable.java

…ould be used

…watch service is closed

…and improve comments

sungshik

Thanks! Addressed the comments and added a few clarifying responses.

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

sungshik · 2025-04-30T09:17:19Z

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

+
+    @Override
+    public List<WatchEvent<?>> pollEvents() {
+        var list = new ArrayList<WatchEvent<?>>(pendingEvents.size());


Nope. (Well, to be fair, yes, computing the length of a linked list is indeed O(n) 😎, but LinkedBlockingQueue has an extra field to track it.)

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

sungshik · 2025-04-30T09:41:10Z

src/main/java/engineering/swat/watch/impl/mac/MacWatchable.java

+    }
+
+    public void unregister(MacWatchService watcher) {
+        registrations.remove(watcher);


This method is used only as part of WatchKey.cancel(), which is called by the poller when the watcher for the path of that watch key is closed. I.e., the watcher triggers the unregistration, so it already knows about it. I'll make it package-private, though, because it's just an internal bookkeeping method that shouldn't be used directly by third parties.

sungshik · 2025-04-30T09:44:56Z

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

+    public MacWatchKey(MacWatchable watchable, MacWatchService service) throws IOException {
+        this.watchable = watchable;
+        this.service = service;
+        this.pendingEvents = new LinkedBlockingQueue<>();


For the sake of finishing this feature in a "good enough" state asap, for now, I'll add this as a possible performance optimization to the list of tasks in the feature PR (#39). Let's decide later if we want/need to do this as part of that PR, or move it to a separate issue.

sungshik · 2025-04-30T11:58:35Z

src/main/java/engineering/swat/watch/impl/mac/MacWatchKey.java

+    private final NativeEventStream stream;
+
+    private volatile Configuration config = new Configuration();
+    private volatile boolean signalled = false; // `!signalled` means "ready"


According to the WatchKey spec, a watch key remains signalled until it's reset, even if the queue becomes (temporarily) nonempty, so a separate field is needed to do this bookkeeping.

The interaction between signalled and pendingEvents is quite tricky, indeed. To make it easier to understand what happens, I've isolated those fields and all code that accesses them in a separate helper inner class (including comments to argue that no harmful races arise).

sungshik added 3 commits April 21, 2025 10:11

Add implementation of the java.nio.file's Watch Service API

3fe8d47

Add an experimental/untested/unused extension of MacWatchService th…

2495559

…at fully implements the `java.nio.file.WatchService` interface

Add MacWatchService and MacWatchable to the poller

587c01a

sungshik mentioned this pull request Apr 21, 2025

Improved macOS support #39

Merged

3 tasks

sungshik added 7 commits April 22, 2025 16:09

Merge branch 'improved-macos-support/jna' into improved-macos-support…

8b71de6

…/nio-file-watch-service

Merge branch 'improved-macos-support-main' into improved-macos-suppor…

8161950

…t/nio-file-watch-service

Disable move/rename test (temporarily)

57ba87e

Make stream in MacWatchKey final

8d85f1f

Fix NPE

3c14f6a

Simplify code in a few places (nits)

feb2f9b

Use computeIfAbsent instead of putIfAbsent

76eb623

sungshik commented Apr 23, 2025

View reviewed changes

sungshik marked this pull request as ready for review April 23, 2025 08:38

sungshik requested a review from DavyLandman April 23, 2025 08:38

DavyLandman approved these changes Apr 25, 2025

View reviewed changes

sungshik added 8 commits April 30, 2025 09:23

Add system property to configure which WatchService implementation sh…

442b98c

…ould be used

Remove MacBlockingWatchService

4087179

Reduce visibility of MacWatchKey to package-private

1cccd6e

Reduce visibility of MacWatchable to package-private

1c5742e

Reduce visibility of internal methods to package-private

10692a7

Clarify logic for when to ignore events

cb07075

Add comment to clarify why we don't need to interrupt threads when a …

ee77302

…watch service is closed

Add helper inner class to group thread-safety-critical code together …

5b04d16

…and improve comments

sungshik commented Apr 30, 2025

View reviewed changes

sungshik merged commit e21382d into improved-macos-support-main Apr 30, 2025
33 of 35 checks passed

sungshik deleted the improved-macos-support/nio-file-watch-service branch April 30, 2025 17:20

sungshik mentioned this pull request May 12, 2025

Enqueue collections of events instead of individual events in MacWatchKey? #48

Open

Conversation

sungshik commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

DavyLandman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sungshik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sungshik commented Apr 21, 2025 •

edited

Loading

codecov bot commented Apr 21, 2025 •

edited

Loading