Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 88128

Summary: Eclipse and JVM Shutdown Hooks
Product: [Eclipse Project] Platform Reporter: Tom Musta <tmusta>
Component: RuntimeAssignee: Thomas Watson <tjwatson>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: jeffmcaffer, pascal
Version: 3.1   
Target Milestone: 3.1 M7   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Bug Depends on: 88915    
Bug Blocks:    
Attachments:
Description Flags
Proposed solution
none
updated patch none

Description Tom Musta CLA 2005-03-15 17:36:06 EST
We are trying to port legacy runtime environments onto the Eclipse platform.
Some of these environments use JVM shutdown hooks that perform some amount of
cleanup.  Unfortunately, in an Eclipse environment the bundles are stopped
before the shutdown hooks are executed, which severely restricts what can be
done in the hook.  For example, no classes may be loaded.

In general, we do not own the code for the hooks, so we cannot easily change the
code to fit some other mechanism (e.g. bundle start levels).

We can, however, live without a standard shutdown of the Eclipse platform.  That
is, if bundles stayed active during the JVM shutdown, I believe that would be
acceptable.

Here is a very trivial example.  If you run it, you should get the following output:

osgi> 
### adding shutdown hook
### in shutdown hook com.acme.Main$ShutdownHook
java.lang.NoClassDefFoundError: com/acme/Foo
        at com.acme.Main$ShutdownHook.run(Main.java:16)



-------------- main application -----------
package com.acme;

import org.eclipse.core.runtime.IPlatformRunnable;

public class Main implements IPlatformRunnable {

    public Object run(Object arg0) throws Exception {
        System.out.println("### adding shutdown hook");
        Runtime.getRuntime().addShutdownHook(new ShutdownHook());
        return null;
    }

    private static class ShutdownHook extends Thread {
        public void run() {
            System.out.println("### in shutdown hook " + this.getClass().getName());
            Foo f = new Foo();
        }
    }
}

--------------- auxiliary class Foo --------------------

package com.acme;

public class Foo {
}

-------------- plugin.xml -----------------------

<?xml version="1.0" encoding="UTF-8"?>
<?eclipse version="3.0"?>
<plugin>

   <extension id="Test" point="org.eclipse.core.runtime.applications">
     <application>
       <run class="com.acme.Main"/>
     </application>
   </extension>
 
</plugin>

------------------ manifest.mf ----------------
Manifest-Version: 1.0
Bundle-Name: Sample Bundle
Bundle-SymbolicName: com.bugsrus
Bundle-Version: 1.0.0
Bundle-ClassPath: testbundle.jar
Bundle-Vendor: Acme Corp.
Bundle-Localization: plugin
Eclipse-AutoStart: true
Import-Package: org.eclipse.core.runtime,
  org.osgi.framework
Comment 1 Jeff McAffer CLA 2005-03-15 22:58:13 EST
need to investigate for M6 as part of the classloading work.
Comment 2 Thomas Watson CLA 2005-03-15 23:14:56 EST
is this related to bug 73059?
Comment 3 Tom Musta CLA 2005-03-16 08:55:47 EST
Yes, this does appear to be the same issue documented in bug 73059.

I will look further into preloading the set of classes needed by the shutdown
hook but in general this will not be practical because parts of the
implementations come from 3rd party code.  It will be very brittle.
Comment 4 Thomas Watson CLA 2005-03-16 09:22:18 EST
I agree, I seem to remember that we did not close the classloaders on 
shutdown.  Jeff, does this not sound familiar?

I notice that you are using Eclipse-AutoStart: true

I think if you use some of the advanced features of the Eclipse-AutoStart 
header then you can filter some packages to prevent your bundle from being 
auto-started.  For example the packages with your shutdown hooks should not 
cause your bundle to be auto-started.  Jeff or Rafael, can you point Tom to 
some documentation about the Eclipse-AutoStart header?
Comment 5 Jeff McAffer CLA 2005-03-16 09:37:27 EST
There is an "exceptions" attribute (TJ, should this be a directive?).  
Basically you set Eclipse-AutoStart to the base behaviour (true or false) and 
then specify exception packages which cause the opposite behaivour.  So in 

Eclipse-AutoStart: true;exceptions="foo"

the bundle will be autostarted for loads of all classes in all packages 
*except* the foo package.
Comment 6 Thomas Watson CLA 2005-03-16 09:46:26 EST
Technically it should be a directive.  I'd hate to have to convert now.  This 
has been available since 3.0 ...

Is there and value in converting this to a directive?  Tooling would be 
effected too ...
Comment 7 Jeff McAffer CLA 2005-03-16 11:07:02 EST
ok, lets not change it.

on the classloader closing, I don't remember.  There must be classloaders being 
closed or disabled because classloads are failing.  We need to recapture the 
logic used here.
Comment 8 Thomas Watson CLA 2005-03-16 11:16:32 EST
see bug 47255.  That explains that we do not close classloaders on shutdown.  
I think the classloader error occurs if the bundle has Eclipse-AutoStart: 
true.  this causes the EclipseClassLoader to prevent classloads if the bundle 
is stopped.
Comment 9 Jeff McAffer CLA 2005-03-16 11:20:21 EST
see also bug 72797
Comment 10 Jeff McAffer CLA 2005-03-16 11:26:42 EST
Cool. so this means that Tom M can either turn off Autostart or list the 
classes involved in shutdown as exceptions.  The bummer would be if for some 
reason he needed autostart to be triggered by loading of classes from the same 
package as the shutdown code.
Comment 11 Tom Musta CLA 2005-03-16 13:37:37 EST
I've tried flipping Auto-Start to false in my simple example and it does
not seem to have any effect (R3.1M5a).  What I'm seeing in the debugger
is that all bundle loaders are being closed as the framework shuts down:

   Thread [System Bundle Shutdown] (Suspended (breakpoint at line 251 in
BundleLoader))
	BundleLoader.close() line: 251
	BundleHost.closeBundleLoader(BundleLoaderProxy) line: 558
	BundleHost.refresh() line: 132
	StartLevelManager.unloadAllBundles(BundleRepository) line: 699
	StartLevelManager.decFWSL(int) line: 617
	StartLevelManager.doSetStartLevel(int, AbstractBundle) line: 284
	StartLevelManager.shutdown() line: 257
	SystemBundle.suspend() line: 190
	Framework.shutdown() line: 420
	SystemBundle$1.run() line: 171
	Thread.run() line: 568

The shutdown hook is triggered by the System.exit() in the
org.eclipse.core.launcher.Main which occurs after framework shutdown.  At this
point, the BundleLoader will not allow any additional classes to be loaded.
Comment 12 Thomas Watson CLA 2005-03-16 14:36:57 EST
Something has changed since 3.0 that reintroduced this problem.  I will 
investigate.
Comment 13 Thomas Watson CLA 2005-03-16 15:01:45 EST
This is not a regression.  The same bug is in 3.0.1.  We are closing the 
BundleLoaders on shutdown which is causing this problem.
Comment 14 Jeff McAffer CLA 2005-03-16 15:39:11 EST
hmmm.  I was pretty sure we were closing them but I'm getting pretty old...

So what to do? some sort of indication that they should not be closed?  and 
what about reactivation?
Comment 15 Thomas Watson CLA 2005-03-16 16:02:04 EST
The simple fix is to just not unload all the bundles on shutdown.  I believe 
that was the original intent of bug 47255.  But that was not fixed properly.  
I tested this out, and it seems to fix the problem but it opens us up to other 
issues.  For example, at shutdown the frameworkLog is set to null.  This will 
cause NPE if any logging is needed while loading the classes in the shutdown 
hooks.  I ran into an NPE when the plugin had autostart set to true because 
the EclipseClassLoader wanted to log an error ...
Comment 16 Jeff McAffer CLA 2005-03-16 18:34:45 EST
there has to be an end to the expectations.  I mean if someone registers 
shutdown code, the code has to be pretty darn careful about what it does as the 
system is going down so various parts are being torn apart.  TomM, do you have 
a sense as to what kinds of things are needed during this shutdown code?  for 
example, can we say that it will not reference any OSGi classes?  will only 
load classes from its own bundle (and the system classloader)?  we have to put 
a box around it somehow.
Comment 17 Thomas Watson CLA 2005-03-23 16:13:36 EST
before fixing this, we need to clean up the open files we are leaving arround 
on shutdown ...
Comment 18 Thomas Watson CLA 2005-03-31 10:41:58 EST
We encountered several issues while investigating this.  Most having to do 
with enabling classloaders to function after the framework has shutdown.  
Several NPE's started to occur if the classloader was used after shutdown.  
Will look into implementing a solution in M7.
Comment 19 Thomas Watson CLA 2005-04-13 12:13:12 EDT
Created attachment 19872 [details]
Proposed solution

This patch allows existing bundle classloaders to load classes even after the
framework has been shutdown.  The bundle classloader and BundleLoader objects
can still function and use the proper OSGi delegation model.  Note that all
references to the BundleLoader and bundle classloader objects have been dropped
by the framework.  In theory this would allow a classloader to be GC'ed if all
classes it loaded have been GC'ed.  

In the shutdown hook case this would not happen because an object would still
exist for the actual shutdown hook.  Also all resources (jar files) have been
closed for the bundles and their classloaders upon shutdown.  If a class is
loaded after the framework has been shutdown then the jars will be reopened on
demand.  Potentially new classloaders could even be created in cases where an
existing BundleLoader has not created the classloader yet.  I tested these
different scenarios out and they all worked as expected.  

Please note that you must not use Eclipse-AutoStart: true in any of the bundles
which contain code that may run in your shutdown hook.	The EclispeClassLoader
will prevent any classes from being loaded after shutdown in bundles marked
with the Eclipse-AutoStart flag.

Jeff, can you review the patch.  Thanks.
Comment 20 Thomas Watson CLA 2005-04-13 14:41:18 EDT
Created attachment 19884 [details]
updated patch

Had to release a fix to BundleLoader for dynamic imports.  This is a new patch
against the latest in HEAD.
Comment 21 Thomas Watson CLA 2005-04-13 14:44:32 EDT
including Pascal for the review because he loves classloaders as much as I 
do :-)
Comment 22 Pascal Rapicault CLA 2005-04-18 09:27:43 EDT
I reviewed the patch and it seems to contain the refactorings to use bit masks
instead of booleans and the fix for this bug.
For clarity we should release them independently.

In BundleLoader.close() shouldn't we keep the "null'ation" of classloader and
bundle, and skip them when a system property is enabled?
Comment 23 Thomas Watson CLA 2005-04-19 13:41:13 EDT
I moved to using bit masks because I did not want to introduce yet another 
boolean slot to indicate the BundleLoader is closed.  Previously the code 
checked if the bundle == null to see if the BundleLoader was closed.  I can 
release the fix to move to bit masks before releasing the rest of the fix.

We no longer null out any of the slots in BundleLoader during shutdown.  This 
is to allow the BundleLoader to remain functional.  We do remove all 
references to the BundleLoaderProxy's.  All access to BundleLoaders (and 
BundleClassLoaders) from the framework is done through the BundleLoaderProxy.  
Keeping a refernce to the BundleClassLaoder from the BundleLoader should not 
prevent the BundleClassLoader from being GC'ed as long as no references exist 
to the BundleLoaderProxy.
Comment 24 Pascal Rapicault CLA 2005-04-19 14:54:18 EDT
I released the patch in HEAD without splitting the concern.