8288627: [AIX] Implement WatchService using system library

[backwaterred]

This PR begins an effort to re-implement the WatchService API on AIX using 'AIX Event Infrastructure' (AHAFS). The initial motivation for doing so was to address errors found by some internal testing in the implementation based on PollingWatchService.java.

I am submitting these changes before they are fully complete because (1) it represents a fairly large change that mostly works well, and could easily be improved upon in the future; (2) to get some early feedback on the changes so the final review process is quicker. My expected plan is to merge these changes after review, add any remaining test failures to a problem list, and return to them soon.

Testing

I am waiting for confirmation that this re-implementation passes the internal tests we saw failing. Initial results look promising. In addition, I see the following failing tests from test/jdk/java/nio/file/WatchService:

• Basic.java fails at testTwoWatchers. Having two WatchService instances registered with the same file, will require some additional work to be supported with AHAFS. This is currently a limitation of this implementation.
• DeleteInterference.java fails for the same reason as above.
• UpdateInterference.java fails for no good reason that I can deduce. Logging the test's progress shows that it doesn't seem to leave the main loop, but that it makes is to within 1ms of doing so.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8288627: [AIX] Implement WatchService using system library

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9281/head:pull/9281
$ git checkout pull/9281

Update a local copy of the PR:
$ git checkout pull/9281
$ git pull https://git.openjdk.org/jdk pull/9281/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9281

View PR using the GUI difftool:
$ git pr show -t 9281

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9281.diff

[bridgekeeper]

👋 Welcome back tsteele! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@backwaterred The following label will be automatically applied to this pull request:

• nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (7106a281)

[AlanBateman]

I don't know AIX or the /aha file system so the following are just a few general comments to help with this feature.

I added AbstractPoller a long time ago as the base class for implementation so my reading of this patch is that it works here too.

If I understand correctly then watching a directory involves create a mirror in /aha. I see the patch uses Files.createDirectories for that. I suspect you really want toRealPath rather than normalise + toAbsolutePath. Calling Files means you are calling the API from the provider which will be problematic if the default provider is configured to be something else. Also I'm quite sure this won't work with a security manager as the code is missing a doPrivileged.

A general point about the NIO area is that we've always tried to keep the native methods as simple as possible, preferably just call one syscall. The loops in the native methods in the native methods in this patch can all be hoisted to the Java code which will make it much easier to maintain.

A general point of code style is to just to keep it as simple as possible. There are some strange formatting in many areas. Also lines of line 170+ characters are a pain for future side-by-side diffs so keeping lines to sane line would be helpful.

[backwaterred]

Thanks for your comments Alan:

I suspect you really want toRealPath rather than normalise + toAbsolutePath.

Thanks for pointing that out. Yes, that method is what I was looking for. I have made this change.

Calling Files means you are calling the API from the provider which will be problematic if the default provider is configured to be something else.

I debated creating and using an instance of AixFileSystem directly. It didn't seem like it would be worth the trouble, but I'm open to changing that opinion. If this code is somehow being run on a non-AIX machine, I'm sure this would not be their only issue. There is another complication if we go this route: createDirectories is implemented in Files.java, so using createDirectory from AixFileSystem would mean duplicating this functionality in my implementation.

Also I'm quite sure this won't work with a security manager as the code is missing a doPrivileged.

I'm sure you're right. I didn't think too hard about the Security Manager since it is now depreciated. I agree that there are a few places that would cause issues. If preferred, I am happy to work in any changes necessary to support it.

The loops ... in the native methods in this patch can all be hoisted to the Java code which will make it much easier to maintain.

This is tricky, since the loops examine values in the c-struct. Without a StructLayout object or some other way of examining values in foreign memory, I'm not sure how to do this in a clean way. Maybe there is a solution for manipulating c-structs in Java that I am not aware of; the way to do this that I am familiar with is currently a preview API (JEP-424).

Also lines of line 170+ characters are a pain for future side-by-side diffs so keeping lines to sane line would be helpful.

I've wrapped the longer lines. I am also happy to change any formatting, just let me know what you'd like changed.

[AlanBateman]

Calling Files means you are calling the API from the provider which will be problematic if the default provider is configured to be something else.

I debated creating and using an instance of AixFileSystem directly. It didn't seem like it would be worth the trouble, but I'm open to changing that opinion. If this code is somehow being run on a non-AIX machine, I'm sure this would not be their only issue. There is another complication if we go this route: createDirectories is implemented in Files.java, so using createDirectory from AixFileSystem would mean duplicating this functionality in my implementation.

It is "architectural broken" to call back into the API from the implementation. It may not be visible now but it will surface once you run in environments where the default file system provider is replaced.

I'm sure you're right. I didn't think too hard about the Security Manager since it is now depreciated. I agree that there are a few places that would cause issues. If preferred, I am happy to work in any changes necessary to support it.

SecurityManager may be terminally deprecated but it's still a supported execution mode so the new code will need to work with a SM.

This is tricky, since the loops examine values in the c-struct. Without a StructLayout object or some other way of examining values in foreign memory, I'm not sure how to do this in a clean way. Maybe there is a solution for manipulating c-structs in Java that I am not aware of; the way to do this that I am familiar with is currently a preview API (JEP-424).

There are many examples in the NIO implementation that access the fields in structs from Java code. In the Linux implementation of WatchService you'll see it access the inotify_event structure directly. In time I expect new code will make more use of the new Panama APIs and maybe some of the existing code will be migrated.

[turbanoff]

let's add space after if

[bplb]

Yes, by convention no blank space before the left ( is for functions / methods.

[backwaterred]

Thanks for the comments. I will be finishing up some work related to jdk19 over the next few weeks, and coming back to this as soon as I have a moment. This is still a priority for me.

[bridgekeeper]

@backwaterred This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[backwaterred]

This pull request has been inactive for more than 4 weeks

I still plan to return to this PR.

[bridgekeeper]

@backwaterred This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@backwaterred This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.