From: Christian Melchior <christian@ilios.dk>
Date: Sun, 12 Nov 2017 14:17:19 +0000 (+0100)
Subject: Remove close method on ClassPath. Add unit tests for new behaviour.
X-Git-Tag: rel_3_23_0_ga~3^2~4
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=f4769c7ad702fa529a790106ea5c318a7af5c7b7;p=javassist.git

Remove close method on ClassPath. Add unit tests for new behaviour.
---

diff --git a/Readme.html b/Readme.html
index 563ce36f..4685062a 100644
--- a/Readme.html
+++ b/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>
diff --git a/pom.xml b/pom.xml
index 06d1f6da..6259547a 100644
--- a/pom.xml
+++ b/pom.xml
@@ -137,6 +137,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>
@@ -158,7 +163,10 @@
           <includes>
             <include>javassist/JvstTest.java</include>
           </includes>
-	  <forkMode>once</forkMode>
+          <additionalClasspathElements>
+            <additionalClasspathElement>resources</additionalClasspathElement>
+          </additionalClasspathElements>
+	      <forkMode>once</forkMode>
           <workingDirectory>runtest</workingDirectory>
         </configuration>
       </plugin>
diff --git a/src/main/javassist/ByteArrayClassPath.java b/src/main/javassist/ByteArrayClassPath.java
index d385eddc..00397470 100644
--- a/src/main/javassist/ByteArrayClassPath.java
+++ b/src/main/javassist/ByteArrayClassPath.java
@@ -62,11 +62,6 @@ public class ByteArrayClassPath implements ClassPath {
         this.classfile = classfile;
     }
 
-    /**
-     * Closes this class path.
-     */
-    public void close() {}
-
     public String toString() {
         return "byte[]:" + classname;
     }
diff --git a/src/main/javassist/ClassClassPath.java b/src/main/javassist/ClassClassPath.java
index 3befbf4c..e1c44f08 100644
--- a/src/main/javassist/ClassClassPath.java
+++ b/src/main/javassist/ClassClassPath.java
@@ -91,12 +91,6 @@ public class ClassClassPath implements ClassPath {
         return thisClass.getResource(filename);
     }
 
-    /**
-     * Does nothing.
-     */
-    public void close() {
-    }
-
     public String toString() {
         return thisClass.getName() + ".class";
     }
diff --git a/src/main/javassist/ClassPath.java b/src/main/javassist/ClassPath.java
index 0c605de2..60fe80e9 100644
--- a/src/main/javassist/ClassPath.java
+++ b/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();
 }
diff --git a/src/main/javassist/ClassPoolTail.java b/src/main/javassist/ClassPoolTail.java
index 1b580855..8e03873c 100644
--- a/src/main/javassist/ClassPoolTail.java
+++ b/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;
 
@@ -72,8 +75,6 @@ final class DirClassPath implements ClassPath {
         return null;
     }
 
-    public void close() {}
-
     public String toString() {
         return directory;
     }
@@ -118,67 +119,63 @@ final class JarDirClassPath implements ClassPath {
 
         return null;    // not found
     }
-
-    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);
-            else
-                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
     }
 
-    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();
     }
 }
 
@@ -235,8 +232,6 @@ final class ClassPoolTail {
                     else
                         list = list.next;
             }
-
-        cp.close();
     }
 
     public ClassPath appendSystemPath() {
diff --git a/src/main/javassist/LoaderClassPath.java b/src/main/javassist/LoaderClassPath.java
index 6921ca6e..299fdb85 100644
--- a/src/main/javassist/LoaderClassPath.java
+++ b/src/main/javassist/LoaderClassPath.java
@@ -94,11 +94,4 @@ public class LoaderClassPath implements ClassPath {
             return url;
         }
     }
-
-    /**
-     * Closes this class path.
-     */
-    public void close() {
-        clref = null;
-    }
 }
diff --git a/src/main/javassist/URLClassPath.java b/src/main/javassist/URLClassPath.java
index bea0b3c2..5eeef3a9 100644
--- a/src/main/javassist/URLClassPath.java
+++ b/src/main/javassist/URLClassPath.java
@@ -112,11 +112,6 @@ public class URLClassPath implements ClassPath {
         return null; 
     }
 
-    /**
-     * Closes this class path.
-     */
-    public void close() {}
-
     /**
      * Reads a class file on an http server.
      *
diff --git a/src/test/javassist/JvstTest.java b/src/test/javassist/JvstTest.java
index d660a335..11cdbe96 100644
--- a/src/test/javassist/JvstTest.java
+++ b/src/test/javassist/JvstTest.java
@@ -1,6 +1,7 @@
 package javassist;
 
 import junit.framework.*;
+import java.io.File;
 import java.io.FileInputStream;
 import java.lang.reflect.Method;
 import javassist.bytecode.*;
@@ -63,6 +64,27 @@ 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 {
+        // TODO: Verify that classes can be loaded from a JarClassPath
+    }
+
     public void testSubtype() throws Exception {
         CtClass cc = sloader.get("test1.Subtype");
         assertTrue(cc.subtypeOf(cc));
diff --git a/src/test/resources/Readme.txt b/src/test/resources/Readme.txt
new file mode 100644
index 00000000..93de248e
--- /dev/null
+++ b/src/test/resources/Readme.txt
@@ -0,0 +1,7 @@
+This directory contains files used by the unit tests.
+
+empty.jar:
+An empty, but valid, jar file.
+
+
+
diff --git a/src/test/resources/empty.jar b/src/test/resources/empty.jar
new file mode 100755
index 00000000..3a12dbae
Binary files /dev/null and b/src/test/resources/empty.jar differ