From 981c85b2f8e12b78a495cb09aa56fd54c4b7bdb8 Mon Sep 17 00:00:00 2001 From: Brian Stansberry Date: Thu, 15 Jun 2023 13:58:34 -0500 Subject: [PATCH] =?UTF-8?q?Revert=20"Revert=20"[WFCORE-6386]=20Ensure=20th?= =?UTF-8?q?at=20read-only=20op=20handlers=20that=20incorrectly=20do=20w?= =?UTF-8?q?=E2=80=A6""?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../controller/AbstractOperationContext.java | 57 ++++++++++++++++--- .../as/controller/OperationContextImpl.java | 18 +++--- .../ParallelBootOperationContext.java | 7 +-- .../jboss/as/controller/ReadOnlyContext.java | 7 +-- 4 files changed, 63 insertions(+), 26 deletions(-) diff --git a/controller/src/main/java/org/jboss/as/controller/AbstractOperationContext.java b/controller/src/main/java/org/jboss/as/controller/AbstractOperationContext.java index 3f3842f2112..dee0ea4e962 100644 --- a/controller/src/main/java/org/jboss/as/controller/AbstractOperationContext.java +++ b/controller/src/main/java/org/jboss/as/controller/AbstractOperationContext.java @@ -162,6 +162,8 @@ abstract class AbstractOperationContext implements OperationContext, AutoCloseab boolean cancelled; /** Currently executing step */ Step activeStep; + /** The step that acquired the write lock */ + Step lockStep; private final Supplier securityIdentitySupplier; /** Whether operation execution has begun; i.e. whether completeStep() has been called */ private boolean executing; @@ -620,7 +622,7 @@ boolean stageCompleted(Stage stage) { * * @param step the step */ - abstract void releaseStepLocks(Step step); + abstract void releaseStepLocks(AbstractStep step); /** * Wait for completion of removal of any services removed by this context. @@ -685,6 +687,10 @@ private void recordControllerOperation(ModelNode operation) { controllerOperations.add(operation.clone()); // clone so we don't log op nodes mutated during execution } + void recordWriteLock() { + this.lockStep = activeStep.recordWriteLock(); + } + void trackConfigurationChange() { if (!isBooting() && !isReadOnly() && configurationChangesCollector.trackAllowed()) { try { @@ -1365,9 +1371,10 @@ private boolean addModelValidationSteps() { return true; } - private abstract class AbstractStep { + abstract class AbstractStep { AbstractStep predecessor; abstract void finalizeStep(); + abstract boolean matches(Step step); final void finalizeRollbackResponse(ModelNode outcome, ModelNode rolledback) { outcome.set(cancelled ? CANCELLED : FAILED); rolledback.set(true); @@ -1401,6 +1408,7 @@ class Step extends AbstractStep { // Whether standard Stage.DONE handling is needed, as opposed to lighter-weight handling. private boolean requiresDoneStage; ManagementResourceRegistration resourceRegistration; + private DoneStagePlaceholder placeholder; private Step(OperationDefinition operationDefinition, final OperationStepHandler handler, final ModelNode response, final ModelNode operation, final PathAddress address) { @@ -1426,6 +1434,20 @@ private Step(OperationDefinition operationDefinition, final OperationStepHandler || !this.operationDefinition.getFlags().contains(OperationEntry.Flag.READ_ONLY); } + boolean matches(Step other) { + return this == other; + } + + Step recordWriteLock() { + recordModification("call to recordWriteLock()"); + return this; + } + + Step modifyServiceContainer() { + recordModification("ServiceContainer modification"); + return this; + } + /** * Gets a {@code ServiceTarget} {@link org.jboss.msc.service.ServiceTarget#subTarget() scoped to this step}, * with a {@link org.jboss.as.controller.ServiceVerificationHelper} registered, so all services @@ -1662,6 +1684,7 @@ private DoneStagePlaceholder(Step replaces) { this.outcome = replaces.response.get(OUTCOME); this.rolledBack = replaces.response.get(ROLLED_BACK); this.failed = replaces.hasFailed(); + replaces.placeholder = this; } @Override @@ -1670,14 +1693,32 @@ void finalizeStep() { // 1) We replaced a step with a no-op result handler, so we don't need to invoke it. // 2) We'll never be the first step finalized, so we don't need to take actions like // changing the Stage to DONE or setting a result action - // 3) We're only used for read-only ops, so we can ignore setting any ROLLED_BACK node. - // 4) We skip some trace logging that would require retaining a lot of state. - if (resultAction == ResultAction.ROLLBACK) { - finalizeRollbackResponse(outcome, rolledBack); - } else { - finalizeNonRollbackResponse(outcome, failed ? rolledBack : null); + // 3) We skip some trace logging that would require retaining a lot of state. + try { + if (resultAction == ResultAction.ROLLBACK) { + finalizeRollbackResponse(outcome, rolledBack); + } else { + finalizeNonRollbackResponse(outcome, failed ? rolledBack : null); + } + } finally { + // It shouldn't be possible that the Step that created us is holding locks, + // as Step.linkNextStep wouldn't have created us if it was used for writes. + // But to be safe, give the context a chance to release locks. + releaseStepLocks(this); + + if (predecessor == null) { + // We're returning from the outermost completeStep() + // Null out the current stage to disallow further access to + // the context + currentStage = null; + } } } + + @Override + boolean matches(Step step) { + return step != null && this == step.placeholder; + } } private static class RollbackDelegatingResultHandler implements ResultHandler { diff --git a/controller/src/main/java/org/jboss/as/controller/OperationContextImpl.java b/controller/src/main/java/org/jboss/as/controller/OperationContextImpl.java index 5bb1ac29fc0..3383c48d6d6 100644 --- a/controller/src/main/java/org/jboss/as/controller/OperationContextImpl.java +++ b/controller/src/main/java/org/jboss/as/controller/OperationContextImpl.java @@ -179,8 +179,6 @@ final class OperationContextImpl extends AbstractOperationContext implements Aut /** Tracks whether any steps have gotten write access to the runtime */ private volatile boolean affectsRuntime; - /** The step that acquired the write lock */ - private Step lockStep; /** The step that acquired the container monitor */ private Step containerMonitorStep; private boolean notifiedModificationBegun; @@ -840,7 +838,7 @@ private void takeWriteLock() { // } // } exclusiveStartTime = System.nanoTime(); - lockStep = activeStep; + recordWriteLock(); } catch (InterruptedException e) { cancelled = true; Thread.currentThread().interrupt(); @@ -859,7 +857,7 @@ private void ensureWriteLockForRuntime() { if (currentStage == Stage.DONE) { throw ControllerLogger.ROOT_LOGGER.invalidModificationAfterCompletedStep(); } - containerMonitorStep = activeStep; + containerMonitorStep = activeStep.modifyServiceContainer(); int timeout = getBlockingTimeout().getLocalBlockingTimeout(); ExecutionStatus origStatus = executionStatus; try { @@ -1222,12 +1220,12 @@ void handleUncaughtException(RuntimeException e) { } @Override - void releaseStepLocks(AbstractOperationContext.Step step) { + void releaseStepLocks(AbstractOperationContext.AbstractStep step) { boolean interrupted = false; try { // Get container stability before releasing controller lock to ensure another // op doesn't get in and destabilize the container. - if (this.containerMonitorStep == step) { + if (step.matches(this.containerMonitorStep)) { // Note: If we allow this thread to be interrupted, an op that has been cancelled // because of minor user impatience can release the controller lock while the // container is unsettled. OTOH, if we don't allow interruption, if the @@ -1249,7 +1247,7 @@ void releaseStepLocks(AbstractOperationContext.Step step) { // We can no longer have any sense of MSC state or how the model relates to the runtime and // we need to start from a fresh service container. Notify the controller of this. modelController.containerCannotStabilize(); - MGMT_OP_LOGGER.interruptedWaitingStability(activeStep.operationId.name, activeStep.operationId.address); + MGMT_OP_LOGGER.interruptedWaitingStability(containerMonitorStep.operationId.name, containerMonitorStep.operationId.address); } catch (TimeoutException te) { // If we can't attain stability on the way out after rollback ops have run, // we can no longer have any sense of MSC state or how the model relates to the runtime and @@ -1258,18 +1256,18 @@ void releaseStepLocks(AbstractOperationContext.Step step) { // Just log; this doesn't change the result of the op. And if we're not stable here // it's almost certain we never stabilized during execution or we are rolling back and destabilized there. // Either one means there is already a failure message associated with this op. - MGMT_OP_LOGGER.timeoutCompletingOperation(timeout / 1000, activeStep.operationId.name, activeStep.operationId.address); + MGMT_OP_LOGGER.timeoutCompletingOperation(timeout / 1000, containerMonitorStep.operationId.name, containerMonitorStep.operationId.address); // Produce and log thread dump for diagnostics ThreadDumpUtil.threadDump(); } } - if (this.lockStep == step) { + if (step.matches(this.lockStep)) { releaseModelControllerLock(); } } finally { try { - if (this.containerMonitorStep == step) { + if (step.matches(this.containerMonitorStep)) { notifyModificationsComplete(); resetContainerStateChanges(); } diff --git a/controller/src/main/java/org/jboss/as/controller/ParallelBootOperationContext.java b/controller/src/main/java/org/jboss/as/controller/ParallelBootOperationContext.java index 6e33edf3c1a..db21553ecfd 100644 --- a/controller/src/main/java/org/jboss/as/controller/ParallelBootOperationContext.java +++ b/controller/src/main/java/org/jboss/as/controller/ParallelBootOperationContext.java @@ -58,7 +58,6 @@ class ParallelBootOperationContext extends AbstractOperationContext { private final OperationContextImpl primaryContext; private final List runtimeOps; private final Thread controllingThread; - private Step lockStep; private final int operationId; private final ModelControllerImpl controller; @@ -215,7 +214,7 @@ public void acquireControllerLock() { if(lockStep == null) { try { controller.acquireWriteLock(operationId, true); - lockStep = activeStep; + recordWriteLock(); } catch (InterruptedException e) { cancelled = true; Thread.currentThread().interrupt(); @@ -423,8 +422,8 @@ public CapabilityServiceSupport getCapabilityServiceSupport() { } @Override - void releaseStepLocks(Step step) { - if(lockStep == step) { + void releaseStepLocks(AbstractStep step) { + if(step.matches(lockStep)) { controller.releaseWriteLock(operationId); lockStep = null; } diff --git a/controller/src/main/java/org/jboss/as/controller/ReadOnlyContext.java b/controller/src/main/java/org/jboss/as/controller/ReadOnlyContext.java index c853ee38a17..f3ba91bb871 100644 --- a/controller/src/main/java/org/jboss/as/controller/ReadOnlyContext.java +++ b/controller/src/main/java/org/jboss/as/controller/ReadOnlyContext.java @@ -63,7 +63,6 @@ class ReadOnlyContext extends AbstractOperationContext { private final ModelControllerImpl controller; private final AbstractOperationContext primaryContext; private final ModelControllerImpl.ManagementModelImpl managementModel; - private Step lockStep; private final ConcurrentMap, Object> valueAttachments = new ConcurrentHashMap, Object>(); @@ -203,7 +202,7 @@ public void acquireControllerLock() { if (lockStep == null) { try { controller.acquireWriteLock(operationId, true); - lockStep = activeStep; + recordWriteLock(); } catch (InterruptedException e) { cancelled = true; Thread.currentThread().interrupt(); @@ -213,8 +212,8 @@ public void acquireControllerLock() { } @Override - void releaseStepLocks(Step step) { - if (step == lockStep) { + void releaseStepLocks(AbstractStep step) { + if (step.matches(lockStep)) { lockStep = null; controller.releaseWriteLock(operationId); }