Bug 190279

Comment 1 Richard Backhouse 2007-05-31 14:01:20 EDT

Created attachment 69566 [details]
FrameworkLauncher using a non-locking resource ClassLoader for the framework classloader

This patch provides an example of a framework classloader that does not extend URLClassloader. It instead uses its own URLSteamHandler and URLConnection to create streams that are memory based and thus does not lock the resource. It has an addURL() method for framework extension support although this currently does nothing. Surprisingly, I found this worked with our environment. The classloader expects a single framework jar url. Support could be added to handle additional urls added via the addURL method. The classloader extends SecureClassLoader. I copied the permission code from FrameworkLauncher into the JarEntryClassLoader to handle permission requests. 

The code is pretty rough and ready and has not been tested that much. For my own test purposes I was able to deploy a bridgeservlet based WAR into Tomcat and using the Tomcat Admin WebUI was able to undeploy the WAR and observe that the temp directory was cleanup correctly. Feel free to use it as a starting point.

Comment 2 Thomas Watson 2007-06-07 10:00:16 EDT

There is a way to get all the JarFile objects from a URLClassLoader.  Something like the following (untested code) could be added to ChildFirstURLClassLoader to close all the JarFiles on shutdown

void close() {
  try {
    Enumeration manifests = findResources("META-INF/MANIFEST.MF");
    if (manifests == null)
      return;
    while (manifests.hasMoreElements()) {
      URL manifestURL = (URL) manifests.nextElement();
      if (!"jar".equals(manifestURL.getProtocol()))
        continue;
      JarURLConnection connection = 
            (JarURLConnection) manifestURL.openConnection();
      JarFile jarFile = connection.getJarFile();
      if (jarFile != null)
        jarFile.close();
    }
  } catch (IOException e) {
    // ah well, consider logging but not much point
  }
}

This method would be called by FrameworkLauncher after the framework has shutdown.

Comment 3 Richard Backhouse 2007-06-08 09:27:09 EDT

I tried Tom's code and it does not work. I see the "org.eclipse.osgi" and the "org.eclipse.equinox.servletbridge.extensionbundle" bundles get closed by this code but the resources are still locked and the temp directory is not removed because of that.

Comment 4 Simon Kaegi 2008-10-07 09:51:23 EDT

I tried an approach that extended the URLClassLoader and used the URLClassLoader's constructor that allows one to pass in a URLStreamHandlerFactory. For this approach I wrote a new JarURLStreamHandler and JARURLConnection that in theory would allow us to correctly close the associated JarFile. Unfortunately what I found is that the URLClassLoader has a built in internal optimization to handle file based JAR's specially. The net effect was that my handler was called to parse the URL but then "never" to connect and create my custom URLConnection. Instead the URLClassLoader has it's own internal JarFile management which leaves the JarFile's open with no way to go in and close them.

If we're going to do something here I suspect it will mean crafting our very own JarClassLoader independent of the URLClassLoader (... just like Richard did in the first place).

Comment 5 Thomas Watson 2008-10-14 11:01:58 EDT

Created attachment 115044 [details]
URLClassLoader closer

Another (dreadful) option is to use reflection to pick through the URLClassLoader and close the ZipFiles.  Here is an example of how to do that.

Comment 6 Thomas Watson 2008-10-14 11:02:42 EDT

I tested this on IBM and Sun 1.5 VMs.  I seriously doubt it would work on Harmony.

Comment 7 Simon Kaegi 2008-10-20 00:07:20 EDT

Created attachment 115535 [details]
CloseableURLClassLoader + changes in FrameworkLauncher

Here's an implementation of URLClassLoader that adds a close() method that will close any jar files it manages. It does this by intercepting any file urls that the URLClassLoader would normally attempt to load as jar files and providing its own handling. I have a basic set of test-cases written tha I'm still adding to, but so far it seems to do the trick at least for the undeploy use case in the Servletbridge.

Comment 8 Simon Kaegi 2008-10-20 11:18:00 EDT

Created attachment 115574 [details]
add verify jar flag to CloseableURLClassLoader

This is a slight tweak on the previous patch as I recall Tom mentioning that it might be good if we supported a mechanism to avoid having to verify the framework jars. The FrameworkLauncher in this patch now uses this tweak.

Comment 9 Thomas Watson 2008-10-23 09:27:52 EDT

Created attachment 115926 [details]
patch review

Overall I think the patch is looking good.  I made a few minor tweaks to the patch.  Mainly added some GuardedBy comments to help me understand the synchronization.  I also change CloseableJarURLConnection to throw an IOException if the entry could not be found for some reason on connect or getInputStream.

I also removed the ArrayList of entry names in favor of using jarFile.getEntry to check if the entry exists in getURL.  JarFile.getEntry should be highly optimized for indexing of entry names.

I am wandering if it would be better to construct a new CloseableJarURLStreamHandler for each call to CloseableJarFileLoader.getURL and pass the JarEntry to the stream handler directly instead of depending on JarURLConnect.super(URL) to parse the entry name out for us so we can call getJarEntry(getEntryName()) on connect.  Simon, what do you think?

Comment 10 Simon Kaegi 2008-10-23 11:01:49 EDT

Thanks Tom. I like your "tweaks".

Given your patch avoids caching entries internally, your URLStreamHandler comment is also probably the way to go as it will let us avoid a second call to jarfile.getEntry. Although it seems a bit strange constructing a new URLStreamHandler for every URL it looks to me like the cheapest/best alternative. I think we still want to extend JARURLConnection although there is a cost in terms of parsing.

Another optimization I thought about but never did, was to do the work in defining a class more directly off of a JarEntry intead of through getting a URL etc. This would let us avoid the parsing costs for the URL and JarURLConnection but would add some complexity. We'd have to measure to see if it's worth it. My take is that if the performance is inline with URLClassLoader are job here is done.

Comment 11 Simon Kaegi 2008-10-23 22:25:51 EDT

I did some further timing and testing and found absolutely no difference in terms of perfomance with doing anything fancier. So... as a result I'm going to stick with your tweaked patch as I like it best. Checked into HEAD. 

Richard, could you try this out to see if it works for you?

--
In terms of timing I ran a few tests that loaded every class with and without verification.

On my laptop...
with verification: 400ms +- 50
without verification: 200ms +- 20

The CloseableURLClassLoader performed more or less identically (possibly slightly better) as compared to the URLClassLoader when verification was on. In the Servletbridge FrameworkLauncher I'm now taking advantage of this free 200ms :)

Finally, I took a look with a code-level profiler and can see that all the time is spent reading bytes and defining classes so it looks like further tweaking isn't worth it. I think we're done so long as the implementation is correct.

Comment 12 Richard Backhouse 2008-10-24 11:13:30 EDT

Simon, this looks great. I was able to stop and undeploy the whole of the RTC server and then redeploy it without having to stop the process. The working directory was completely free of locks.

Comment 13 Simon Kaegi 2008-10-25 13:58:33 EDT

Marking FIXED.
Thanks Richard.