Merge pull request #168 from cmelchior/cm/bug/classpath-close

[WIP] Fix leaking file handlers

.gitignore

@@ -1,3 +1,4 @@
+.idea/
 bin/
 eclipse-output/
 target/

Readme.html

@@ -281,6 +281,14 @@ see javassist.Dump.
 
 <h2>Changes</h2>
 
+<p>-version 3.23 on MMM DD, YYYY
+
+<ul>
+    <li>Fix leaking file handlers in ClassPool and removed ClassPath.close(). Github issue #165.
+</ul>
+</p>
+
+
 <p>-version 3.22 on October 10, 2017
 
 <ul>

pom.xml

@@ -140,6 +140,11 @@
   <build>
     <sourceDirectory>src/main/</sourceDirectory>
     <testSourceDirectory>src/test/</testSourceDirectory>
+    <testResources>
+      <testResource>
+        <directory>src/test/resources</directory>
+      </testResource>
+    </testResources>
     <plugins>
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
@@ -161,8 +166,11 @@
           <includes>
             <include>javassist/JvstTest.java</include>
           </includes>
-	  <forkMode>once</forkMode>
-      <workingDirectory>${project.build.directory}/runtest</workingDirectory>
+          <forkMode>once</forkMode>
+          <additionalClasspathElements>
+            <additionalClasspathElement>resources</additionalClasspathElement>
+          </additionalClasspathElements>
+          <workingDirectory>${project.build.directory}/runtest</workingDirectory>
         </configuration>
       </plugin>
       <plugin>

src/main/javassist/ByteArrayClassPath.java

@@ -63,12 +63,6 @@ public class ByteArrayClassPath implements ClassPath {
         this.classfile = classfile;
     }
 
-    /**
-     * Closes this class path.
-     */
-    @Override
-    public void close() {}
-
     @Override
     public String toString() {
         return "byte[]:" + classname;

src/main/javassist/ClassClassPath.java

@@ -91,13 +91,6 @@ public class ClassClassPath implements ClassPath {
         return thisClass.getResource(filename);
     }
 
-    /**
-     * Does nothing.
-     */
-    @Override
-    public void close() {
-    }
-
     @Override
     public String toString() {
         return thisClass.getName() + ".class";

src/main/javassist/ClassPath.java

@@ -58,11 +58,4 @@ public interface ClassPath {
      * @return null if the specified class file could not be found.
      */
     URL find(String classname);
-
-    /**
-     * This method is invoked when the <code>ClassPath</code> object is
-     * detached from the search path.  It will be an empty method in most of
-     * classes.
-     */
-    void close();
 }

src/main/javassist/ClassPoolTail.java

@@ -25,6 +25,9 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.MalformedURLException;
 import java.net.URL;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 import java.util.jar.JarEntry;
 import java.util.jar.JarFile;
 
@@ -74,9 +77,6 @@ final class DirClassPath implements ClassPath {
         return null;
     }
 
-    @Override
-    public void close() {}
-
     @Override
     public String toString() {
         return directory;
@@ -125,71 +125,63 @@ final class JarDirClassPath implements ClassPath {
 
         return null;    // not found
     }
-
-    @Override
-    public void close() {
-        if (jars != null)
-            for (int i = 0; i < jars.length; i++)
-                jars[i].close();
-    }
 }
 
 final class JarClassPath implements ClassPath {
-    JarFile jarfile;
+    List<String> jarfileEntries;
     String jarfileURL;
 
     JarClassPath(String pathname) throws NotFoundException {
+        JarFile jarfile = null;
         try {
             jarfile = new JarFile(pathname);
+            jarfileEntries = new ArrayList<String>();
+            for (JarEntry je:Collections.list(jarfile.entries()))
+                if (je.getName().endsWith(".class"))
+                    jarfileEntries.add(je.getName());
             jarfileURL = new File(pathname).getCanonicalFile()
-                                           .toURI().toURL().toString();
+                    .toURI().toURL().toString();
             return;
+        } catch (IOException e) {}
+        finally {
+            if (null != jarfile)
+                try {
+                    jarfile.close();
+                } catch (IOException e) {}
         }
-        catch (IOException e) {}
         throw new NotFoundException(pathname);
     }
 
     @Override
     public InputStream openClassfile(String classname)
-        throws NotFoundException
+            throws NotFoundException
     {
-        try {
-            String jarname = classname.replace('.', '/') + ".class";
-            JarEntry je = jarfile.getJarEntry(jarname);
-            if (je != null)
-                return jarfile.getInputStream(je);
-            return null;    // not found
-        }
-        catch (IOException e) {}
-        throw new NotFoundException("broken jar file?: "
-                                    + jarfile.getName());
+        URL jarURL = find(classname);
+        if (null != jarURL)
+            try {
+                return jarURL.openConnection().getInputStream();
+            }
+            catch (IOException e) {
+                throw new NotFoundException("broken jar file?: "
+                        + classname);
+            }
+        return null;
     }
 
     @Override
     public URL find(String classname) {
         String jarname = classname.replace('.', '/') + ".class";
-        JarEntry je = jarfile.getJarEntry(jarname);
-        if (je != null)
+        if (jarfileEntries.contains(jarname))
             try {
-                return new URL("jar:" + jarfileURL + "!/" + jarname);
+                return new URL(String.format("jar:%s!/%s", jarfileURL, jarname));
             }
             catch (MalformedURLException e) {}
-
         return null;            // not found
     }
 
-    @Override
-    public void close() {
-        try {
-            jarfile.close();
-            jarfile = null;
-        }
-        catch (IOException e) {}
-    }
-
     @Override
     public String toString() {
-        return jarfile == null ? "<null>" : jarfile.toString();
+        return jarfileURL == null ? "<null>" : jarfileURL.toString();
     }
 }
 
@@ -247,8 +239,6 @@ final class ClassPoolTail {
                     else
                         list = list.next;
             }
-
-        cp.close();
     }
 
     public ClassPath appendSystemPath() {

src/main/javassist/LoaderClassPath.java

@@ -90,12 +90,4 @@ public class LoaderClassPath implements ClassPath {
         URL url = cl.getResource(cname);
         return url;
     }
-
-    /**
-     * Closes this class path.
-     */
-    @Override
-    public void close() {
-        clref = null;
-    }
 }

src/main/javassist/URLClassPath.java

@@ -117,12 +117,6 @@ public class URLClassPath implements ClassPath {
         return null; 
     }
 
-    /**
-     * Closes this class path.
-     */
-    @Override
-    public void close() {}
-
     /**
      * Reads a class file on an http server.
      *

src/main/javassist/scopedpool/ScopedClassPool.java

@@ -119,7 +119,6 @@ public class ScopedClassPool extends ClassPool {
      */
     public void close() {
         this.removeClassPath(classPath);
-        classPath.close();
         classes.clear();
         softcache.clear();
     }

src/test/Readme.txt

@@ -1,2 +1,18 @@
-check javassist.JvstTestRoot.PATH and .JAR_PATH and then
-run javassist.JvstTest under ./runtest
+# How to run tests
+
+Requirements:
+Java JDK 9
+Maven
+
+1) Build jar file and move it to the top level folder.
+
+    > mvn package
+    > mv ./target/javassist*-GA.jar ./javaassist.jar
+
+2) Check that ./src/test/javassist/JvstTestRoot.PATH and .JAR_PATH point to the compiled jar file.
+   The default is "../../".
+
+3) Run Tests
+
+    > mvn test
+    > mvn surefire:test

src/test/javassist/JvstTest.java

@@ -1,7 +1,9 @@
 package javassist;
 
 import junit.framework.*;
+import java.io.File;
 import java.io.FileInputStream;
+import java.io.InputStream;
 import java.lang.reflect.Method;
 import javassist.bytecode.*;
 import javassist.expr.*;
@@ -64,6 +66,36 @@ public class JvstTest extends JvstTestRoot {
         assertTrue("[class path: ]".equals(pool.toString()));
     }
 
+    public void testReleaseJarClassPathFileHandle() throws Exception {
+        String jarFileName = "./empty.jar";
+        ClassLoader classLoader = getClass().getClassLoader();
+        File jarFile = new File(classLoader.getResource(jarFileName).getFile());
+        assertTrue(jarFile.exists());
+
+        // Prepare class pool and force it to open the Jar file
+        ClassPool pool = ClassPool.getDefault();
+        ClassPath cp = pool.appendClassPath(jarFile.getAbsolutePath());
+        assertNull(cp.openClassfile("nothere.Dummy"));
+
+        // Assert that it is possible to delete the jar file.
+        // On Windows deleting an open file will fail, while on on Mac/Linux this is always possible.
+        // This check will thus only fail on Windos if the file is still open.
+        assertTrue(jarFile.delete());
+    }
+
+    public void testJarClassPath() throws Exception {
+        String jarFileName = "./simple.jar";
+        ClassLoader classLoader = getClass().getClassLoader();
+        File jarFile = new File(classLoader.getResource(jarFileName).getFile());
+        assertTrue(jarFile.exists());
+
+        ClassPool pool = ClassPool.getDefault();
+        ClassPath cp = pool.appendClassPath(jarFile.getAbsolutePath());
+        InputStream is = cp.openClassfile("com.test.Test");
+        assertNotNull(is);
+        is.close();
+    }
+
     public void testSubtype() throws Exception {
         CtClass cc = sloader.get("test1.Subtype");
         assertTrue(cc.subtypeOf(cc));

src/test/resources/Readme.txt

@@ -0,0 +1,16 @@
+This directory contains files used by the unit tests.
+
+empty.jar:
+An empty, but valid, jar file.
+
+simple.jar:
+Contains a single Java class
+
+```
+package com.test;
+
+public class Test {
+	public Test() {
+	}
+}
+```