QPID-3325: unregister the shutdown hook when closing an ApplicationRegistry instance (by means other than the shutdownhook having run) and tidy up [use of] the close() method.

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1292479 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 81 insertions, 42 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
index 224d694932..3ca6dc1dc5 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
@@ -82,6 +82,8 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
 
     private static AtomicReference<IApplicationRegistry> _instance = new AtomicReference<IApplicationRegistry>(null);
 
+    private volatile Thread _shutdownHookThread;
+
     private final ServerConfiguration _configuration;
 
     private final Map<InetSocketAddress, QpidAcceptor> _acceptors = new HashMap<InetSocketAddress, QpidAcceptor>();
@@ -186,11 +188,6 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
         _qmfService = qmfService;
     }
 
-    static
-    {
-        Runtime.getRuntime().addShutdownHook(new Thread(new ShutdownService()));
-    }
-
     private static class ShutdownService implements Runnable
     {
         public void run()
@@ -246,6 +243,45 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
         }
     }
 
+    private void addShutdownHook()
+    {
+        Thread shutdownHookThread = new Thread(new ShutdownService());
+        Runtime.getRuntime().addShutdownHook(shutdownHookThread);
+        _shutdownHookThread = shutdownHookThread;
+    }
+
+    private void removeShutdownHook()
+    {
+        Thread shutdownThread = _shutdownHookThread;
+
+        //if there is a shutdown thread and we aren't it, we should remove it
+        if(shutdownThread != null && !(Thread.currentThread() == shutdownThread))
+        {
+            _logger.debug("Removing shutdown hook");
+
+            _shutdownHookThread = null;
+
+            boolean removed = false;
+            try
+            {
+                removed = Runtime.getRuntime().removeShutdownHook(shutdownThread);
+            }
+            catch(IllegalStateException ise)
+            {
+                //ignore, means the JVM is already shutting down
+            }
+
+            if(_logger.isDebugEnabled())
+            {
+                _logger.debug("Removed shutdown hook: " + removed);
+            }
+        }
+        else
+        {
+            _logger.debug("Skipping shutdown hook removal as there either isnt one, or we are it.");
+        }
+    }
+
     public ConfigStore getConfigStore()
     {
         return _configStore;
@@ -273,7 +309,6 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
                     _logger.info("Shutting down ApplicationRegistry(" + instance + ")");
                 }
                 instance.close();
-                instance.getBroker().getSystem().removeBroker(instance.getBroker());
             }
         }
         catch (Exception e)
@@ -355,6 +390,8 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
             // Startup complete, so pop the current actor
             CurrentActor.remove();
         }
+
+        addShutdownHook();
     }
 
 
@@ -536,35 +573,56 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
         }
     }
 
-
     public void close()
     {
         if (_logger.isInfoEnabled())
         {
             _logger.info("Shutting down ApplicationRegistry:" + this);
         }
-        
-        //Stop Statistics Reporting
-        if (_reportingTimer != null)
+
+        //Set the Actor for Broker Shutdown
+        CurrentActor.set(new BrokerActor(getRootMessageLogger()));
+        try
         {
-            _reportingTimer.cancel();
-        }
+            //Stop Statistics Reporting
+            if (_reportingTimer != null)
+            {
+                _reportingTimer.cancel();
+            }
 
-        //Stop incoming connections
-        unbind();
+            //Stop incoming connections
+            unbind();
 
-        //Shutdown virtualhosts
-        close(_virtualHostRegistry);
+            //Shutdown virtualhosts
+            close(_virtualHostRegistry);
 
-        close(_authenticationManager);
+            close(_authenticationManager);
 
-        close(_qmfService);
+            close(_qmfService);
 
-        close(_pluginManager);
+            close(_pluginManager);
 
-        close(_managedObjectRegistry);
+            close(_managedObjectRegistry);
 
-        CurrentActor.get().message(BrokerMessages.STOPPED());
+            BrokerConfig broker = getBroker();
+            if(broker != null)
+            {
+                broker.getSystem().removeBroker(broker);
+            }
+
+            CurrentActor.get().message(BrokerMessages.STOPPED());
+        }
+        finally
+        {
+            try
+            {
+                CurrentActor.remove();
+            }
+            finally
+            {
+                removeShutdownHook();
+            }
+        }
     }
 
     private void unbind()
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ConfigurationFileApplicationRegistry.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ConfigurationFileApplicationRegistry.java
index e9ba2764e5..b28e3d6c89 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ConfigurationFileApplicationRegistry.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ConfigurationFileApplicationRegistry.java
@@ -25,8 +25,6 @@ import org.osgi.framework.BundleContext;
 
 import org.apache.qpid.AMQException;
 import org.apache.qpid.server.configuration.ServerConfiguration;
-import org.apache.qpid.server.logging.actors.BrokerActor;
-import org.apache.qpid.server.logging.actors.CurrentActor;
 import org.apache.qpid.server.management.JMXManagedObjectRegistry;
 import org.apache.qpid.server.management.NoopManagedObjectRegistry;
 
@@ -45,22 +43,6 @@ public class ConfigurationFileApplicationRegistry extends ApplicationRegistry
     }
 
     @Override
-    public void close()
-    {
-        //Set the Actor for Broker Shutdown
-        CurrentActor.set(new BrokerActor(getRootMessageLogger()));
-        try
-        {
-            super.close();
-        }
-        finally
-        {
-            CurrentActor.remove();
-        }
-    }
-
-
-    @Override
     protected void initialiseManagedObjectRegistry() throws AMQException
     {
         if (getConfiguration().getManagementEnabled())
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/registry/ApplicationRegistryShutdownTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/registry/ApplicationRegistryShutdownTest.java
index 6a8a6dc2d0..9ff8f0a531 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/registry/ApplicationRegistryShutdownTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/registry/ApplicationRegistryShutdownTest.java
@@ -80,11 +80,10 @@ public class ApplicationRegistryShutdownTest extends InternalBrokerBaseCase
             }
         }
 
-        // Not using isEmpty as that is not in Java 5
-        assertTrue("No new SASL mechanisms added by initialisation.", additions.size() != 0 );
+        assertFalse("No new SASL mechanisms added by initialisation.", additions.isEmpty());
 
         //Close the registry which will perform the close the AuthenticationManager
-        getRegistry().close();
+        stopBroker();
 
         //Validate that the SASL plugFins have been removed.
         Provider[] providersAfterClose = Security.getProviders();