Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 4 commits into from
Dec 12, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b264875
[WFCORE-6156] Use ServiceContainer.awaitStability() instead of Stabil…
ropalka Dec 2, 2022
a9bb73d
[WFCORE-6157] Forbid interruption of operation thread awaiting contai…
ropalka Dec 7, 2022
a31e41f
[WFCORE-6158] Add information about what needs to be investigated
ropalka Dec 7, 2022
4240d1e
Revert "[WFCORE-6141] Fixing transient test failures" - it was fixed …
ropalka Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 1 addition & 11 deletions controller/src/main/java/org/jboss/as/controller/AbstractControllerService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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
Expand Down
25 changes: 9 additions & 16 deletions controller/src/main/java/org/jboss/as/controller/ContainerStateMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
}

/**
Expand All @@ -74,10 +71,6 @@ void logContainerStateChangesAndReset() {
}
}

StabilityMonitor getStabilityMonitor() {
return monitor;
}

/**
* Await service container stability ignoring thread interruption.
*
Expand All @@ -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;
Expand All @@ -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();
}
}
Expand All @@ -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();
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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()));
}
Expand Down
5 changes: 2 additions & 3 deletions controller/src/main/java/org/jboss/as/controller/ModelControllerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions controller/src/main/java/org/jboss/as/controller/OperationContextImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you wanted to use see WFCORE-6157, instead of seeing WFCORE-6158. We can correct it after merging, but could you confirm @ropalka ?

notifyModificationBegun();
} catch (InterruptedException e) {
if (resultAction != ResultAction.ROLLBACK) {
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions controller/src/test/java/org/jboss/as/controller/OperationCancellationUnitTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ropalka , would you mind adding a short description to WFCORE-6158? It is unclear your intention with this Jira, I'm going to merge this PR since I understand WFCORE-6158 is in place to just investigate whether this test case must be removed, but I'll leave the Jira unresolved until it is clear what you want to do with this case.

public class OperationCancellationUnitTestCase {

private static final Executor executor = Executors.newCachedThreadPool();
Expand Down
2 changes: 1 addition & 1 deletion host-controller/src/main/java/org/jboss/as/host/controller/DomainModelControllerService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
5 changes: 1 addition & 4 deletions server/src/main/java/org/jboss/as/server/ExternalManagementRequestExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 1 addition & 2 deletions server/src/main/java/org/jboss/as/server/ServerService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions ...c/test/java/org/jboss/as/subsystem/test/otherservices/OtherServicesSubsystemTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

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

Expand Down