[MSC-193] Fixing broken ServiceContainer shutdown process.

There were two kind of problems with previous solution:
[1] shutdownListener was executed in regular listeners chain
[2] race condition between shutdown() and install() methods

Both aforementioned problems sometimes caused unexpected
premature executor shutdown while there were still
pending tasks (either scheduled or not) to be executed.

This caused many broken scenarios to happen.

One example of broken scenario (assuming [1] was the problem):
1) There is only one service A in registry in UP state.
   It has 1 registered transition listener.
2) Thread {1} called Container.shutdown() and this method
   completed successfully. It associated shutdownListener
   with controller for A and scheduled it for removal.
   There are two listeners now in controller for A
   and A is transitioning to REMOVED state.
3) A is not in REMOVED state yet
4) User code executing in Thread {2} calls
   Container.awaitStability(). This method hangs
   because previous Container.shutdown() asked
   service A to stop, but A is still transitioning.
5) A is entering REMOVED state and its RemoveTask is finished.
6) Last step is to notify listeners that REMOVING -> REMOVED
   transition happened.
7) shutdownListener is executed first and because it
   was registered just with one service it calls
   executor.shutdown(). Executor is terminated.
8) Problem is there is pending last listener task not executed yet
   and because it didn't execute yet stability notification
   was not sent to service container that controller stability
   was achieved. User code in Thread {2} will hang forever.

Similar example of broken scenario can be constructed
assuming [2] was the initial problem.

This fix:
 * removes terminateListener from regular controller
   listeners chain and creates separate and last controller phase
   intended just for container termination listeners.
   When controller enters REMOVED state and its stability
   is achieved (i.e. all its tasks have been executed)
   terminate listener is being called. This is the only
   appropriate time to call it.
 * container instrinsic lock is being held
   while container.install() is associating controller with
   primary and alias registrations. Only so it can be guaranteed
   shutdown() will not see stale registry data (no race condition)

src/main/java/org/jboss/msc/service/ContainerShutdownListener.java

@@ -0,0 +1,64 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2017, Red Hat, Inc., and individual contributors
+ * as indicated by the @author tags. See the copyright.txt file in the
+ * distribution for a full listing of individual contributors.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+
+package org.jboss.msc.service;
+
+import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
+
+/**
+ * @author <a href="mailto:ropalka@redhat.com">Richard Opalka</a>
+ */
+final class ContainerShutdownListener {
+    @SuppressWarnings({ "UnusedDeclaration" })
+    private volatile int count = 1;
+    @SuppressWarnings({ "UnusedDeclaration" })
+    private volatile int done;
+    @SuppressWarnings({ "RawUseOfParameterizedType" })
+    private static final AtomicIntegerFieldUpdater<ContainerShutdownListener> countUpdater = AtomicIntegerFieldUpdater.newUpdater(ContainerShutdownListener.class, "count");
+    @SuppressWarnings({ "RawUseOfParameterizedType" })
+    private static final AtomicIntegerFieldUpdater<ContainerShutdownListener> doneUpdater = AtomicIntegerFieldUpdater.newUpdater(ContainerShutdownListener.class, "done");
+    private final Runnable callback;
+
+    ContainerShutdownListener(final Runnable callback) {
+        this.callback = callback;
+    }
+
+    final void controllerAlive() {
+        countUpdater.getAndIncrement(this);
+    }
+
+    final void controllerDied() {
+        tick();
+    }
+
+    final void done() {
+        if (doneUpdater.getAndSet(this, 1) == 0) {
+            tick();
+        }
+    }
+
+    private void tick() {
+        if (countUpdater.decrementAndGet(this) == 0) {
+            callback.run();
+        }
+    }
+}

src/main/java/org/jboss/msc/service/ServiceContainerImpl.java

@@ -26,14 +26,9 @@
 import static org.jboss.modules.management.ObjectProperties.property;
 
 import java.io.ByteArrayOutputStream;
-import java.io.FileNotFoundException;
-import java.io.FileOutputStream;
-import java.io.OutputStreamWriter;
 import java.io.PrintStream;
 import java.io.UnsupportedEncodingException;
-import java.io.Writer;
 import java.lang.management.ManagementFactory;
-import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
@@ -146,9 +141,9 @@ private ShutdownHookHolder() {
         }
     }
 
-    private TerminateListener.Info terminateInfo = null;
+    private volatile TerminateListener.Info terminateInfo;
 
-    private volatile boolean down = false;
+    private volatile boolean down;
 
     private final ContainerExecutor executor;
 
@@ -371,6 +366,7 @@ private void printServiceStatus(Collection<ServiceStatus> serviceStatuses, Print
         synchronized (set) {
             // if the shutdown hook was triggered, then no services can ever come up in any new containers.
             if (ShutdownHookHolder.down) {
+                terminateInfo = new TerminateListener.Info(System.nanoTime(), System.nanoTime());
                 down = true;
             }
             //noinspection ThisEscapedInObjectConstruction
@@ -447,12 +443,19 @@ long getStart() {
     }
 
     @Override
-    public synchronized void addTerminateListener(TerminateListener listener) {
-        if (terminateInfo != null) { // if shutdown is already performed
-            listener.handleTermination(terminateInfo); // invoke handleTermination immediately
+    public void addTerminateListener(final TerminateListener listener) {
+        if (terminateInfo == null) {
+            synchronized (this) {
+                if (terminateInfo == null) {
+                    terminateListeners.add(listener);
+                    return;
+                }
+            }
         }
-        else {
-            terminateListeners.add(listener);
+        try {
+            listener.handleTermination(terminateInfo);
+        } catch (final Throwable t) {
+            // ignored
         }
     }
 
@@ -527,38 +530,33 @@ public boolean isShutdown() {
     }
 
     public void shutdown() {
-        final MultipleRemoveListener<Runnable> shutdownListener;
-        synchronized(this) {
-            if (down){
-                return;
-            }
+        synchronized (this) {
+            if (down) return;
             down = true;
             shutdownInitiated = System.nanoTime();
         }
-        shutdownListener = MultipleRemoveListener.create(new Runnable() {
+        final ContainerShutdownListener shutdownListener = new ContainerShutdownListener(new Runnable() {
             public void run() {
                 executor.shutdown();
             }
         });
-        final HashSet<ServiceControllerImpl<?>> done = new HashSet<ServiceControllerImpl<?>>();
+        ServiceControllerImpl<?> controller;
         for (ServiceRegistrationImpl registration : registry.values()) {
-            ServiceControllerImpl<?> serviceInstance = registration.getInstance();
-            if (serviceInstance != null && serviceInstance.getSubstate() != Substate.CANCELLED && serviceInstance.getSubstate() != Substate.REMOVED && done.add(serviceInstance)) {
+            controller = registration.getInstance();
+            if (controller != null) {
+                controller.addListener(shutdownListener);
                 try {
-                    serviceInstance.addListener(shutdownListener);
-                } catch (IllegalArgumentException e) {
-                    continue;
+                    controller.setMode(Mode.REMOVE);
+                } catch (IllegalArgumentException ignored) {
+                    // controller removed in the meantime
                 }
-                serviceInstance.setMode(Mode.REMOVE);
             }
         }
         shutdownListener.done();
     }
 
     public boolean isShutdownComplete() {
-        synchronized (this) {
-            return terminateInfo != null;
-        }
+        return terminateInfo != null;
     }
 
     public void dumpServices() {
@@ -591,8 +589,10 @@ protected void finalize() throws Throwable {
         shutdown();
     }
 
-    private synchronized void shutdownComplete(long started) {
-        terminateInfo = new TerminateListener.Info(started, System.nanoTime());
+    private void shutdownComplete(final long started) {
+        synchronized (this) {
+            terminateInfo = new TerminateListener.Info(started, System.nanoTime());
+        }
         for (TerminateListener terminateListener : terminateListeners) {
             try {
                 terminateListener.handleTermination(terminateInfo);
@@ -691,9 +691,6 @@ void apply(ServiceBuilderImpl<?> builder) {
 
     @Override
     <T> ServiceController<T> install(final ServiceBuilderImpl<T> serviceBuilder) throws DuplicateServiceException {
-        if (down) {
-            throw new IllegalStateException ("Container is down");
-        }
         apply(serviceBuilder);
 
         // Get names & aliases
@@ -744,7 +741,19 @@ <T> ServiceController<T> install(final ServiceBuilderImpl<T> serviceBuilder) thr
         boolean ok = false;
         try {
             serviceValue.setValue(instance);
-            instance.startInstallation();
+            synchronized (this) {
+                if (down) {
+                    throw new IllegalStateException ("Container is down");
+                }
+                // It is necessary to call startInstallation() under container intrinsic lock.
+                // This is the only point in MSC code where ServiceRegistrationImpl.instance
+                // field is being set to non null value. So in order for shutdown() method
+                // which iterates 'registry' concurrent hash map outside of intrinsic lock
+                // to see up to date installed controllers this synchronized section is necessary.
+                // Otherwise it may happen container stability will be seriously broken on shutdown.
+                instance.startInstallation();
+            }
+            instance.startConfiguration();
             // detect circularity before committing
             detectCircularity(instance);
             instance.commitInstallation(serviceBuilder.getInitialMode());

src/main/java/org/jboss/msc/service/ServiceControllerImpl.java

@@ -79,6 +79,10 @@ final class ServiceControllerImpl<S> implements ServiceController<S>, Dependent
      * The set of registered service listeners.
      */
     private final IdentityHashSet<ServiceListener<? super S>> listeners;
+    /**
+     * Container shutdown listener.
+     */
+    private ContainerShutdownListener shutdownListener;
     /**
      * The set of registered stability monitors.
      */
@@ -214,10 +218,9 @@ Substate getSubstateLocked() {
     }
 
     /**
-     * Start this service installation, connecting it to its parent and dependencies. Also,
-     * set the instance in primary and alias registrations.
-     * <p>
-     * All notifications from dependencies, parents, and registrations will be ignored until the
+     * Set this instance into primary and alias registrations.
+     * <p></p>
+     * All notifications from registrations will be ignored until the
      * installation is {@link #commitInstallation(org.jboss.msc.service.ServiceController.Mode) committed}.
      */
     void startInstallation() {
@@ -227,14 +230,23 @@ void startInstallation() {
         } finally {
             primaryRegistration.getLock().writeLock().unlock();
         }
-        for (ServiceRegistrationImpl aliasRegistration: aliasRegistrations) {
+        for (ServiceRegistrationImpl aliasRegistration : aliasRegistrations) {
             aliasRegistration.getLock().writeLock().lock();
             try {
                 aliasRegistration.setInstance(this);
             } finally {
                 aliasRegistration.getLock().writeLock().unlock();
             }
         }
+    }
+
+    /**
+     * Start this service configuration connecting it to its parent and dependencies.
+     * <p></p>
+     * All notifications from dependencies and parents will be ignored until the
+     * installation is {@link #commitInstallation(org.jboss.msc.service.ServiceController.Mode) committed}.
+     */
+    void startConfiguration() {
         for (Dependency dependency : dependencies) {
             dependency.getLock().writeLock().lock();
             try {
@@ -371,6 +383,9 @@ private void updateStabilityState(final boolean leavingStableRestState) {
                 for (StabilityMonitor monitor : monitors) {
                     monitor.decrementUnstableServices();
                 }
+                if (shutdownListener != null && state == Substate.REMOVED) {
+                    shutdownListener.controllerDied();
+                }
             }
         }
     }
@@ -1148,6 +1163,20 @@ public ServiceName[] getAliases() {
         return names;
     }
 
+    public void addListener(final ContainerShutdownListener listener) {
+        assert !holdsLock(this);
+        synchronized (this) {
+            if (state == Substate.REMOVED && asyncTasks == 0) {
+                return; // controller is dead
+            }
+            if (shutdownListener != null) {
+                return; // register listener only once
+            }
+            shutdownListener = listener;
+            shutdownListener.controllerAlive();
+        }
+    }
+
     public void addListener(final ServiceListener<? super S> listener) {
         assert !holdsLock(this);
         ListenerTask listenerAddedTask, listenerRemovedTask = null;