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

DavyLandman

Some small comments

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

+ 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.

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

+ 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).

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

+ }
+
+ 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.

…ould be used

…watch service is closed

…and improve comments

sungshik

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

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

+
+ @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.)

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

+ }
+
+ 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

+ 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

+ 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-mainApr 30, 2025

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecovBot 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sungshik commented Apr 21, 2025•
edited
Loading

codecovBot commented Apr 21, 2025•
edited
Loading