RFR: 8293067: (fs) Implement WatchService using system library (macOS)

[Maxim Kartashev]

On Sun, 11 Sep 2022 18:20:57 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> This is an implementation of `WatchService` based on File System Events API that is capable of generating events whenever a change occurs in an interesting directory or underneath it. Since the API naturally supports "recursive" watch, the `FILE_TREE` is supported by the watch service. 
>> 
>> Some things of note:
>> * There's one "service" thread per `WatchService` instance that is inactive unless changes occur in the watched directory. The changes are grouped by introducing a time delay between when they occurred and when they are reported, which is controlled by the sensitivity modifier of the watch service.
>> * Since FSEvents API reports directories only, the watch service keeps a snapshot (hierarchical if necessary) of the files in the directory being watched. The snapshot gets updated when an event in that directory or underneath it gets delivered. File changes are detected by comparing "last modified" time with a millisecond precision (`BasicFileAttributes.lastModifiedTime()`).
>> * There is a slight complication with the move of an entire directory hierarchy: FSEvents API only reports about the containing directory of that move and not about any of the directories actually moved. There's a separate test for that (`Move.java`).
>> * The code is careful not to do any I/O (such as reading the contents of a directory or attributes of a file) unless unavoidable. Any deviation from this line should be considered a bug (of, arguably, low priority).
>> * The native part consists mostly of API wrappers with one exception of the callback function invoked by the system to report the events that occurred. The sole task of the function is to convert from C strings to Java strings and pass the array of affected directories to Java code. This can be rewritten if desired to make the code more future-proof.
>> 
>> This commit leaves `PollingWatchService` unused. I'm not sure if I should/can do anything about it. Any advice is welcomed.
>> 
>> ### Testing
>> 
>> * Tested by running `test/jdk/java/nio/file` and `test/jdk/jdk/nio` on MacOS 10.15.7 (x64) and `test/jdk/java/nio/` plus `test/jdk/jdk/nio` on MacOS 12.5.1 (aarch64).
>> * Also verified that new tests pass on Linux and Windows.
>> * This code (albeit in a slightly modified form) has been in use at JetBrains for around half a year and a few bugs have been found and fixed during that time period.
>
> I did a first pass over this. It's a good start and demonstrates that the  file system events / FSEventStream* API can be used to implement WatchService.
> 
> It's really unfortunate that it requires calling CFRunLoopRun to do the run loop as that means an upcall and JNI code that we would normally try to keep out of this area. One concern is that there is a lot of UnixPath (bytes) <-> String <-> CFString conversions going on and these will need time to work through (even with UTF-8). Also 
> CFRunLoopThread will need to be an InnocuousThread to avoid inheriting inheritable thread locals or context class loader.
> 
> One architectural issue is that the WatchService implementation should not be using Path.of or Files.* methods as that will cause a loop when interposing on the default provider. The provider implementation should only use use the Unix provider methods directly.
> 
> The use of toRealPath().normalize() looks odd, I would expect the normalize to be a no-op here.  This is an area that will require careful study as it's just not clear how the macOS work when the sym link is to another directory on another file system.
> 
> Look at ptr_to_jlong and jlong_to_jptr to go between points and jlong and that should eliminate some of the casts.
> 
> The code includes SUBTREE support. I had hope this wouldn't be included initially as it adds a bit more complexity and could be added later.
> 
> Style-wise the code is very different to the existing code and I think we'll need to do a few cleanup passes (overly long lines, naming, formatting, overdue of final, ... all minor stuff and there is a lot to go through before these things).

@AlanBateman 
Thank you for taking time to review this, much appreciated. I made some changes based on your feedback; see inlined comments below.

> It's really unfortunate that it requires calling CFRunLoopRun to do the run loop as that means an upcall and JNI code that we would normally try to keep out of this area. One concern is that there is a lot of UnixPath (bytes) <-> String <-> CFString conversions going on and these will need time to work through (even with UTF-8). Also CFRunLoopThread will need to be an InnocuousThread to avoid inheriting inheritable thread locals or context class loader.

Done, `CFRunLoopThread` is now `InnocuousThread`.

 
> One architectural issue is that the WatchService implementation should not be using Path.of or Files.* methods as that will cause a loop when interposing on the default provider. The provider implementation should only use use the Unix provider methods directly.

Duly noted and, I think, corrected.


> The use of toRealPath().normalize() looks odd, I would expect the normalize to be a no-op here. This is an area that will require careful study as it's just not clear how the macOS work when the sym link is to another directory on another file system.

I may be misinterpreting you here, but to me it seems `toRealPath()` is actually necessary. Let's say the watch root is a directory named "../symlink" that points to "/Users/maxim/work/tests/dir-watcher/". The FSEvents API will report strings denoting absolute path names like "/Users/maxim/work/tests/dir-watcher/subdir" and in order to be able to remove the common prefix (watch root), one has to make that watch root into an unambiguous absolute path name.

> Look at ptr_to_jlong and jlong_to_jptr to go between points and jlong and that should eliminate some of the casts.

All such cases were eliminated.

> The code includes SUBTREE support. I had hope this wouldn't be included initially as it adds a bit more complexity and could be added later.

The recursive subdirectory watch support was removed (for now); let's focus on the initial support for FSEvents.

> 
> Style-wise the code is very different to the existing code and I think we'll need to do a few cleanup passes (overly long lines, naming, formatting, overdue of final, ... all minor stuff and there is a lot to go through before these things).

 I also reformatted the native code and a bit of Java mostly to shorten the lines. The rest will have to be on case-by-case basis, I'm afraid.


This time around I only ran the `test/jdk/java/nio/file/WatchService` tests on MacOs 10.15.

-------------

PR: https://git.openjdk.org/jdk/pull/10140