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

[Radim Vansa]

On Tue, 31 Jan 2023 12:20:01 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

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

OK, that comes from your commits, I'll clean it up.

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

> 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

I've checked if it would be possible to extend FileDescriptor and put this method there, but there's several non-trivial places where this would have to be changed (e.g. in `sun.nio.ch.InheritedChannel` the FD is constructed using a non-public ctor via reflection). So we can make this package-private and use reflection, or probablz a better way would be through `JavaIOFileDescriptorAccess`.

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

> 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?

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

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

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