[crac] RFR: Improved open file descriptors tracking [v3]

[Anton Kozlov]

On Wed, 1 Feb 2023 14:30:36 GMT, Radim Vansa <duke at openjdk.org> wrote:

>> Tracks `java.io.FileDescriptor` instances as CRaC resource; before checkpoint these are reported and if not allow-listed (e.g. as opened as standard descriptors) an exception is thrown. Further investigation can use system property `jdk.crac.collect-fd-stacktraces=true` to record origin of those file descriptors.
>> File descriptors claimed in Java code are passed to native; native code checks all open file descriptors and reports error if there's an unexpected FD that is not included in the list passed previously.
>
> Radim Vansa has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Empty commit to trigger GHA

src/hotspot/os/linux/os_linux.cpp line 57:

> 55: #include "runtime/javaCalls.hpp"
> 56: #include "runtime/jniHandles.hpp"
> 57: #include "runtime/jniHandles.inline.hpp"

These ones probably not needed

src/java.base/linux/classes/sun/nio/ch/EPollSelectorImpl.java line 117:

> 115:             // This FileDescriptor is a one-time use, the actual FD will be closed from EventFD
> 116:             fd.markClosedByNIO();
> 117:             IOUtil.configureBlocking(fd, false);

The eventfd was actually closed on line 149, so it's incorrect to mark it is closed here. I think everything was fine before this chunk

src/java.base/share/classes/java/io/FileDescriptor.java line 67:

> 65:      * Called by FileDispatcherImpl when the file descriptor is about to be closed natively.
> 66:      */
> 67:     public void markClosedByNIO() {

Can we avoid changing the public interface? This is not a method we'd like users to call

src/java.base/share/classes/java/io/FileDescriptor.java line 73:

> 71:     class Resource implements jdk.internal.crac.JDKResource {
> 72:         private static final boolean COLLECT_FD_STACKTRACES =
> 73:                 GetBooleanAction.privilegedGetProperty(JDKContext.COLLECT_FD_STACKTRACES_PROPERTY);

Should not be moved to JDKContext or jdk.internal.Core ? To avoid any possible user to get the property value the similar way

src/java.base/share/classes/java/io/FileDescriptor.java line 75:

> 73:                 GetBooleanAction.privilegedGetProperty(JDKContext.COLLECT_FD_STACKTRACES_PROPERTY);
> 74: 
> 75:         private boolean closedByNIO;

I think it may perfectly reside in FileDesriptor, as it looks like a bug for me that FileDispatcherImpl does not change valid() status of the FileDesriptor. Or even make FileDispatcherImpl to reset FileDescriptor.fd? Are there reasons not to do that?

src/java.base/share/classes/java/io/RandomAccessFile.java line 96:

> 94:         public void beforeCheckpoint(Context<? extends jdk.crac.Resource> context) throws Exception {
> 95:             if (Core.getJDKContext().claimFdWeak(fd, this)) {
> 96:                 if (Core.getJDKContext().matchClasspath(path)) {

Actually it would be more natural to match with PersistentJarFile who claimed their FDs already. So we'll be able to handle dynamically loaded JARs that are not a part of the `java.class.path`

src/java.base/share/classes/java/io/RandomAccessFile.java line 100:

> 98:                     return;
> 99:                 }
> 100:                 throw new CheckpointOpenFileException("RandomAccessFile " + path + " left open." + JDKContext.COLLECT_FD_STACKTRACES_HINT,

Stack-traces are good idea! 

Here I would suggest add file desriptor number as well.

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

PR: https://git.openjdk.org/crac/pull/43