7105952: Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile

Reviewed-by: alanb

src/share/classes/java/io/FileInputStream.java

@@ -124,7 +124,7 @@ class FileInputStream extends InputStream
             throw new NullPointerException();
         }
         fd = new FileDescriptor();
-        fd.incrementAndGetUseCount();
+        fd.attach(this);
         open(name);
     }
 
@@ -164,10 +164,9 @@ class FileInputStream extends InputStream
 
         /*
          * FileDescriptor is being shared by streams.
-         * Ensure that it's GC'ed only when all the streams/channels are done
-         * using it.
+         * Register this stream with FileDescriptor tracker.
          */
-        fd.incrementAndGetUseCount();
+        fd.attach(this);
     }
 
     /**
@@ -294,27 +293,14 @@ class FileInputStream extends InputStream
             closed = true;
         }
         if (channel != null) {
-            /*
-             * Decrement the FD use count associated with the channel
-             * The use count is incremented whenever a new channel
-             * is obtained from this stream.
-             */
-           fd.decrementAndGetUseCount();
            channel.close();
         }
 
-        /*
-         * Decrement the FD use count associated with this stream
-         */
-        int useCount = fd.decrementAndGetUseCount();
-
-        /*
-         * If FileDescriptor is still in use by another stream, we
-         * will not close it.
-         */
-        if (useCount <= 0) {
-            close0();
-        }
+        fd.closeAll(new Closeable() {
+            public void close() throws IOException {
+               close0();
+           }
+        });
     }
 
     /**
@@ -328,7 +314,9 @@ class FileInputStream extends InputStream
      * @see        java.io.FileDescriptor
      */
     public final FileDescriptor getFD() throws IOException {
-        if (fd != null) return fd;
+        if (fd != null) {
+            return fd;
+        }
         throw new IOException();
     }
 
@@ -352,13 +340,6 @@ class FileInputStream extends InputStream
         synchronized (this) {
             if (channel == null) {
                 channel = FileChannelImpl.open(fd, true, false, this);
-
-                /*
-                 * Increment fd's use count. Invoking the channel's close()
-                 * method will result in decrementing the use count set for
-                 * the channel.
-                 */
-                fd.incrementAndGetUseCount();
             }
             return channel;
         }
@@ -381,7 +362,12 @@ class FileInputStream extends InputStream
      */
     protected void finalize() throws IOException {
         if ((fd != null) &&  (fd != FileDescriptor.in)) {
-                close();
+            /* if fd is shared, the references in FileDescriptor
+             * will ensure that finalizer is only called when
+             * safe to do so. All references using the fd have
+             * become unreachable. We can call close()
+             */
+            close();
         }
     }
 }

src/share/classes/java/io/FileOutputStream.java

@@ -197,9 +197,9 @@ class FileOutputStream extends OutputStream
             throw new NullPointerException();
         }
         this.fd = new FileDescriptor();
+        fd.attach(this);
         this.append = append;
 
-        fd.incrementAndGetUseCount();
         open(name, append);
     }
 
@@ -237,12 +237,7 @@ class FileOutputStream extends OutputStream
         this.fd = fdObj;
         this.append = false;
 
-        /*
-         * FileDescriptor is being shared by streams.
-         * Ensure that it's GC'ed only when all the streams/channels are done
-         * using it.
-         */
-        fd.incrementAndGetUseCount();
+        fd.attach(this);
     }
 
     /**
@@ -331,27 +326,14 @@ class FileOutputStream extends OutputStream
         }
 
         if (channel != null) {
-            /*
-             * Decrement FD use count associated with the channel
-             * The use count is incremented whenever a new channel
-             * is obtained from this stream.
-             */
-            fd.decrementAndGetUseCount();
             channel.close();
         }
 
-        /*
-         * Decrement FD use count associated with this stream
-         */
-        int useCount = fd.decrementAndGetUseCount();
-
-        /*
-         * If FileDescriptor is still in use by another stream, we
-         * will not close it.
-         */
-        if (useCount <= 0) {
-            close0();
-        }
+        fd.closeAll(new Closeable() {
+            public void close() throws IOException {
+               close0();
+           }
+        });
     }
 
     /**
@@ -365,7 +347,9 @@ class FileOutputStream extends OutputStream
      * @see        java.io.FileDescriptor
      */
      public final FileDescriptor getFD()  throws IOException {
-        if (fd != null) return fd;
+        if (fd != null) {
+            return fd;
+        }
         throw new IOException();
      }
 
@@ -390,13 +374,6 @@ class FileOutputStream extends OutputStream
         synchronized (this) {
             if (channel == null) {
                 channel = FileChannelImpl.open(fd, false, true, append, this);
-
-                /*
-                 * Increment fd's use count. Invoking the channel's close()
-                 * method will result in decrementing the use count set for
-                 * the channel.
-                 */
-                fd.incrementAndGetUseCount();
             }
             return channel;
         }
@@ -415,7 +392,12 @@ class FileOutputStream extends OutputStream
             if (fd == FileDescriptor.out || fd == FileDescriptor.err) {
                 flush();
             } else {
-                    close();
+                /* if fd is shared, the references in FileDescriptor
+                 * will ensure that finalizer is only called when
+                 * safe to do so. All references using the fd have
+                 * become unreachable. We can call close()
+                 */
+                close();
             }
         }
     }

src/share/classes/java/io/RandomAccessFile.java

@@ -229,7 +229,7 @@ public class RandomAccessFile implements DataOutput, DataInput, Closeable {
             throw new NullPointerException();
         }
         fd = new FileDescriptor();
-        fd.incrementAndGetUseCount();
+        fd.attach(this);
         open(name, imode);
     }
 
@@ -242,7 +242,9 @@ public class RandomAccessFile implements DataOutput, DataInput, Closeable {
      * @see        java.io.FileDescriptor
      */
     public final FileDescriptor getFD() throws IOException {
-        if (fd != null) return fd;
+        if (fd != null) {
+            return fd;
+        }
         throw new IOException();
     }
 
@@ -268,17 +270,6 @@ public class RandomAccessFile implements DataOutput, DataInput, Closeable {
         synchronized (this) {
             if (channel == null) {
                 channel = FileChannelImpl.open(fd, true, rw, this);
-
-                /*
-                 * FileDescriptor could be shared by FileInputStream or
-                 * FileOutputStream.
-                 * Ensure that FD is GC'ed only when all the streams/channels
-                 * are done using it.
-                 * Increment fd's use count. Invoking the channel's close()
-                 * method will result in decrementing the use count set for
-                 * the channel.
-                 */
-                fd.incrementAndGetUseCount();
             }
             return channel;
         }
@@ -577,28 +568,14 @@ public class RandomAccessFile implements DataOutput, DataInput, Closeable {
             closed = true;
         }
         if (channel != null) {
-            /*
-             * Decrement FD use count associated with the channel. The FD use
-             * count is incremented whenever a new channel is obtained from
-             * this stream.
-             */
-            fd.decrementAndGetUseCount();
             channel.close();
         }
 
-        /*
-         * Decrement FD use count associated with this stream.
-         * The count got incremented by FileDescriptor during its construction.
-         */
-        int useCount = fd.decrementAndGetUseCount();
-
-        /*
-         * If FileDescriptor is still in use by another stream, we
-         * will not close it.
-         */
-        if (useCount <= 0) {
-            close0();
-        }
+        fd.closeAll(new Closeable() {
+            public void close() throws IOException {
+               close0();
+           }
+        });
     }
 
     //

src/solaris/classes/java/io/FileDescriptor.java

 /*
- * Copyright (c) 1995, 2008, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2011, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,7 +25,8 @@
 
 package java.io;
 
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * Instances of the file descriptor class serve as an opaque handle
@@ -46,12 +47,9 @@ public final class FileDescriptor {
 
     private int fd;
 
-    /**
-     * A counter for tracking the FIS/FOS/RAF instances that
-     * use this FileDescriptor. The FIS/FOS.finalize() will not release
-     * the FileDescriptor if it is still under user by a stream.
-     */
-    private AtomicInteger useCount;
+    private Closeable parent;
+    private List<Closeable> otherParents;
+    private boolean closed;
 
     /**
      * Constructs an (invalid) FileDescriptor
@@ -59,12 +57,10 @@ public final class FileDescriptor {
      */
     public /**/ FileDescriptor() {
         fd = -1;
-        useCount = new AtomicInteger();
     }
 
     private /* */ FileDescriptor(int fd) {
         this.fd = fd;
-        useCount = new AtomicInteger();
     }
 
     /**
@@ -164,13 +160,67 @@ public final class FileDescriptor {
         );
     }
 
-    // package private methods used by FIS, FOS and RAF
+    /*
+     * Package private methods to track referents.
+     * If multiple streams point to the same FileDescriptor, we cycle
+     * through the list of all referents and call close()
+     */
 
-    int incrementAndGetUseCount() {
-        return useCount.incrementAndGet();
+    /**
+     * Attach a Closeable to this FD for tracking.
+     * parent reference is added to otherParents when
+     * needed to make closeAll simpler.
+     */
+    synchronized void attach(Closeable c) {
+        if (parent == null) {
+            // first caller gets to do this
+            parent = c;
+        } else if (otherParents == null) {
+            otherParents = new ArrayList<>();
+            otherParents.add(parent);
+            otherParents.add(c);
+        } else {
+            otherParents.add(c);
+        }
     }
 
-    int decrementAndGetUseCount() {
-        return useCount.decrementAndGet();
+    /**
+     * Cycle through all Closeables sharing this FD and call
+     * close() on each one.
+     *
+     * The caller closeable gets to call close0().
+     */
+    @SuppressWarnings("try")
+    synchronized void closeAll(Closeable releaser) throws IOException {
+        if (!closed) {
+            closed = true;
+            IOException ioe = null;
+            try (Closeable c = releaser) {
+                if (otherParents != null) {
+                    for (Closeable referent : otherParents) {
+                        try {
+                            referent.close();
+                        } catch(IOException x) {
+                            if (ioe == null) {
+                                ioe = x;
+                            } else {
+                                ioe.addSuppressed(x);
+                            }
+                        }
+                    }
+                }
+            } catch(IOException ex) {
+                /*
+                 * If releaser close() throws IOException
+                 * add other exceptions as suppressed.
+                 */
+                if (ioe != null)
+                    ex.addSuppressed(ioe);
+                ioe = ex;
+            } finally {
+                if (ioe != null)
+                    throw ioe;
+            }
+        }
     }
 }

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

 /*
- * Copyright (c) 2003, 2008, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2011, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,7 +25,8 @@
 
 package java.io;
 
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * Instances of the file descriptor class serve as an opaque handle
@@ -45,13 +46,9 @@ public final class FileDescriptor {
 
     private long handle;
 
-    /**
-     * A use counter for tracking the FIS/FOS/RAF instances that
-     * use this FileDescriptor. The FIS/FOS.finalize() will not release
-     * the FileDescriptor if it is still under use by any stream.
-     */
-    private AtomicInteger useCount;
-
+    private Closeable parent;
+    private List<Closeable> otherParents;
+    private boolean closed;
 
     /**
      * Constructs an (invalid) FileDescriptor
@@ -60,7 +57,6 @@ public final class FileDescriptor {
     public /**/ FileDescriptor() {
         fd = -1;
         handle = -1;
-        useCount = new AtomicInteger();
     }
 
     static {
@@ -168,13 +164,67 @@ public final class FileDescriptor {
         return desc;
     }
 
-    // package private methods used by FIS, FOS and RAF.
+    /*
+     * Package private methods to track referents.
+     * If multiple streams point to the same FileDescriptor, we cycle
+     * through the list of all referents and call close()
+     */
 
-    int incrementAndGetUseCount() {
-        return useCount.incrementAndGet();
+    /**
+     * Attach a Closeable to this FD for tracking.
+     * parent reference is added to otherParents when
+     * needed to make closeAll simpler.
+     */
+    synchronized void attach(Closeable c) {
+        if (parent == null) {
+            // first caller gets to do this
+            parent = c;
+        } else if (otherParents == null) {
+            otherParents = new ArrayList<>();
+            otherParents.add(parent);
+            otherParents.add(c);
+        } else {
+            otherParents.add(c);
+        }
     }
 
-    int decrementAndGetUseCount() {
-        return useCount.decrementAndGet();
+    /**
+     * Cycle through all Closeables sharing this FD and call
+     * close() on each one.
+     *
+     * The caller closeable gets to call close0().
+     */
+    @SuppressWarnings("try")
+    synchronized void closeAll(Closeable releaser) throws IOException {
+        if (!closed) {
+            closed = true;
+            IOException ioe = null;
+            try (Closeable c = releaser) {
+                if (otherParents != null) {
+                    for (Closeable referent : otherParents) {
+                        try {
+                            referent.close();
+                        } catch(IOException x) {
+                            if (ioe == null) {
+                                ioe = x;
+                            } else {
+                                ioe.addSuppressed(x);
+                            }
+                        }
+                    }
+                }
+            } catch(IOException ex) {
+                /*
+                 * If releaser close() throws IOException
+                 * add other exceptions as suppressed.
+                 */
+                if (ioe != null)
+                    ex.addSuppressed(ioe);
+                ioe = ex;
+            } finally {
+                if (ioe != null)
+                    throw ioe;
+            }
+        }
     }
 }

test/java/io/FileDescriptor/FileChannelFDTest.java

-/*
- * Copyright (c) 2006, Oracle and/or its affiliates. All rights reserved.
- * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
- *
- * This code is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License version 2 only, as
- * published by the Free Software Foundation.
- *
- * This code is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
- * version 2 for more details (a copy is included in the LICENSE file that
- * accompanied this code).
- *
- * You should have received a copy of the GNU General Public License version
- * 2 along with this work; if not, write to the Free Software Foundation,
- * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
- * or visit www.oracle.com if you need additional information or have any
- * questions.
- */
-
-/**
- *
- * @test
- * @bug 6322678
- * @summary Test for making sure that fd is closed during
- *          finalization of a stream, when an associated
- *          file channel is not available
- */
-
-import java.io.*;
-import java.nio.*;
-import java.nio.channels.*;
-
-public class FileChannelFDTest {
-
-    static byte data[] = new byte[] {48, 49, 50, 51, 52, 53, 54, 55, 56, 57,};
-    static String inFileName = "fd-in-test.txt";
-    static String outFileName = "fd-out-test.txt";
-    static File inFile;
-    static File outFile;
-
-    private static void writeToInFile() throws IOException {
-        FileOutputStream out = new FileOutputStream(inFile);
-        out.write(data);
-        out.close();
-    }
-
-    public static void main(String[] args)
-                throws Exception {
-
-        inFile= new File(System.getProperty("test.dir", "."),
-                        inFileName);
-        inFile.createNewFile();
-        inFile.deleteOnExit();
-        writeToInFile();
-
-        outFile  = new File(System.getProperty("test.dir", "."),
-                        outFileName);
-        outFile.createNewFile();
-        outFile.deleteOnExit();
-
-        doFileChannel();
-    }
-
-     private static void doFileChannel() throws Exception {
-
-        FileInputStream fis = new FileInputStream(inFile);
-        FileDescriptor fd = fis.getFD();
-        FileChannel fc = fis.getChannel();
-        System.out.println("Created fis:" + fis);
-
-        /**
-         * Encourage the GC
-         */
-        fis = null;
-        fc = null;
-        System.gc();
-        Thread.sleep(500);
-
-        if (fd.valid()) {
-            throw new Exception("Finalizer either didn't run --" +
-                "try increasing the Thread's sleep time after System.gc();" +
-                "or the finalizer didn't close the file");
-        }
-
-        System.out.println("File Closed successfully");
-        System.out.println();
-  }
-}

test/java/io/etc/FileDescriptorSharing.java → test/java/io/FileDescriptor/Sharing.java

@@ -23,10 +23,9 @@
 
 /*
  * @test
- * @bug 6322678 7082769
- * @summary FileInputStream/FileOutputStream/RandomAccessFile allow file descriptor
- *          to be closed while still in use.
- * @run main/othervm FileDescriptorSharing
+ * @bug 7105952 6322678 7082769
+ * @summary Improve finalisation for FileInputStream/FileOutputStream/RandomAccessFile
+ * @run main/othervm Sharing
  */
 
 import java.io.*;
@@ -34,7 +33,7 @@ import java.nio.channels.FileChannel;
 import java.nio.channels.FileLock;
 import java.util.concurrent.CountDownLatch;
 
-public class FileDescriptorSharing {
+public class Sharing {
 
     final static int numFiles = 10;
     volatile static boolean fail;
@@ -44,11 +43,12 @@ public class FileDescriptorSharing {
         TestMultipleFD();
         TestIsValid();
         MultiThreadedFD();
+        TestCloseAll();
     }
 
     /**
-     * We shouldn't discard a file descriptor until all streams have
-     * finished with it
+     * Finalizer shouldn't discard a file descriptor until all streams have
+     * finished with it.
      */
     private static void TestFinalizer() throws Exception {
         FileDescriptor fd = null;
@@ -96,9 +96,6 @@ public class FileDescriptorSharing {
                 System.out.print(".");
                 ret = fis3.read();
             }
-            if(!fd.valid()) {
-                throw new RuntimeException("TestFinalizer() : FileDescriptor should be valid");
-            }
         } finally {
             testFinalizerFile.delete();
         }
@@ -194,23 +191,19 @@ public class FileDescriptorSharing {
         } finally {
             try {
                 if (fis != null) fis.close();
-                if (fos != null) fos.close();
-                if (!fd.valid()) {
-                    throw new RuntimeException("FileDescriptor should be valid");
-                }
-                if (raf != null) raf.close();
                 if (fd.valid()) {
-                    throw new RuntimeException("close() called and FileDescriptor still valid");
+                    throw new RuntimeException("[FIS close()] FileDescriptor shouldn't be valid");
                 }
-            } finally {
+                if (fos != null) fos.close();
                 if (raf != null) raf.close();
+            } finally {
                 test1.delete();
             }
         }
 
         /*
-         * Close out in different order to ensure FD is not
-         * closed out too early
+         * Close out in different order to ensure FD is
+         * closed correctly.
          */
         File test2 = new File("test2");
         try {
@@ -221,14 +214,11 @@ public class FileDescriptorSharing {
         } finally {
             try {
                 if (raf != null) raf.close();
-                if (fos != null) fos.close();
-                if (!fd.valid()) {
-                    throw new RuntimeException("FileDescriptor should be valid");
-                }
-                if (fis != null) fis.close();
                 if (fd.valid()) {
-                    throw new RuntimeException("close() called and FileDescriptor still valid");
+                    throw new RuntimeException("[RAF close()] FileDescriptor shouldn't be valid");
                 }
+                if (fos != null) fos.close();
+                if (fis != null) fis.close();
             } finally {
                 test2.delete();
             }
@@ -244,14 +234,11 @@ public class FileDescriptorSharing {
         } finally {
             try {
                 if (fos != null) fos.close();
-                if (raf != null) raf.close();
-                if (!fd.valid()) {
-                    throw new RuntimeException("FileDescriptor should be valid");
-                }
-                if (fis != null) fis.close();
                 if (fd.valid()) {
-                    throw new RuntimeException("close() called and FileDescriptor still valid");
+                    throw new RuntimeException("[FOS close()] FileDescriptor shouldn't be valid");
                 }
+                if (raf != null) raf.close();
+                if (fis != null) fis.close();
             } finally {
                 test3.delete();
             }
@@ -259,7 +246,7 @@ public class FileDescriptorSharing {
     }
 
     /**
-     * Test concurrent access to the same fd.useCount field
+     * Test concurrent access to the same FileDescriptor
      */
     private static void MultiThreadedFD() throws Exception {
         RandomAccessFile raf = null;
@@ -293,6 +280,68 @@ public class FileDescriptorSharing {
         }
     }
 
+    /**
+     * Test closeAll handling in FileDescriptor
+     */
+    private static void TestCloseAll() throws Exception {
+        File testFile = new File("test");
+        testFile.deleteOnExit();
+        RandomAccessFile raf = new RandomAccessFile(testFile, "rw");
+        FileInputStream fis = new FileInputStream(raf.getFD());
+        fis.close();
+        if (raf.getFD().valid()) {
+             throw new RuntimeException("FD should not be valid.");
+        }
+
+        // Test the suppressed exception handling - FileInputStream
+
+        raf = new RandomAccessFile(testFile, "rw");
+        fis = new FileInputStream(raf.getFD());
+        BadFileInputStream bfis1 = new BadFileInputStream(raf.getFD());
+        BadFileInputStream bfis2 = new BadFileInputStream(raf.getFD());
+        BadFileInputStream bfis3 = new BadFileInputStream(raf.getFD());
+        // extra test - set bfis3 to null
+        bfis3 = null;
+        try {
+            fis.close();
+        } catch (IOException ioe) {
+            ioe.printStackTrace();
+            if (ioe.getSuppressed().length != 2) {
+                throw new RuntimeException("[FIS]Incorrect number of suppressed " +
+                          "exceptions received : " + ioe.getSuppressed().length);
+            }
+        }
+        if (raf.getFD().valid()) {
+            // we should still have closed the FD
+            // even with the exception.
+            throw new RuntimeException("[FIS]TestCloseAll : FD still valid.");
+        }
+
+        // Now test with FileOutputStream
+
+        raf = new RandomAccessFile(testFile, "rw");
+        FileOutputStream fos = new FileOutputStream(raf.getFD());
+        BadFileOutputStream bfos1 = new BadFileOutputStream(raf.getFD());
+        BadFileOutputStream bfos2 = new BadFileOutputStream(raf.getFD());
+        BadFileOutputStream bfos3 = new BadFileOutputStream(raf.getFD());
+        // extra test - set bfos3 to null
+        bfos3 = null;
+        try {
+            fos.close();
+        } catch (IOException ioe) {
+            ioe.printStackTrace();
+            if (ioe.getSuppressed().length != 2) {
+                throw new RuntimeException("[FOS]Incorrect number of suppressed " +
+                          "exceptions received : " + ioe.getSuppressed().length);
+            }
+        }
+        if (raf.getFD().valid()) {
+            // we should still have closed the FD
+            // even with the exception.
+            throw new RuntimeException("[FOS]TestCloseAll : FD still valid.");
+        }
+    }
+
     /**
      * A thread which will open and close a number of FileInputStreams and
      * FileOutputStreams referencing the same native file descriptor.
@@ -325,12 +374,35 @@ public class FileDescriptorSharing {
                  System.out.println("OpenClose encountered IO issue :" + ioe);
                  fail = true;
              } finally {
-                 if (!fd.valid()) { // fd should still be valid given RAF reference
-                     System.out.println("OpenClose: FileDescriptor should be valid");
+                 if (fd.valid()) { // fd should not be valid after first close() call
+                     System.out.println("OpenClose: FileDescriptor shouldn't be valid");
                      fail = true;
                  }
                  done.countDown();
              }
          }
     }
+
+    private static class BadFileInputStream extends FileInputStream {
+
+        BadFileInputStream(FileDescriptor fd) {
+            super(fd);
+        }
+
+        public void close() throws IOException {
+            throw new IOException("Bad close operation");
+        }
+    }
+
+    private static class BadFileOutputStream extends FileOutputStream {
+
+        BadFileOutputStream(FileDescriptor fd) {
+            super(fd);
+        }
+
+        public void close() throws IOException {
+            throw new IOException("Bad close operation");
+        }
+    }
+
 }