6829919: URLClassLoader.close() doesn't close resource file if...

6829919: URLClassLoader.close() doesn't close resource file if getResourceAsStream(...) was called before
Reviewed-by: chegar

src/share/classes/java/net/URLClassLoader.java

@@ -27,19 +27,15 @@ package java.net;
 
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
-import java.io.File;
-import java.io.FilePermission;
-import java.io.InputStream;
-import java.io.IOException;
-import java.io.Closeable;
+import java.lang.ref.*;
+import java.io.*;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLStreamHandlerFactory;
 import java.util.Enumeration;
-import java.util.List;
-import java.util.NoSuchElementException;
-import java.util.StringTokenizer;
+import java.util.*;
 import java.util.jar.Manifest;
+import java.util.jar.JarFile;
 import java.util.jar.Attributes;
 import java.util.jar.Attributes.Name;
 import java.security.CodeSigner;
@@ -194,6 +190,65 @@ public class URLClassLoader extends SecureClassLoader implements Closeable {
         acc = AccessController.getContext();
     }
 
+    /* A map (used as a set) to keep track of closeable local resources
+     * (either JarFiles or FileInputStreams). We don't care about
+     * Http resources since they don't need to be closed.
+     *
+     * If the resource is coming from a jar file
+     * we keep a (weak) reference to the JarFile object which can
+     * be closed if URLClassLoader.close() called. Due to jar file
+     * caching there will typically be only one JarFile object
+     * per underlying jar file.
+     *
+     * For file resources, which is probably a less common situation
+     * we have to keep a weak reference to each stream.
+     */
+
+    private WeakHashMap<Closeable,Void>
+        closeables = new WeakHashMap<>();
+
+    /**
+     * Returns an input stream for reading the specified resource.
+     * If this loader is closed, then any resources opened by this method
+     * will be closed.
+     *
+     * <p> The search order is described in the documentation for {@link
+     * #getResource(String)}.  </p>
+     *
+     * @param  name
+     *         The resource name
+     *
+     * @return  An input stream for reading the resource, or <tt>null</tt>
+     *          if the resource could not be found
+     *
+     * @since  1.7
+     */
+    public InputStream getResourceAsStream(String name) {
+        URL url = getResource(name);
+        try {
+            if (url == null) {
+                return null;
+            }
+            URLConnection urlc = url.openConnection();
+            InputStream is = urlc.getInputStream();
+            if (urlc instanceof JarURLConnection) {
+                JarURLConnection juc = (JarURLConnection)urlc;
+                JarFile jar = juc.getJarFile();
+                synchronized (closeables) {
+                    if (!closeables.containsKey(jar)) {
+                        closeables.put(jar, null);
+                    }
+                }
+            } else if (urlc instanceof sun.net.www.protocol.file.FileURLConnection) {
+                synchronized (closeables) {
+                    closeables.put(is, null);
+                }
+            }
+            return is;
+        } catch (IOException e) {
+            return null;
+        }
+    }
 
    /**
     * Closes this URLClassLoader, so that it can no longer be used to load
@@ -202,8 +257,8 @@ public class URLClassLoader extends SecureClassLoader implements Closeable {
     * delegation hierarchy are still accessible. Also, any classes or resources
     * that are already loaded, are still accessible.
     * <p>
-    * In the case of jar: and file: URLs, it also closes any class files,
-    * or JAR files that were opened by it. If another thread is loading a
+    * In the case of jar: and file: URLs, it also closes any files
+    * that were opened by it. If another thread is loading a
     * class when the {@code close} method is invoked, then the result of
     * that load is undefined.
     * <p>
@@ -213,10 +268,10 @@ public class URLClassLoader extends SecureClassLoader implements Closeable {
     * loader has no effect.
     * <p>
     * @throws IOException if closing any file opened by this class loader
-    * resulted in an IOException. Any such exceptions are caught, and a
-    * single IOException is thrown after the last file has been closed.
-    * If only one exception was thrown, it will be set as the <i>cause</i>
-    * of this IOException.
+    * resulted in an IOException. Any such exceptions are caught internally.
+    * If only one is caught, then it is re-thrown. If more than one exception
+    * is caught, then the second and following exceptions are added
+    * as suppressed exceptions of the first one caught, which is then re-thrown.
     *
     * @throws SecurityException if a security manager is set, and it denies
     *   {@link RuntimePermission}<tt>("closeClassLoader")</tt>
@@ -229,21 +284,33 @@ public class URLClassLoader extends SecureClassLoader implements Closeable {
             security.checkPermission(new RuntimePermission("closeClassLoader"));
         }
         List<IOException> errors = ucp.closeLoaders();
+
+        // now close any remaining streams.
+
+        synchronized (closeables) {
+            Set<Closeable> keys = closeables.keySet();
+            for (Closeable c : keys) {
+                try {
+                    c.close();
+                } catch (IOException ioex) {
+                    errors.add(ioex);
+                }
+            }
+            closeables.clear();
+        }
+
         if (errors.isEmpty()) {
             return;
         }
-        if (errors.size() == 1) {
-            throw new IOException (
-                "Error closing URLClassLoader resource",
-                errors.get(0)
-            );
-        }
-        // Several exceptions. So, just combine the error messages
-        String errormsg = "Error closing resources: ";
+
+        IOException firstex = errors.remove(0);
+
+        // Suppress any remaining exceptions
+
         for (IOException error: errors) {
-            errormsg = errormsg + "[" + error.toString() + "] ";
+            firstex.addSuppressed(error);
         }
-        throw new IOException (errormsg);
+        throw firstex;
     }
 
     /**

test/java/net/URLClassLoader/closetest/CloseTest.java

@@ -36,94 +36,7 @@ import java.net.*;
 import java.lang.reflect.*;
 import com.sun.net.httpserver.*;
 
-public class CloseTest {
-
-    static void copyFile (String src, String dst) {
-        copyFile (new File(src), new File(dst));
-    }
-
-    static void copyDir (String src, String dst) {
-        copyDir (new File(src), new File(dst));
-    }
-
-    static void copyFile (File src, File dst) {
-        try {
-            if (!src.isFile()) {
-                throw new RuntimeException ("File not found: " + src.toString());
-            }
-            dst.delete();
-            dst.createNewFile();
-            FileInputStream i = new FileInputStream (src);
-            FileOutputStream o = new FileOutputStream (dst);
-            byte[] buf = new byte [1024];
-            int count;
-            while ((count=i.read(buf)) >= 0) {
-                o.write (buf, 0, count);
-            }
-            i.close();
-            o.close();
-        } catch (IOException e) {
-            throw new RuntimeException (e);
-        }
-    }
-
-    static void rm_minus_rf (File path) {
-        if (!path.exists()) {
-            return;
-        }
-        if (path.isFile()) {
-            if (!path.delete()) {
-                throw new RuntimeException ("Could not delete " + path);
-            }
-        } else if (path.isDirectory ()) {
-            String[] names = path.list();
-            File[] files = path.listFiles();
-            for (int i=0; i<files.length; i++) {
-                rm_minus_rf (new File(path, names[i]));
-            }
-            if (!path.delete()) {
-                throw new RuntimeException ("Could not delete " + path);
-            }
-        } else {
-            throw new RuntimeException ("Trying to delete something that isn't a file or a directory");
-        }
-    }
-
-    static void copyDir (File src, File dst) {
-        if (!src.isDirectory()) {
-            throw new RuntimeException ("Dir not found: " + src.toString());
-        }
-        if (dst.exists()) {
-            throw new RuntimeException ("Dir exists: " + dst.toString());
-        }
-        dst.mkdir();
-        String[] names = src.list();
-        File[] files = src.listFiles();
-        for (int i=0; i<files.length; i++) {
-            String f = names[i];
-            if (files[i].isDirectory()) {
-                copyDir (files[i], new File (dst, f));
-            } else {
-                copyFile (new File (src, f), new File (dst, f));
-            }
-        }
-    }
-
-    /* expect is true if you expect to find it, false if you expect not to */
-    static Class loadClass (String name, URLClassLoader loader, boolean expect){
-        try {
-            Class clazz = Class.forName (name, true, loader);
-            if (!expect) {
-                throw new RuntimeException ("loadClass: "+name+" unexpected");
-            }
-            return clazz;
-        } catch (ClassNotFoundException e) {
-            if (expect) {
-                throw new RuntimeException ("loadClass: " +name + " not found");
-            }
-        }
-        return null;
-    }
+public class CloseTest extends Common {
 
 //
 // needs two jar files test1.jar and test2.jar with following structure

test/java/net/URLClassLoader/closetest/Common.java

+/*
+ * Copyright (c) 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
+ * 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.
+ */
+
+import java.io.*;
+import java.net.*;
+
+public class Common {
+
+    static void copyFile (String src, String dst) {
+        copyFile (new File(src), new File(dst));
+    }
+
+    static void copyDir (String src, String dst) {
+        copyDir (new File(src), new File(dst));
+    }
+
+    static void copyFile (File src, File dst) {
+        try {
+            if (!src.isFile()) {
+                throw new RuntimeException ("File not found: " + src.toString());
+            }
+            dst.delete();
+            dst.createNewFile();
+            FileInputStream i = new FileInputStream (src);
+            FileOutputStream o = new FileOutputStream (dst);
+            byte[] buf = new byte [1024];
+            int count;
+            while ((count=i.read(buf)) >= 0) {
+                o.write (buf, 0, count);
+            }
+            i.close();
+            o.close();
+        } catch (IOException e) {
+            throw new RuntimeException (e);
+        }
+    }
+
+    static void rm_minus_rf (File path) {
+        if (!path.exists()) {
+            return;
+        }
+        if (path.isFile()) {
+            if (!path.delete()) {
+                throw new RuntimeException ("Could not delete " + path);
+            }
+        } else if (path.isDirectory ()) {
+            String[] names = path.list();
+            File[] files = path.listFiles();
+            for (int i=0; i<files.length; i++) {
+                rm_minus_rf (new File(path, names[i]));
+            }
+            if (!path.delete()) {
+                throw new RuntimeException ("Could not delete " + path);
+            }
+        } else {
+            throw new RuntimeException ("Trying to delete something that isn't a file or a directory");
+        }
+    }
+
+    static void copyDir (File src, File dst) {
+        if (!src.isDirectory()) {
+            throw new RuntimeException ("Dir not found: " + src.toString());
+        }
+        if (dst.exists()) {
+            throw new RuntimeException ("Dir exists: " + dst.toString());
+        }
+        dst.mkdir();
+        String[] names = src.list();
+        File[] files = src.listFiles();
+        for (int i=0; i<files.length; i++) {
+            String f = names[i];
+            if (files[i].isDirectory()) {
+                copyDir (files[i], new File (dst, f));
+            } else {
+                copyFile (new File (src, f), new File (dst, f));
+            }
+        }
+    }
+
+    /* expect is true if you expect to find it, false if you expect not to */
+    static Class loadClass (String name, URLClassLoader loader, boolean expect){
+        try {
+            Class clazz = Class.forName (name, true, loader);
+            if (!expect) {
+                throw new RuntimeException ("loadClass: "+name+" unexpected");
+            }
+            return clazz;
+        } catch (ClassNotFoundException e) {
+            if (expect) {
+                throw new RuntimeException ("loadClass: " +name + " not found");
+            }
+        }
+        return null;
+    }
+}

test/java/net/URLClassLoader/closetest/GetResourceAsStream.java

+/*
+ * Copyright (c) 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
+ * 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 6899919
+ * @run shell build2.sh
+ * @run main/othervm GetResourceAsStream
+ */
+
+import java.io.*;
+import java.net.*;
+
+public class GetResourceAsStream extends Common {
+
+/*
+ * We simply test various scenarios with class/resource files
+ * and make sure the files can be deleted after closing
+ * the loader. Therefore, the test will only really be verified
+ * on Windows. It will still run correctly on other platforms
+ */
+    public static void main (String args[]) throws Exception {
+
+        String workdir = System.getProperty("test.classes");
+        if (workdir == null) {
+            workdir = args[0];
+        }
+
+        /* the jar we copy for each test */
+        File srcfile = new File (workdir, "foo.jar");
+
+        /* the jar we use for the test */
+        File testfile = new File (workdir, "test.jar");
+
+        copyFile (srcfile, testfile);
+        test (testfile, false, false);
+
+        copyFile (srcfile, testfile);
+        test (testfile, true, false);
+
+        copyFile (srcfile, testfile);
+        test (testfile, true, true);
+
+        // repeat test using a directory of files
+
+        File testdir= new File (workdir, "testdir");
+        File srcdir= new File (workdir, "test3");
+
+        copyDir (srcdir, testdir);
+        test (testdir, true, false);
+
+    }
+
+    // create a loader on jarfile (or directory)
+    // load a class , then look for a resource
+    // then close the loader
+    // check further new classes/resources cannot be loaded
+    // check jar (or dir) can be deleted
+
+    static void test (File file, boolean loadclass, boolean readall)
+        throws Exception
+    {
+        URL[] urls = new URL[] {file.toURL()};
+        System.out.println ("Doing tests with URL: " + urls[0]);
+        URLClassLoader loader = new URLClassLoader (urls);
+        if (loadclass) {
+            Class testclass = loadClass ("com.foo.TestClass", loader, true);
+        }
+        InputStream s = loader.getResourceAsStream ("hello.txt");
+        s.read();
+        if (readall) {
+            while (s.read() != -1) ;
+            s.close();
+        }
+
+        loader.close ();
+
+        // shouuld not find bye.txt now
+        InputStream s1 = loader.getResourceAsStream("bye.txt");
+        if (s1 != null) {
+            throw new RuntimeException ("closed loader returned resource");
+        }
+
+        // now check we can delete the path
+        rm_minus_rf (file);
+        System.out.println (" ... OK");
+    }
+}

test/java/net/URLClassLoader/closetest/build2.sh

+#!/bin/sh
+#
+#  Copyright (c) 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
+#  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.
+#
+if [ "${TESTSRC}" = "" ]
+then
+  echo "TESTSRC not set.  Test cannot execute.  Failed."
+  exit 1
+fi
+echo "TESTSRC=${TESTSRC}"
+
+if [ "${TESTJAVA}" = "" ]
+then
+  echo "TESTJAVA not set.  Test cannot execute.  Failed."
+  exit 1
+fi
+echo "TESTJAVA=${TESTJAVA}"
+
+if [ "${TESTCLASSES}" = "" ]
+then
+  echo "TESTCLASSES not set.  Test cannot execute.  Failed."
+  exit 1
+fi
+
+JAVAC="${TESTJAVA}/bin/javac"
+JAR="${TESTJAVA}/bin/jar"
+
+rm -rf ${TESTCLASSES}/test3
+mkdir -p ${TESTCLASSES}/test3
+
+echo "Hello world" > ${TESTCLASSES}/test3/hello.txt
+echo "Bye world" > ${TESTCLASSES}/test3/bye.txt
+cp ${TESTSRC}/test1/com/foo/TestClass.java ${TESTCLASSES}/test3
+cd ${TESTCLASSES}/test3
+${JAVAC} -d . TestClass.java
+
+${JAR} cvf foo.jar hello.txt bye.txt com/foo/TestClass.class
+rm -f ../foo.jar
+mv foo.jar ..