ClassPool is leaking file handles

[cmelchior]

Reported in realm/realm-java#5521

By further investigation, it appears that the ClassPool is holding on to file descriptors if ClassPath elements referencing Jar or Zip files are not removed again before discarding the ClassPool.

This can be seen in the docs for ClassPath.close(): https://github.com/jboss-javassist/javassist/blob/master/src/main/javassist/ClassPath.java#L67 which are only being called when calling ClassPool.removeClassPath(ClassPath): https://github.com/jboss-javassist/javassist/blob/master/src/main/javassist/ClassPoolTail.java#L239

This is rather non-obvious as the ClassPool is not marked as Closable and nowhere in the description of the ClassPool does it indicate that you need to cleanup resources.

I'm not sure if this is a bug, but as a minimum the docs probably need to be updated to make this use case clear.

Right now we work around it by having subclass ManagedClassPool that is marked as Closable and remember all ClassPath elements so it can remove them when close() is called.

[nickl-]

I agree that interface ClassPath should probably extend java.io.Closeable which in turns means it does not have to define the inherited void close(); member anymore. This is probably missing due to pre 1.5 compatibility which never got refactored. Since javassist is currently minimum source 1.6 there is no reason that I can see why we should not reference Closeable.

However, I am not convinced that jar and/or zip file handles are not correctly terminated. Are you not perhaps using openClassFile in your code and holding on to the returned InputStream references by any chance?

As far as I can tell the only time javassist opens these streams is in ClassPoolTail.writeClassfile which does correctly release the resource see:

javassist/src/main/javassist/ClassPoolTail.java

Lines 281 to 297 in e41e079

	/**
	* This method does not close the output stream.
	*/
	void writeClassfile(String classname, OutputStream out)
	throws NotFoundException, IOException, CannotCompileException
	{
	InputStream fin = openClassfile(classname);
	if (fin == null)
	throw new NotFoundException(classname);
	try {
	copyStream(fin, out);
	}
	finally {
	fin.close();
	}
	}

Unless I am mistaken...

[cmelchior]

If you call ClassPoolTail.appendClassPath(String) with "mylib.jar" It will result in makePathObject() being called which in turn will create an instance of JarClassPath. This class holds a JarFile variable which is not closed until ClassPool.removeClassPath(ClassPath) is called. In our, and I assume many others, case this is not done because you have no way of knowing that was needed.

Relevant code:
https://github.com/jboss-javassist/javassist/blob/master/src/main/javassist/ClassPoolTail.java#L257
https://github.com/jboss-javassist/javassist/blob/master/src/main/javassist/ClassPoolTail.java#L268
https://github.com/jboss-javassist/javassist/blob/master/src/main/javassist/ClassPoolTail.java#L135

It is entirely possible I'm missing something though

[nickl-]

Ahh yes, you are correct, my bad. JarFile does not indicate any special needs/requirements but its super class ZipFile on the other hand implements Closeable and opens the archive when instantiated.

JarClassPath keeps a reference to the open file which needs to be released with JarClassPath::close but absent Closeable contract no one is aware of unreleased resources.

ClassPoolTail retains a ClassPathList and implements ClassPoolTail::removeClassPath, after doing some obscure juggling/comparison, will close the supplied ClassPath but this is neither self managed, documented or in any other indication an obvious requirement. I can't help but wish the purpose was more clear, surely this can be simplified.

javassist/src/main/javassist/ClassPoolTail.java

Lines 226 to 240 in e41e079

	public synchronized void removeClassPath(ClassPath cp) {
	ClassPathList list = pathList;
	if (list != null)
	if (list.path == cp)
	pathList = list.next;
	else {
	while (list.next != null)
	if (list.next.path == cp)
	list.next = list.next.next;
	else
	list = list.next;
	}
	cp.close();
	}

ClassPool delegates to a locally referenced ClassPoolTail but also refrains from doing any cleanup. The exposed ClassPool::removeClassPath provides some documentation, referring to "detaching" the supplied class path and noting that a "detached" class path cannot be added again, which might not be the complete truth, nor is the reason why clear but none the less, the documentation makes no mention of any required use.

javassist/src/main/javassist/ClassPool.java

Lines 1000 to 1007 in e41e079

	/**
	* Detatches the <code>ClassPath</code> object from the search path.
	* The detached <code>ClassPath</code> object cannot be added
	* to the path again.
	*/
	public void removeClassPath(ClassPath cp) {
	source.removeClassPath(cp);
	}

Note: Since you need to locate the class path instance to supply to removeClassPath why would you bother with removeClassPath since manually closing it will release the resource too. I guess what I am getting at, what exactly is the the purpose of removeClassPath? If it is intended to be used for releasing resources, as we are assuming, it's not very helpful.

I have to agree, this seems broken or incomplete and we need to find a fix.

As I mentioned in my first comment, it is ClassPath which stipulates the requirement of a close method which can be inherited from Closeable instead, but do we need the hassle of having to manage ClassPath instances? Having a look at the current implementations of ClassPath specifically with regards to the close method implementations, I noticed something rather peculiar.

ClassPath implementation	close method implementation
ByteArrayClassPath	empty close method implementation
ClassClassPath	empty close method implementation
URLClassPath	empty close method implementation
DirClassPath	empty close method implementation
LoaderClassPath	close method nullifies weak reference (not so much a necessity)
JarDirClassPath	close method releases 0 or more JarClassPath instances
JarClassPath	close method to release JarFile instance

This means that so far, unless a ClassPath is an instance of JarClassPath it does not actually require Closeable, which begs the question should ClassPath be a managed resource then?

If we want to free ClassPath from requiring a close method implementation we still need to manage JarFile. Possible approaches:

• The easy and probably cleanest solution is to retain a reference to the entries instead, for lookup purposes, and closing the opened archive.

The only disadvantage I can think of is a possible performance knock having to open & close the archive each time to load a class found to be located within said archive and whether this will be noticeable/significant.

• Another option is for JarClassPath to self manage its resources which could be accomplished via a shutdown hook or similar.

Disadvantage is always that these hooks cannot be guaranteed which kind of defeats the purpose.

• Let the containing classes (ClassPool and/or ClassPoolTail) implement Closeable instead and in so doing shift the responsibility to the api user.

The enclosing class knows what resources it has opened and should at the very least provide a documented mechanism by which to release them. I think this is the solution @cmelchior had in mind but instead of tracking ClassPath instances we only need to track JarClassPath instances freeing ClassPath from the Closeable contract.

Or am I making no sense whatsoever =)

[cmelchior]

No, that is pretty much what I had in mind 😄

This was also our work-around as seen here: https://github.com/realm/realm-java/blob/master/realm-transformer/src/main/groovy/io/realm/transformer/ManagedClassPool.groovy

The problem with only letting ClassPath implement Closable is you end up shifting the responsibility to API users in a non-obvious way, e.g you need to save a reference to the ClassPath cp = classPool.appendClassPath("mylib.jar"); and then manually close it when done with ClassPool. This is not an obvious API IMO , even if it was documented, so I would much prefer if ClassPool/ClassPoolTail implemented Closable itself.

That said, I have only touched a very small part of the JavaAssist API, and you seem to have other ideas about avoiding having to close anything at all. If that is possible it would be less breaking to already existing users of ClassPool.

[nickl-]

Oh yes you make a valid point... this won't break backward compatibility but instead a fix will apply backwards because nobody, ok most bodies present company excluded, weren't aware they needed to do resource clean up anyway.

As a non intrusive fix the ManagedClassPool implementation is solid and gets the job done! I am not against a Closeable fix but you hit the nail on the head, "avoiding having to close anything at all" seems ideal. if we're taking the time to fix it lets fix it done.

What I had in mind is turning this snippet:

javassist/src/main/javassist/ClassPoolTail.java

Lines 129 to 183 in e41e079

	final class JarClassPath implements ClassPath {
	JarFile jarfile;
	String jarfileURL;
	JarClassPath(String pathname) throws NotFoundException {
	try {
	jarfile = new JarFile(pathname);
	jarfileURL = new File(pathname).getCanonicalFile()
	.toURI().toURL().toString();
	return;
	}
	catch (IOException e) {}
	throw new NotFoundException(pathname);
	}
	public InputStream openClassfile(String classname)
	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());
	}
	public URL find(String classname) {
	String jarname = classname.replace('.', '/') + ".class";
	JarEntry je = jarfile.getJarEntry(jarname);
	if (je != null)
	try {
	return new URL("jar:" + jarfileURL + "!/" + jarname);
	}
	catch (MalformedURLException e) {}
	return null; // not found
	}
	public void close() {
	try {
	jarfile.close();
	jarfile = null;
	}
	catch (IOException e) {}
	}
	public String toString() {
	return jarfile == null ? "<null>" : jarfile.toString();
	}
	}

Into something like:

hmmmmmm ok maybe I should stare at this a little while longer. Mind you... if I am not mistaken URL should be able to load a class directly out of its jar file, that would save us having to open and close the archive every time to load the class file. hmmmmmmm pondering

Let me hack out a new JarClassPath in a follow up post.

@cmelchior I hope you are keen to drive this patch, I really don't mind helping out but I'm not looking to swipe this from you. As you may have noticed I already have several patches lying in waiting, not at all sure how I feel about that but either way I should probably not pile on more work before those get done. Besides having more pending PRs form devs other than myself might just be the motivation we need. =)

This definitely needs to be fixed, lets get it done!

[nickl-]

So something on these lines:

final class JarClassPath implements ClassPath {
    List<String> jarfileEntries;
    String jarfileURL;

    JarClassPath(String pathname) throws NotFoundException {
        try(JarFile jarfile = new JarFile(pathname)) {
            jarfileEntries = jarfile.stream()
                    .map(e->e.getName())
                    .filter(e->e.endsWith(".class"))
                    .collect(Collectors.toList());
            jarfileURL = new File(pathname).getCanonicalFile()
                               .toURI().toURL().toString();
            return;
        }
        catch (IOException e) {}
        throw new NotFoundException(pathname);
    }

    public InputStream openClassfile(String classname)
        throws NotFoundException
    {
        URL jarURL = find(classname);
        if (null != jarURL)
            try {

                return jarURL.openConnection().getInputStream();
            }
        catch (IOException e) {}
        throw new NotFoundException("broken jar file?: "
                                    + classname);
    }

    public URL find(String classname) {
        String jarname = classname.replace('.', '/') + ".class";
        if (jarfileEntries.contains(jarname)) 
            try {
                return new URL("jar:" + jarfileURL + "!/" + jarname);
            }
            catch (MalformedURLException e) {}

        return null;            // not found
    }

    public String toString() {
        return jarfileURL == null ? "<null>" : jarfileURL;
    }
}

Ok it's not source=1.6, will have to dumb it down a bit, does give the general idea. All tests succeed so that has to be a good sign.

Basic break down:

• Filter JarFile::entries for JarEntry.getName().endsWith(".class") and collect List<String> jarfileEntries of names.
• find method transforms class name to file name and if jarfileEntries::contains return new jar URL
• open uses find if not null opens connection and returns InputStream

@cmelchior What you thinking?

Not sure if URL::openConnection() needs closing or general TLC.... checking

[nickl-]

Seems impossible to screw this up.

URL::openConnection return URLConnection but more specifically, based on the URL being of schema jar returns JarURLConnection. It doesn't actually open the connection only when URLConnection::connect is called but from the looks of things anything that needs to be connected implicitly calls connect anyway.

JarURLConnection still uses JarFile internally and just look how gorgeously beautiful. JarURLConnection::getInputStream returns a JarInputStream that looks like this:

    class JarURLInputStream extends java.io.FilterInputStream {
        JarURLInputStream (InputStream src) {
            super (src);
        }
        public void close () throws IOException {
            try {
                super.close();
            } finally {
                if (!getUseCaches()) {
                    jarFile.close();
                }
            }
        }
    }

JarInputStream has close method overridden to include JarFile::close so everything is closed on InputStream::close... could not have thought up a more elegant solution. BOOM!

Now seeing this I am convinced we have the correct solution in hand.

[nickl-]

So here you go the dumbed down and source=1.6 compliant version of JarClassPath

final class JarClassPath implements ClassPath {
    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();
            return;
        } catch (IOException e) {}
        finally {
            if (null != jarfile)
            try {
                jarfile.close();
            } catch (IOException e) {}
        }
        throw new NotFoundException(pathname);
    }

    @Override
    public InputStream openClassfile(String classname)
        throws NotFoundException
    {
        URL jarURL = find(classname);
        if (null != jarURL)
        try {
            return jarURL.openConnection().getInputStream();
        }
        catch (IOException e) {}
        throw new NotFoundException("broken jar file?: "
                                    + classname);
    }

    @Override
    public URL find(String classname) {
        String jarname = classname.replace('.', '/') + ".class";
        if (jarfileEntries.contains(jarname)) 
        try {
            return new URL(String.format("jar:%s!/%s", jarfileURL, jarname));
        }
        catch (MalformedURLException e) {}
        return null;            // not found
    }

    @Override
    public String toString() {
        return jarfileURL == null ? "<null>" : jarfileURL.toString();
    }
}

nJoy!

[cmelchior]

Cool, I'll be happy to put together a PR. Would it also make sense to deprecate ClassPath.close() in the same PR, since it then no longer seems required? (I don't assume you want to remove it straight away).

[nickl-]

Yes go ahead and roll away.

Checklist:

• Replace JarClassPath with new implementation.

• Remove ClassPath::close

Since it's package scope and not public API, add to that the fact that we just replaced the only actual implementation I think they can all be removed.

• Remove ByteArrayClassPath::close
• Remove ClassClassPath::close
• Remove URLClassPath::close
• Remove DirClassPath::close
• Remove LoaderClassPath::close
• Remove JarDirClassPath::close
• Remove cp.close(); from ClassPoolTail::removeClassPath

hmmm I still don't like that ClassPoolTail::removeClassPath implementation but perhaps it's out of scope for this PR, what you think?

Did I miss anything?

[nickl-]

@cmelchior please feel free to make it your own!

[cmelchior]

@nickl- I created #168, but there are a few open questions I hope you can answer?