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

[Anton Kozlov]

On Tue, 14 Feb 2023 07:20:01 GMT, Radim Vansa <duke at openjdk.org> wrote:

>> 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
>
> The native descriptor is closed, but the FileDescriptor instance is created and forgotten. If the GC doesn't remove it before checkpoint it may be reported as a FileDescriptor leak. I think I saw that happening.

Agree, I've missed the configureBlocking requires a temporary FileDescriptor just to match its interface.

>> 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
>
> Not sure what you're trying to prevent, user code reading a property? The field here is private.

My note is minor, I'm trying to save future users of COLLECT_FD_STACKTRACES_PROPERTY from readingthe property, instead the property can be used, parsed in JDKContext 

class JDKContext {
        static final boolean COLLECT_FD_STACKTRACES =
               GetBooleanAction.privilegedGetProperty(JDKContext.COLLECT_FD_STACKTRACES_PROPERTY);
}

// In this class: just use JDKContext.COLLECT_FD_STACKTRACES without GetProperty

>> 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?
>
> I haven't changed that to minimize the impact of changes; I still assume at one point it made sense to close the native FD without going through FileDescriptor. The reply I got on nio-dev was this:
> 
>> In the java.io APIs, FIS/FOS/RAF define getFD methods so the FileDescriptor can be obtained by applications. This is problematic for a bunch of reasons but for the discussion here, setting it to -1 is needed for the FileDescriptor::valid. Another point is that java.io pre-dates the more robust async close mechanism that is NIO and setting it to -1 helps avoid trying to use a file descriptor that has been closed and/or recycled.
>> In the NIO area, FileDescriptor objects do not leak to applications. The only case where it is necessary to set to -1 is on Windows with sockets and this is because there is no equivalent of dup2 there. Another point here is that FileDescriptors aren't really needed as the channels implementation just need the raw file descriptor or handle. 
> 
> Having that in FileDescriptor seemed more consolidated but if you prefer it that way I won't object. So, should I do that or risk breaking stuff by setting fd to -1?

>From the reply [1] I understand that there was no reason to keep track of the native fd/valid() status in FileDescriptors used by NIO, so that was not done. And for me the status of the FD is closer to FD itself, but I don't have a strong preference.

[1] https://mail.openjdk.org/pipermail/nio-dev/2023-January/013075.html, just for bookkeeping

>> 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`
>
> This case is trying to prevent a situation when the FD is **not** opened using `PersistentJarFile`, as Spring Boot uses own implementation.

Indeed! This is a required transition from the old logic in hotspot, which ignored all files (by path) specified in classpath. 

I was thinking about another use-case, when we have an instance of `PersistentJarFile` (which allows underlying fd), and RandomAccessFile with another underlying fd, but reffering to the same path on file system.

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

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