Merge pull request #5311 from ropalka/stability-fixes

[WFCORE-6141][WFCORE-6156][WFCORE-6157][WFCORE-6158] Server Stability Fixes

controller/src/main/java/org/jboss/as/controller/AbstractControllerService.java

@@ -89,7 +89,6 @@
 import org.jboss.msc.service.ServiceController;
 import org.jboss.msc.service.ServiceName;
 import org.jboss.msc.service.ServiceTarget;
-import org.jboss.msc.service.StabilityMonitor;
 import org.jboss.msc.service.StartContext;
 import org.jboss.msc.service.StartException;
 import org.jboss.msc.service.StopContext;
@@ -208,7 +207,6 @@ private static int getBootStackSize() {
     private final Supplier<ControllerInstabilityListener> instabilityListener;
     private final ExpressionResolver expressionResolver;
     private volatile ModelControllerImpl controller;
-    private volatile StabilityMonitor stabilityMonitor;
     private ConfigurationPersister configurationPersister;
     private final ManagedAuditLogger auditLogger;
     private final BootErrorCollector bootErrorCollector;
@@ -424,7 +422,7 @@ public void start(final StartContext context) throws StartException {
         ManagementResourceRegistration rootResourceRegistration = ManagementResourceRegistration.Factory.forProcessType(processType).createRegistration(rootResourceDefinition, authorizerConfig, capabilityRegistry);
         final ModelControllerImpl controller = new ModelControllerImpl(container, target,
                 rootResourceRegistration,
-                new ContainerStateMonitor(container, getStabilityMonitor()),
+                new ContainerStateMonitor(container),
                 configurationPersister, processType, runningModeControl, prepareStep,
                 processState, executorService, expressionResolver, authorizer, securityIdentitySupplier, auditLogger, notificationSupport,
                 bootErrorCollector, createExtraValidationStepHandler(), capabilityRegistry, getPartialModelIndicator(),
@@ -667,19 +665,11 @@ protected PartialModelIndicator getPartialModelIndicator() {
         return PartialModelIndicator.DEFAULT;
     }
 
-    protected final StabilityMonitor getStabilityMonitor() {
-        if (stabilityMonitor == null) {
-            stabilityMonitor = new StabilityMonitor();
-        }
-        return stabilityMonitor;
-    }
-
     public void stop(final StopContext context) {
         capabilityRegistry.clear();
         capabilityRegistry.publish();
         ServiceNameFactory.clearCache();
         controller = null;
-        stabilityMonitor = null;
         processState.setStopping();
         Runnable r = new Runnable() {
             @Override

controller/src/main/java/org/jboss/as/controller/ContainerStateMonitor.java

@@ -33,10 +33,9 @@
 import java.util.concurrent.TimeoutException;
 
 import org.jboss.as.controller.logging.ControllerLogger;
+import org.jboss.msc.service.ServiceContainer;
 import org.jboss.msc.service.ServiceController;
 import org.jboss.msc.service.ServiceName;
-import org.jboss.msc.service.ServiceRegistry;
-import org.jboss.msc.service.StabilityMonitor;
 import org.jboss.msc.service.StartException;
 
 import static org.jboss.as.controller.logging.ControllerLogger.ROOT_LOGGER;
@@ -47,17 +46,15 @@
  */
 final class ContainerStateMonitor {
 
-    private final ServiceRegistry serviceRegistry;
-    private final StabilityMonitor monitor;
+    private final ServiceContainer container;
     private final Set<ServiceController<?>> failed = new HashSet<ServiceController<?>>();
     private final Set<ServiceController<?>> problems = new HashSet<ServiceController<?>>();
 
     private Set<ServiceName> previousMissingDepSet = new HashSet<ServiceName>();
     private Set<ServiceController<?>> previousFailedSet = new HashSet<>();
 
-    ContainerStateMonitor(final ServiceRegistry registry, final StabilityMonitor stabilityMonitor) {
-        serviceRegistry = registry;
-        monitor = stabilityMonitor;
+    ContainerStateMonitor(final ServiceContainer container) {
+        this.container = container;
     }
 
     /**
@@ -74,10 +71,6 @@ void logContainerStateChangesAndReset() {
         }
     }
 
-    StabilityMonitor getStabilityMonitor() {
-        return monitor;
-    }
-
     /**
      * Await service container stability ignoring thread interruption.
      *
@@ -96,7 +89,7 @@ void awaitStabilityUninterruptibly(long timeout, TimeUnit timeUnit) throws Timeo
                     toWait = msTimeout - System.currentTimeMillis();
                 }
                 try {
-                    if (toWait <= 0 || !monitor.awaitStability(toWait, TimeUnit.MILLISECONDS, failed, problems)) {
+                    if (toWait <= 0 || !container.awaitStability(toWait, TimeUnit.MILLISECONDS, failed, problems)) {
                         throw new TimeoutException();
                     }
                     break;
@@ -121,7 +114,7 @@ void awaitStabilityUninterruptibly(long timeout, TimeUnit timeUnit) throws Timeo
      * @throws java.util.concurrent.TimeoutException if service container stability is not reached before the specified timeout
      */
     void awaitStability(long timeout, TimeUnit timeUnit) throws InterruptedException, TimeoutException {
-        if (!monitor.awaitStability(timeout, timeUnit, failed, problems)) {
+        if (!container.awaitStability(timeout, timeUnit, failed, problems)) {
             throw new TimeoutException();
         }
     }
@@ -140,7 +133,7 @@ void awaitStability(long timeout, TimeUnit timeUnit) throws InterruptedException
      * @throws java.util.concurrent.TimeoutException if service container stability is not reached before the specified timeout
      */
     ContainerStateChangeReport awaitContainerStateChangeReport(long timeout, TimeUnit timeUnit) throws InterruptedException, TimeoutException {
-        if (monitor.awaitStability(timeout, timeUnit, failed, problems)) {
+        if (container.awaitStability(timeout, timeUnit, failed, problems)) {
             return createContainerStateChangeReport(false);
         }
         throw new TimeoutException();
@@ -188,7 +181,7 @@ private synchronized ContainerStateChangeReport createContainerStateChangeReport
             noLongerMissingServices = new TreeMap<ServiceName, Boolean>();
             for (ServiceName name : previousMissing) {
                 if (!missingDeps.containsKey(name)) {
-                    ServiceController<?> controller = serviceRegistry.getService(name);
+                    ServiceController<?> controller = container.getService(name);
                     noLongerMissingServices.put(name, controller != null);
                 }
             }
@@ -203,7 +196,7 @@ private synchronized ContainerStateChangeReport createContainerStateChangeReport
             for (Map.Entry<ServiceName, Set<ServiceName>> entry : missingDeps.entrySet()) {
                 final ServiceName name = entry.getKey();
                 if (!previousMissing.contains(name)) {
-                    ServiceController<?> controller = serviceRegistry.getService(name);
+                    ServiceController<?> controller = container.getService(name);
                     boolean unavailable = controller != null && controller.getMode() != ServiceController.Mode.NEVER;
                     missingServices.put(name, new MissingDependencyInfo(name, unavailable, entry.getValue()));
                 }

controller/src/main/java/org/jboss/as/controller/ModelControllerImpl.java

@@ -190,7 +190,6 @@ class ModelControllerImpl implements ModelController {
         this.prepareStep = prepareStep == null ? new DefaultPrepareStepHandler() : prepareStep;
         assert processState != null;
         this.processState = processState;
-        this.serviceTarget.addMonitor(stateMonitor.getStabilityMonitor());
         this.executorService = executorService;
         assert expressionResolver != null;
         this.expressionResolver = expressionResolver;
@@ -867,9 +866,9 @@ void logContainerStateChangesAndReset() {
      *
      * @param timeout maximum period to wait for service container stability
      * @param timeUnit unit in which {@code timeout} is expressed
-     * @param interruptibly {@code true} if thread interruption should be ignored
+     * @param interruptibly {@code false} if thread interruption should be ignored
      *
-     * @throws java.lang.InterruptedException if {@code interruptibly} is {@code false} and the thread is interrupted while awaiting service container stability
+     * @throws java.lang.InterruptedException if {@code interruptibly} is {@code true} and the thread is interrupted while awaiting service container stability
      * @throws java.util.concurrent.TimeoutException if service container stability is not reached before the specified timeout
      */
     void awaitContainerStability(long timeout, TimeUnit timeUnit, final boolean interruptibly)

controller/src/main/java/org/jboss/as/controller/OperationContextImpl.java

@@ -846,7 +846,7 @@ private void ensureWriteLockForRuntime() {
                 ExecutionStatus origStatus = executionStatus;
                 try {
                     executionStatus = ExecutionStatus.AWAITING_STABILITY;
-                    modelController.awaitContainerStability(timeout, TimeUnit.MILLISECONDS, respectInterruption);
+                    modelController.awaitContainerStability(timeout, TimeUnit.MILLISECONDS, false); // interruption forbidden - see WFCORE-6158
                     notifyModificationBegun();
                 } catch (InterruptedException e) {
                     if (resultAction != ResultAction.ROLLBACK) {
@@ -1219,7 +1219,7 @@ void releaseStepLocks(AbstractOperationContext.Step step) {
                 // is going to have to await the monitor uninterruptibly anyway before proceeding.
                 long timeout = getBlockingTimeout().getLocalBlockingTimeout();
                 try {
-                    modelController.awaitContainerStability(timeout, TimeUnit.MILLISECONDS, true);
+                    modelController.awaitContainerStability(timeout, TimeUnit.MILLISECONDS, false); // interruption forbidden - see WFCORE-6158
                 }  catch (InterruptedException e) {
                     // Cancelled in some way
                     interrupted = true;

controller/src/test/java/org/jboss/as/controller/OperationCancellationUnitTestCase.java

@@ -66,13 +66,15 @@
 import org.jboss.msc.service.StopContext;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Ignore;
 import org.junit.Test;
 
 /**
  * Unit tests of {@link ModelControllerImpl}.
  *
  * @author Brian Stansberry (c) 2011 Red Hat Inc.
  */
+@Ignore("WFCORE-6158")
 public class OperationCancellationUnitTestCase {
 
     private static final Executor executor = Executors.newCachedThreadPool();

host-controller/src/main/java/org/jboss/as/host/controller/DomainModelControllerService.java

@@ -303,7 +303,7 @@ ignoredRegistry, bootstrapListener, pathManager, expressionResolver, new DomainD
         sb.install();
 
         ExternalManagementRequestExecutor.install(serviceTarget, threadGroup,
-                EXECUTOR_CAPABILITY.getCapabilityServiceName(), service.getStabilityMonitor());
+                EXECUTOR_CAPABILITY.getCapabilityServiceName());
     }
 
     private DomainModelControllerService(final Supplier<ExecutorService> executorService,

server/src/main/java/org/jboss/as/server/ExternalManagementRequestExecutor.java

@@ -32,7 +32,6 @@
 import org.jboss.msc.service.ServiceController;
 import org.jboss.msc.service.ServiceName;
 import org.jboss.msc.service.ServiceTarget;
-import org.jboss.msc.service.StabilityMonitor;
 import org.jboss.msc.service.StartContext;
 import org.jboss.msc.service.StartException;
 import org.jboss.msc.service.StopContext;
@@ -88,13 +87,11 @@ private static int getPoolSize() {
     private ExecutorService executorService;
 
     @SuppressWarnings("deprecation")
-    public static void install(ServiceTarget target, ThreadGroup threadGroup, ServiceName cleanupExecutor,
-                               StabilityMonitor monitor) {
+    public static void install(ServiceTarget target, ThreadGroup threadGroup, ServiceName cleanupExecutor) {
         final ExternalManagementRequestExecutor service = new ExternalManagementRequestExecutor(threadGroup);
         ServiceController<?> controller = target.addService(SERVICE_NAME, service)
                 .addDependency(cleanupExecutor, ExecutorService.class, service.injectedExecutor)
                 .setInitialMode(ServiceController.Mode.ON_DEMAND).install();
-        monitor.addController(controller);
     }
 
     private ExternalManagementRequestExecutor(ThreadGroup threadGroup) {

server/src/main/java/org/jboss/as/server/ServerService.java

@@ -273,8 +273,7 @@ public ThreadFactory run() {
 
         serviceBuilder.install();
 
-        ExternalManagementRequestExecutor.install(serviceTarget, threadGroup, EXECUTOR_CAPABILITY.getCapabilityServiceName(), service.getStabilityMonitor());
-
+        ExternalManagementRequestExecutor.install(serviceTarget, threadGroup, EXECUTOR_CAPABILITY.getCapabilityServiceName());
     }
 
     public synchronized void start(final StartContext context) throws StartException {

subsystem-test/tests/src/test/java/org/jboss/as/subsystem/test/otherservices/OtherServicesSubsystemTestCase.java

@@ -88,7 +88,7 @@ public void testOtherService() throws Exception {
 
         Assert.assertNotNull(services.getContainer().getService(OtherService.NAME));
         Assert.assertNotNull(services.getContainer().getService(MyService.NAME));
-        MyService myService = (MyService)services.getContainer().getService(MyService.NAME).awaitValue();
+        MyService myService = (MyService)services.getContainer().getService(MyService.NAME).getValue();
         Assert.assertNotNull(myService.otherValue.getValue());
     }
 
@@ -144,7 +144,7 @@ public void testPortOffsetZero() throws Exception {
 
         ServiceController<?> controller = services.getContainer().getService(SocketBindingUserService.NAME);
         Assert.assertNotNull(controller);
-        SocketBindingUserService service = (SocketBindingUserService)controller.awaitValue();
+        SocketBindingUserService service = (SocketBindingUserService)controller.getValue();
         SocketBinding socketBinding = service.socketBindingValue.getValue();
         Assert.assertEquals(8000, socketBinding.getSocketBindings().getPortOffset());
         Assert.assertFalse("fixed port", socketBinding.isFixedPort());
@@ -189,7 +189,7 @@ public void testSocketBinding() throws Exception {
 
         ServiceController<?> controller = services.getContainer().getService(SocketBindingUserService.NAME);
         Assert.assertNotNull(controller);
-        SocketBindingUserService service = (SocketBindingUserService)controller.awaitValue();
+        SocketBindingUserService service = (SocketBindingUserService)controller.getValue();
         SocketBinding socketBinding = service.socketBindingValue.getValue();
         Assert.assertEquals(234, socketBinding.getPort());
         Assert.assertEquals("127.0.0.1", socketBinding.getAddress().getHostAddress());
@@ -226,7 +226,7 @@ public void testPath() throws Exception {
 
         ServiceController<?> controller = services.getContainer().getService(PathUserService.NAME);
         Assert.assertNotNull(controller);
-        PathUserService service = (PathUserService)controller.awaitValue();
+        PathUserService service = (PathUserService)controller.getValue();
         Assert.assertEquals(new File(".", "target").getAbsolutePath(), service.pathValue.getValue());
     }