[WFCORE-7278] Only send CancelAsyncRequest or report its failure if the ActiveOperation isn't complete

Author: bstansberry

controller-client/src/main/java/org/jboss/as/controller/client/impl/AbstractModelControllerClient.java

@@ -24,6 +24,7 @@
 import org.jboss.as.controller.client.OperationBuilder;
 import org.jboss.as.controller.client.OperationMessageHandler;
 import org.jboss.as.controller.client.OperationResponse;
+import org.jboss.as.controller.client.logging.ControllerClientLogger;
 import org.jboss.as.protocol.StreamUtils;
 import org.jboss.as.protocol.mgmt.AbstractManagementRequest;
 import org.jboss.as.protocol.mgmt.ActiveOperation;
@@ -37,6 +38,7 @@
 import org.jboss.as.protocol.mgmt.ManagementRequestHeader;
 import org.jboss.as.protocol.mgmt.ManagementResponseHeader;
 import org.jboss.dmr.ModelNode;
+import org.jboss.threads.AsyncFuture;
 
 
 /**
@@ -264,11 +266,27 @@ <T> CompletableFuture<T> executeRequest(final ManagementRequest<OperationRespons
     }
 
     private void executeCancelAsyncRequest(ActiveOperation<?, ?> activeOperation) {
-        try {
-            getChannelAssociation().executeRequest(activeOperation.getOperationId(), new CancelAsyncRequest());
-        } catch (Exception e) {
-            throw new RuntimeException(e);
-        }
+        if (activeOperation.getResult().getStatus() == AsyncFuture.Status.WAITING) {
+            // Tell the remote side to cancel
+            Integer operationId = activeOperation.getOperationId();
+            try {
+                getChannelAssociation().executeRequest(operationId, new CancelAsyncRequest());
+            } catch (Exception e) {
+                AsyncFuture.Status status = activeOperation.getResult().getStatus();
+                if (status == AsyncFuture.Status.WAITING) {
+                    if (e instanceof RuntimeException) {
+                        throw (RuntimeException) e;
+                    } else {
+                        throw new RuntimeException(e);
+                    }
+                } else {
+                    // Most likely there was a race between our sending CancelAsyncRequest and the operation finishing.
+                    // The operation is done though, so just debug log the failure and move on.
+                    ControllerClientLogger.ROOT_LOGGER.debugf(e, "Executing CancelAsyncRequest for operation %s " +
+                            "failed but the operation reached status %s, so the failure is being ignored", operationId, status);
+                }
+            }
+        } // else the ActiveOperation is already done and there's nothing to cancel
     }
 
     static class OperationExecutionContext implements ActiveOperation.CompletedCallback<OperationResponse> {

protocol/src/main/java/org/jboss/as/protocol/mgmt/AsyncToCompletableFutureAdapter.java

@@ -11,6 +11,7 @@
 import java.util.function.Consumer;
 import java.util.function.Function;
 
+import org.jboss.as.protocol.logging.ProtocolLogger;
 import org.jboss.threads.AsyncFuture;
 
 /**
@@ -116,34 +117,39 @@ public boolean cancel(boolean mayInterruptIfRunning) {
                 asyncCancelConsumer.accept(mayInterruptIfRunning);
                 boolean afCancelled = asyncFuture.awaitUninterruptibly() == AsyncFuture.Status.CANCELLED;
 
-                if (!afCancelled) {
+                if (afCancelled) {
+                    ProtocolLogger.ROOT_LOGGER.tracef("%s: AsyncFuture is cancelled; proceeding to cancel ourself",
+                            getClass().getEnclosingClass().getSimpleName(), getClass().getSimpleName());
+                } else {
                     // Our AsyncFuture.Listener completes this future asynchronously,
                     // so block until it has done so before providing the resulting
                     // state by calling super.cancel.
                     //
                     // We do this because canceling the async future may result in
-                    // the canceled management op returning with either outcome=cancelled
-                    // or outcome=failed. We're in this block, so we know it's the latter.
+                    // the canceled management op returning with outcome=success,
+                    // outcome=failed or outcome=cancelled.
                     // We know this will result in the listener, in another
-                    // thread, eventually trying to complete this exceptionally.
+                    // thread, eventually trying to complete or cancel this CompletableFuture.
                     // We don't want the determination of whether this method returns
                     // true or false to be decided by a race between our call below to
-                    // super.cancel and the listener thread completing the
-                    // future as failed. So we block to let the listener thread win the race.
+                    // super.cancel and the listener thread completing or cancelling the
+                    // future. So we block to let the listener thread win the race.
                     //
                     // An alternative approach would be to call super.cancel first before
                     // cancelling the asyncFuture. But that could result in inconsistency
                     // between what this future reports vs the 'outcome' value of the
                     // executed op. When ModelControllerClient async methods formerly returned an
                     // AsyncFuture these results were consistent, so we're maintaining
                     // that behavior with the CompletableFuture we now return.
+                    ProtocolLogger.ROOT_LOGGER.tracef("%s.%s: Awaiting AsyncFuture.Listener before proceeding to cancel ourself",
+                            getClass().getEnclosingClass().getSimpleName(), getClass().getSimpleName());
                     awaitLatch();
                 }
                 return super.cancel(mayInterruptIfRunning);
             } finally {
                 // Code hardening:
                 // If for some reason asyncFuture.awaitUninterruptibly() returns CANCELLED
-                // without the listener getting invoked, now that we're cancelled make sure
+                // without the listener getting invoked (yet), now that we're cancelled make sure
                 // the latch is counted down so other calls to cancel don't block.
                 latch.countDown();
             }