Skip to content

Commit

Permalink
Revert "Revert "[WFCORE-6386] Ensure that read-only op handlers that …
Browse files Browse the repository at this point in the history 
…incorrectly do w…""
  • Loading branch information
@bstansberry
bstansberry authored Jun 15, 2023
1 parent adb9aa0 commit 981c85b
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 26 deletions.
57 changes: 49 additions & 8 deletions controller/src/main/java/org/jboss/as/controller/AbstractOperationContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<SecurityIdentity> securityIdentitySupplier;
/** Whether operation execution has begun; i.e. whether completeStep() has been called */
private boolean executing;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down
18 changes: 8 additions & 10 deletions controller/src/main/java/org/jboss/as/controller/OperationContextImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -840,7 +838,7 @@ private void takeWriteLock() {
// }
// }
exclusiveStartTime = System.nanoTime();
lockStep = activeStep;
recordWriteLock();
} catch (InterruptedException e) {
cancelled = true;
Thread.currentThread().interrupt();
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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();
}
Expand Down
7 changes: 3 additions & 4 deletions controller/src/main/java/org/jboss/as/controller/ParallelBootOperationContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ class ParallelBootOperationContext extends AbstractOperationContext {
private final OperationContextImpl primaryContext;
private final List<ParsedBootOp> runtimeOps;
private final Thread controllingThread;
private Step lockStep;
private final int operationId;
private final ModelControllerImpl controller;

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
Expand Down
7 changes: 3 additions & 4 deletions controller/src/main/java/org/jboss/as/controller/ReadOnlyContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<AttachmentKey<?>, Object> valueAttachments = new ConcurrentHashMap<AttachmentKey<?>, Object>();

Expand Down Expand Up @@ -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();
Expand All @@ -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);
}
Expand Down

0 comments on commit 981c85b

Please sign in to comment.