8080225: FileInput/OutputStream/FileChannel cleanup should be improved

Reviewed-by: mchung, plevart, bpb

src/java.base/windows/classes/java/io/FileDescriptor.java

@@ -25,10 +25,14 @@
 
 package java.io;
 
+import java.lang.ref.Cleaner;
 import java.util.ArrayList;
 import java.util.List;
+
 import jdk.internal.misc.JavaIOFileDescriptorAccess;
 import jdk.internal.misc.SharedSecrets;
+import jdk.internal.ref.CleanerFactory;
+import jdk.internal.ref.PhantomCleanable;
 
 /**
  * Instances of the file descriptor class serve as an opaque handle
@@ -81,12 +85,16 @@ public final class FileDescriptor {
                         return fdo.append;
                     }
 
-                    public void close(FileDescriptor fdo) {
+                    public void close(FileDescriptor fdo) throws IOException {
                         fdo.close();
                     }
 
+                    public void registerCleanup(FileDescriptor fdo) {
+                        fdo.registerCleanup();
+                    }
+
                     public void setHandle(FileDescriptor fdo, long handle) {
-                        fdo.handle = handle;
+                        fdo.setHandle(handle);
                     }
 
                     public long getHandle(FileDescriptor fdo) {
@@ -96,6 +104,11 @@ public final class FileDescriptor {
         );
     }
 
+    /**
+     * Cleanup in case FileDescriptor is not explicitly closed.
+     */
+    private FDCleanup cleanup;
+
     /**
      * Constructs an (invalid) FileDescriptor
      * object.
@@ -149,7 +162,7 @@ public final class FileDescriptor {
      * relevant device(s).  In particular, if this FileDescriptor
      * refers to a physical storage medium, such as a file in a file
      * system, sync will not return until all in-memory modified copies
-     * of buffers associated with this FileDesecriptor have been
+     * of buffers associated with this FileDescriptor have been
      * written to the physical medium.
      *
      * sync is meant to be used by code that requires physical
@@ -175,20 +188,69 @@ public final class FileDescriptor {
     /* This routine initializes JNI field offsets for the class */
     private static native void initIDs();
 
-    private static native long set(int d);
-
     private static FileDescriptor standardStream(int fd) {
         FileDescriptor desc = new FileDescriptor();
-        desc.handle = set(fd);
+        desc.handle = getHandle(fd);
         return desc;
     }
 
+    private static native long getHandle(int d);
+
+    /**
+     * Set the handle.
+     * If setting to -1, clear the cleaner.
+     * The {@link #registerCleanup()} method should be called for new handles.
+     * @param handle the handle or -1 to indicate closed
+     */
+    @SuppressWarnings("unchecked")
+    void setHandle(long handle) {
+        if (handle == -1 && cleanup != null) {
+            cleanup.clear();
+            cleanup = null;
+        }
+        this.handle = handle;
+    }
+
+    /**
+     * Register a cleanup for the current handle.
+     * Used directly in java.io and indirectly via fdAccess.
+     * The cleanup should be registered after the handle is set in the FileDescriptor.
+     */
+    @SuppressWarnings("unchecked")
+    synchronized void registerCleanup() {
+        if (cleanup != null) {
+            cleanup.clear();
+        }
+        cleanup = FDCleanup.create(this);
+    }
+
     /**
      * Close the raw file descriptor or handle, if it has not already been closed
      * and set the fd and handle to -1.
+     * Clear the cleaner so the close does not happen twice.
      * Package private to allow it to be used in java.io.
+     * @throws IOException if close fails
      */
-    native void close();
+    @SuppressWarnings("unchecked")
+    synchronized void close() throws IOException {
+        if (cleanup != null) {
+            cleanup.clear();
+            cleanup = null;
+        }
+        close0();
+    }
+
+    /*
+     * Close the raw file descriptor or handle, if it has not already been closed
+     * and set the fd and handle to -1.
+     */
+    private native void close0() throws IOException;
+
+    /*
+    * Raw close of the file handle.
+    * Used only for last chance cleanup.
+    */
+    private static native void cleanupClose0(long handle) throws IOException;
 
     /*
      * Package private methods to track referents.
@@ -253,4 +315,45 @@ public final class FileDescriptor {
             }
         }
     }
+
+    /**
+     * Cleanup for a FileDescriptor when it becomes phantom reachable.
+     * Create a cleanup if handle != -1.
+     * Subclassed from {@code PhantomCleanable} so that {@code clear} can be
+     * called to disable the cleanup when the fd is closed by any means other
+     * than calling {@link FileDescriptor#close}.
+     * Otherwise, it may close the handle after it has been reused.
+     */
+    static final class FDCleanup extends PhantomCleanable<Object> {
+        private final long handle;
+
+        static FDCleanup create(FileDescriptor fdo) {
+            return fdo.handle == -1
+                    ? null
+                    : new FDCleanup(fdo, CleanerFactory.cleaner(), fdo.handle);
+        }
+
+        /**
+         * Constructor for a phantom cleanable reference.
+         * @param obj the object to monitor
+         * @param cleaner the cleaner
+         * @param handle file handle to close
+         */
+        private FDCleanup(Object obj, Cleaner cleaner, long handle) {
+            super(obj, cleaner);
+            this.handle = handle;
+        }
+
+        /**
+         * Close the native handle.
+         */
+        @Override
+        protected void performCleanup() {
+            try {
+                cleanupClose0(handle);
+            } catch (IOException ioe) {
+                throw new UncheckedIOException("close", ioe);
+            }
+        }
+    }
 }

src/java.base/windows/classes/sun/nio/ch/FileDispatcherImpl.java

@@ -114,6 +114,7 @@ class FileDispatcherImpl extends FileDispatcher {
         FileDescriptor result = new FileDescriptor();
         long handle = duplicateHandle(fdAccess.getHandle(fd));
         fdAccess.setHandle(result, handle);
+        fdAccess.registerCleanup(result);
         return result;
     }
 

src/java.base/windows/classes/sun/nio/ch/WindowsAsynchronousFileChannelImpl.java

@@ -139,7 +139,7 @@ public class WindowsAsynchronousFileChannelImpl
         invalidateAllLocks();
 
         // close the file
-        close0(handle);
+        nd.close(fdObj);
 
         // waits until all I/O operations have completed
         ioCache.close();
@@ -728,8 +728,6 @@ public class WindowsAsynchronousFileChannelImpl
     private static native int lockFile(long handle, long position, long size,
         boolean shared, long overlapped) throws IOException;
 
-    private static native void close0(long handle);
-
     static {
         IOUtil.load();
     }

src/java.base/windows/classes/sun/nio/fs/WindowsChannelFactory.java

@@ -216,8 +216,7 @@ class WindowsChannelFactory {
         } catch (IOException x) {
             // IOException is thrown if the file handle cannot be associated
             // with the completion port. All we can do is close the file.
-            long handle = fdAccess.getHandle(fdObj);
-            CloseHandle(handle);
+            fdAccess.close(fdObj);
             throw x;
         }
     }
@@ -347,6 +346,7 @@ class WindowsChannelFactory {
         FileDescriptor fdObj = new FileDescriptor();
         fdAccess.setHandle(fdObj, handle);
         fdAccess.setAppend(fdObj, flags.append);
+        fdAccess.registerCleanup(fdObj);
         return fdObj;
     }
 }

src/java.base/windows/native/libjava/FileDescriptor_md.c

@@ -57,7 +57,7 @@ Java_java_io_FileDescriptor_initIDs(JNIEnv *env, jclass fdClass) {
 }
 
 JNIEXPORT jlong JNICALL
-Java_java_io_FileDescriptor_set(JNIEnv *env, jclass fdClass, jint fd) {
+Java_java_io_FileDescriptor_getHandle(JNIEnv *env, jclass fdClass, jint fd) {
     SET_HANDLE(fd);
 }
 
@@ -73,8 +73,17 @@ Java_java_io_FileDescriptor_sync(JNIEnv *env, jobject this) {
     }
 }
 
+JNIEXPORT void JNICALL
+Java_java_io_FileDescriptor_cleanupClose0(JNIEnv *env, jclass fdClass, jlong handle) {
+    if (handle != -1) {
+        if (CloseHandle((HANDLE)handle) == -1) {
+            JNU_ThrowIOExceptionWithLastError(env, "close failed");
+        }
+    }
+}
+
 // instance method close0 for FileDescriptor
 JNIEXPORT void JNICALL
-Java_java_io_FileDescriptor_close(JNIEnv *env, jobject this) {
+Java_java_io_FileDescriptor_close0(JNIEnv *env, jobject this) {
     fileDescriptorClose(env, this);
 }

src/java.base/windows/native/libnio/ch/WindowsAsynchronousFileChannelImpl.c

@@ -121,13 +121,3 @@ Java_sun_nio_ch_WindowsAsynchronousFileChannelImpl_lockFile(JNIEnv *env, jobject
     return 0;
 }
 
-JNIEXPORT void JNICALL
-Java_sun_nio_ch_WindowsAsynchronousFileChannelImpl_close0(JNIEnv* env, jclass this,
-    jlong handle)
-{
-    HANDLE h = (HANDLE)jlong_to_ptr(handle);
-    BOOL result = CloseHandle(h);
-    if (result == 0) {
-        JNU_ThrowIOExceptionWithLastError(env, "Close failed");
-    }
-}