You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2024/04/05 12:17:16 UTC
(tomcat) branch 10.1.x updated: Harmonize syncs for some components
This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 6bcd35e129 Harmonize syncs for some components
6bcd35e129 is described below
commit 6bcd35e129ef3e1d17df44fb3444d5481a73115f
Author: remm <re...@apache.org>
AuthorDate: Fri Apr 5 14:08:16 2024 +0200
Harmonize syncs for some components
Take lifecycle operations (which have their own sync) and notifications
out of sync.
---
java/org/apache/catalina/core/ContainerBase.java | 105 ++++++++++++---------
java/org/apache/catalina/core/StandardContext.java | 79 ++++++++--------
java/org/apache/catalina/core/StandardServer.java | 80 ++++++----------
java/org/apache/catalina/core/StandardService.java | 86 +++++++++--------
webapps/docs/changelog.xml | 6 +-
5 files changed, 177 insertions(+), 179 deletions(-)
diff --git a/java/org/apache/catalina/core/ContainerBase.java b/java/org/apache/catalina/core/ContainerBase.java
index c15298a359..43201c19ed 100644
--- a/java/org/apache/catalina/core/ContainerBase.java
+++ b/java/org/apache/catalina/core/ContainerBase.java
@@ -152,6 +152,7 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai
* The child Containers belonging to this Container, keyed by name.
*/
protected final HashMap<String,Container> children = new HashMap<>();
+ private final ReadWriteLock childrenLock = new ReentrantReadWriteLock();
/**
@@ -398,32 +399,31 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai
return;
}
this.cluster = cluster;
-
- // Stop the old component if necessary
- if (getState().isAvailable() && (oldCluster instanceof Lifecycle)) {
- try {
- ((Lifecycle) oldCluster).stop();
- } catch (LifecycleException e) {
- log.error(sm.getString("containerBase.cluster.stop"), e);
- }
- }
-
// Start the new component if necessary
if (cluster != null) {
cluster.setContainer(this);
}
-
- if (getState().isAvailable() && (cluster instanceof Lifecycle)) {
- try {
- ((Lifecycle) cluster).start();
- } catch (LifecycleException e) {
- log.error(sm.getString("containerBase.cluster.start"), e);
- }
- }
} finally {
writeLock.unlock();
}
+ // Stop the old component if necessary
+ if (getState().isAvailable() && (oldCluster instanceof Lifecycle)) {
+ try {
+ ((Lifecycle) oldCluster).stop();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("containerBase.cluster.stop"), e);
+ }
+ }
+
+ if (getState().isAvailable() && (cluster instanceof Lifecycle)) {
+ try {
+ ((Lifecycle) cluster).start();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("containerBase.cluster.start"), e);
+ }
+ }
+
// Report this property change to interested listeners
support.firePropertyChange("cluster", oldCluster, cluster);
}
@@ -592,43 +592,44 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai
@Override
public void setRealm(Realm realm) {
+ Realm oldRealm = null;
Lock l = realmLock.writeLock();
l.lock();
try {
// Change components if necessary
- Realm oldRealm = this.realm;
+ oldRealm = this.realm;
if (oldRealm == realm) {
return;
}
this.realm = realm;
- // Stop the old component if necessary
- if (getState().isAvailable() && (oldRealm instanceof Lifecycle)) {
- try {
- ((Lifecycle) oldRealm).stop();
- } catch (LifecycleException e) {
- log.error(sm.getString("containerBase.realm.stop"), e);
- }
- }
-
// Start the new component if necessary
if (realm != null) {
realm.setContainer(this);
}
- if (getState().isAvailable() && (realm instanceof Lifecycle)) {
- try {
- ((Lifecycle) realm).start();
- } catch (LifecycleException e) {
- log.error(sm.getString("containerBase.realm.start"), e);
- }
- }
-
- // Report this property change to interested listeners
- support.firePropertyChange("realm", oldRealm, this.realm);
} finally {
l.unlock();
}
+ // Stop the old component if necessary
+ if (getState().isAvailable() && oldRealm instanceof Lifecycle) {
+ try {
+ ((Lifecycle) oldRealm).stop();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("containerBase.realm.stop"), e);
+ }
+ }
+
+ if (getState().isAvailable() && realm instanceof Lifecycle) {
+ try {
+ ((Lifecycle) realm).start();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("containerBase.realm.start"), e);
+ }
+ }
+
+ // Report this property change to interested listeners
+ support.firePropertyChange("realm", oldRealm, this.realm);
}
@@ -665,12 +666,15 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai
log.debug(sm.getString("containerBase.child.add", child, this));
}
- synchronized (children) {
+ childrenLock.writeLock().lock();
+ try {
if (children.get(child.getName()) != null) {
throw new IllegalArgumentException(sm.getString("containerBase.child.notUnique", child.getName()));
}
child.setParent(this); // May throw IAE
children.put(child.getName(), child);
+ } finally {
+ childrenLock.writeLock().unlock();
}
fireContainerEvent(ADD_CHILD_EVENT, child);
@@ -721,8 +725,11 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai
if (name == null) {
return null;
}
- synchronized (children) {
+ childrenLock.readLock().lock();
+ try {
return children.get(name);
+ } finally {
+ childrenLock.readLock().unlock();
}
}
@@ -733,8 +740,11 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai
*/
@Override
public Container[] findChildren() {
- synchronized (children) {
+ childrenLock.readLock().lock();
+ try {
return children.values().toArray(new Container[0]);
+ } finally {
+ childrenLock.readLock().unlock();
}
}
@@ -786,11 +796,11 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai
fireContainerEvent(REMOVE_CHILD_EVENT, child);
}
- synchronized (children) {
- if (children.get(child.getName()) == null) {
- return;
- }
+ childrenLock.writeLock().lock();
+ try {
children.remove(child.getName());
+ } finally {
+ childrenLock.writeLock().unlock();
}
}
@@ -1199,13 +1209,16 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai
public ObjectName[] getChildren() {
List<ObjectName> names;
- synchronized (children) {
+ childrenLock.readLock().lock();
+ try {
names = new ArrayList<>(children.size());
for (Container next : children.values()) {
if (next instanceof ContainerBase) {
names.add(next.getObjectName());
}
}
+ } finally {
+ childrenLock.readLock().unlock();
}
return names.toArray(new ObjectName[0]);
}
diff --git a/java/org/apache/catalina/core/StandardContext.java b/java/org/apache/catalina/core/StandardContext.java
index 4f13b69a3f..3b88a43b46 100644
--- a/java/org/apache/catalina/core/StandardContext.java
+++ b/java/org/apache/catalina/core/StandardContext.java
@@ -1860,31 +1860,31 @@ public class StandardContext extends ContainerBase implements Context, Notificat
return;
}
this.loader = loader;
-
- // Stop the old component if necessary
- if (getState().isAvailable() && (oldLoader != null) && (oldLoader instanceof Lifecycle)) {
- try {
- ((Lifecycle) oldLoader).stop();
- } catch (LifecycleException e) {
- log.error(sm.getString("standardContext.setLoader.stop"), e);
- }
- }
-
// Start the new component if necessary
if (loader != null) {
loader.setContext(this);
}
- if (getState().isAvailable() && (loader != null) && (loader instanceof Lifecycle)) {
- try {
- ((Lifecycle) loader).start();
- } catch (LifecycleException e) {
- log.error(sm.getString("standardContext.setLoader.start"), e);
- }
- }
} finally {
writeLock.unlock();
}
+ // Stop the old component if necessary
+ if (getState().isAvailable() && oldLoader instanceof Lifecycle) {
+ try {
+ ((Lifecycle) oldLoader).stop();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("standardContext.setLoader.stop"), e);
+ }
+ }
+
+ if (getState().isAvailable() && loader instanceof Lifecycle) {
+ try {
+ ((Lifecycle) loader).start();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("standardContext.setLoader.start"), e);
+ }
+ }
+
// Report this property change to interested listeners
support.firePropertyChange("loader", oldLoader, loader);
}
@@ -1915,32 +1915,32 @@ public class StandardContext extends ContainerBase implements Context, Notificat
return;
}
this.manager = manager;
-
- // Stop the old component if necessary
- if (oldManager instanceof Lifecycle) {
- try {
- ((Lifecycle) oldManager).stop();
- ((Lifecycle) oldManager).destroy();
- } catch (LifecycleException e) {
- log.error(sm.getString("standardContext.setManager.stop"), e);
- }
- }
-
// Start the new component if necessary
if (manager != null) {
manager.setContext(this);
}
- if (getState().isAvailable() && manager instanceof Lifecycle) {
- try {
- ((Lifecycle) manager).start();
- } catch (LifecycleException e) {
- log.error(sm.getString("standardContext.setManager.start"), e);
- }
- }
} finally {
writeLock.unlock();
}
+ // Stop the old component if necessary
+ if (oldManager instanceof Lifecycle) {
+ try {
+ ((Lifecycle) oldManager).stop();
+ ((Lifecycle) oldManager).destroy();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("standardContext.setManager.stop"), e);
+ }
+ }
+
+ if (getState().isAvailable() && manager instanceof Lifecycle) {
+ try {
+ ((Lifecycle) manager).start();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("standardContext.setManager.start"), e);
+ }
+ }
+
// Report this property change to interested listeners
support.firePropertyChange("manager", oldManager, manager);
}
@@ -2478,11 +2478,12 @@ public class StandardContext extends ContainerBase implements Context, Notificat
if (resources != null) {
resources.setContext(this);
}
-
- support.firePropertyChange("resources", oldResources, resources);
} finally {
writeLock.unlock();
}
+
+ support.firePropertyChange("resources", oldResources, resources);
+
}
@@ -4584,8 +4585,6 @@ public class StandardContext extends ContainerBase implements Context, Notificat
boolean ok = true;
- Lock writeLock = resourcesLock.writeLock();
- writeLock.lock();
try {
if (resources != null) {
resources.stop();
@@ -4594,8 +4593,6 @@ public class StandardContext extends ContainerBase implements Context, Notificat
ExceptionUtils.handleThrowable(t);
log.error(sm.getString("standardContext.resourcesStop"), t);
ok = false;
- } finally {
- writeLock.unlock();
}
return ok;
diff --git a/java/org/apache/catalina/core/StandardServer.java b/java/org/apache/catalina/core/StandardServer.java
index f77c5b38cd..f84624be74 100644
--- a/java/org/apache/catalina/core/StandardServer.java
+++ b/java/org/apache/catalina/core/StandardServer.java
@@ -511,26 +511,25 @@ public final class StandardServer extends LifecycleMBeanBase implements Server {
service.setServer(this);
servicesWriteLock.lock();
-
try {
Service results[] = new Service[services.length + 1];
System.arraycopy(services, 0, results, 0, services.length);
results[services.length] = service;
services = results;
-
- if (getState().isAvailable()) {
- try {
- service.start();
- } catch (LifecycleException e) {
- // Ignore
- }
- }
-
- // Report this property change to interested listeners
- support.firePropertyChange("service", null, service);
} finally {
servicesWriteLock.unlock();
}
+
+ if (getState().isAvailable()) {
+ try {
+ service.start();
+ } catch (LifecycleException e) {
+ // Ignore
+ }
+ }
+
+ // Report this property change to interested listeners
+ support.firePropertyChange("service", null, service);
}
public void stopAwait() {
@@ -769,11 +768,6 @@ public final class StandardServer extends LifecycleMBeanBase implements Server {
if (j < 0) {
return;
}
- try {
- services[j].stop();
- } catch (LifecycleException e) {
- // Ignore
- }
int k = 0;
Service[] results = new Service[services.length - 1];
for (int i = 0; i < services.length; i++) {
@@ -782,12 +776,18 @@ public final class StandardServer extends LifecycleMBeanBase implements Server {
}
}
services = results;
-
- // Report this property change to interested listeners
- support.firePropertyChange("service", service, null);
} finally {
servicesWriteLock.unlock();
}
+
+ try {
+ service.stop();
+ } catch (LifecycleException e) {
+ // Ignore
+ }
+
+ // Report this property change to interested listeners
+ support.firePropertyChange("service", service, null);
}
@@ -943,14 +943,8 @@ public final class StandardServer extends LifecycleMBeanBase implements Server {
globalNamingResources.start();
// Start our defined Services
- servicesReadLock.lock();
-
- try {
- for (Service service : services) {
- service.start();
- }
- } finally {
- servicesReadLock.unlock();
+ for (Service service : findServices()) {
+ service.start();
}
if (periodicEventDelay > 0) {
@@ -1000,14 +994,8 @@ public final class StandardServer extends LifecycleMBeanBase implements Server {
fireLifecycleEvent(CONFIGURE_STOP_EVENT, null);
// Stop our defined Services
- servicesReadLock.lock();
-
- try {
- for (Service service : services) {
- service.stop();
- }
- } finally {
- servicesReadLock.unlock();
+ for (Service service : findServices()) {
+ service.stop();
}
synchronized (utilityExecutorLock) {
@@ -1047,28 +1035,16 @@ public final class StandardServer extends LifecycleMBeanBase implements Server {
globalNamingResources.init();
// Initialize our defined Services
- servicesReadLock.lock();
-
- try {
- for (Service service : services) {
- service.init();
- }
- } finally {
- servicesReadLock.unlock();
+ for (Service service : findServices()) {
+ service.init();
}
}
@Override
protected void destroyInternal() throws LifecycleException {
// Destroy our defined Services
- servicesReadLock.lock();
-
- try {
- for (Service service : services) {
- service.destroy();
- }
- } finally {
- servicesReadLock.unlock();
+ for (Service service : findServices()) {
+ service.destroy();
}
globalNamingResources.destroy();
diff --git a/java/org/apache/catalina/core/StandardService.java b/java/org/apache/catalina/core/StandardService.java
index dbb32e55ce..461d8aa62b 100644
--- a/java/org/apache/catalina/core/StandardService.java
+++ b/java/org/apache/catalina/core/StandardService.java
@@ -85,6 +85,7 @@ public class StandardService extends LifecycleMBeanBase implements Service {
* The list of executors held by the service.
*/
protected final ArrayList<Executor> executors = new ArrayList<>();
+ private final ReadWriteLock executorsLock = new ReentrantReadWriteLock();
private Engine engine = null;
@@ -310,14 +311,6 @@ public class StandardService extends LifecycleMBeanBase implements Service {
if (j < 0) {
return;
}
- if (connectors[j].getState().isAvailable()) {
- try {
- connectors[j].stop();
- } catch (LifecycleException e) {
- log.error(sm.getString("standardService.connector.stopFailed", connectors[j]), e);
- }
- }
- connector.setService(null);
int k = 0;
Connector results[] = new Connector[connectors.length - 1];
for (int i = 0; i < connectors.length; i++) {
@@ -327,12 +320,21 @@ public class StandardService extends LifecycleMBeanBase implements Service {
}
connectors = results;
- // Report this property change to interested listeners
- support.firePropertyChange("connector", connector, null);
} finally {
writeLock.unlock();
}
+ if (connector.getState().isAvailable()) {
+ try {
+ connector.stop();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("standardService.connector.stopFailed", connector), e);
+ }
+ }
+ connector.setService(null);
+
+ // Report this property change to interested listeners
+ support.firePropertyChange("connector", connector, null);
}
@@ -365,16 +367,21 @@ public class StandardService extends LifecycleMBeanBase implements Service {
*/
@Override
public void addExecutor(Executor ex) {
- synchronized (executors) {
+ boolean added = false;
+ executorsLock.writeLock().lock();
+ try {
if (!executors.contains(ex)) {
+ added = true;
executors.add(ex);
- if (getState().isAvailable()) {
- try {
- ex.start();
- } catch (LifecycleException x) {
- log.error(sm.getString("standardService.executor.start"), x);
- }
- }
+ }
+ } finally {
+ executorsLock.writeLock().unlock();
+ }
+ if (added && getState().isAvailable()) {
+ try {
+ ex.start();
+ } catch (LifecycleException x) {
+ log.error(sm.getString("standardService.executor.start"), x);
}
}
}
@@ -387,8 +394,11 @@ public class StandardService extends LifecycleMBeanBase implements Service {
*/
@Override
public Executor[] findExecutors() {
- synchronized (executors) {
+ executorsLock.readLock().lock();
+ try {
return executors.toArray(new Executor[0]);
+ } finally {
+ executorsLock.readLock().unlock();
}
}
@@ -402,12 +412,15 @@ public class StandardService extends LifecycleMBeanBase implements Service {
*/
@Override
public Executor getExecutor(String executorName) {
- synchronized (executors) {
+ executorsLock.readLock().lock();
+ try {
for (Executor executor : executors) {
if (executorName.equals(executor.getName())) {
return executor;
}
}
+ } finally {
+ executorsLock.readLock().unlock();
}
return null;
}
@@ -420,13 +433,18 @@ public class StandardService extends LifecycleMBeanBase implements Service {
*/
@Override
public void removeExecutor(Executor ex) {
- synchronized (executors) {
- if (executors.remove(ex) && getState().isAvailable()) {
- try {
- ex.stop();
- } catch (LifecycleException e) {
- log.error(sm.getString("standardService.executor.stop"), e);
- }
+ boolean removed = false;
+ executorsLock.writeLock().lock();
+ try {
+ removed = executors.remove(ex);
+ } finally {
+ executorsLock.writeLock().unlock();
+ }
+ if (removed && getState().isAvailable()) {
+ try {
+ ex.stop();
+ } catch (LifecycleException e) {
+ log.error(sm.getString("standardService.executor.stop"), e);
}
}
}
@@ -449,15 +467,11 @@ public class StandardService extends LifecycleMBeanBase implements Service {
// Start our defined Container first
if (engine != null) {
- synchronized (engine) {
- engine.start();
- }
+ engine.start();
}
- synchronized (executors) {
- for (Executor executor : executors) {
- executor.start();
- }
+ for (Executor executor : findExecutors()) {
+ executor.start();
}
mapperListener.start();
@@ -509,9 +523,7 @@ public class StandardService extends LifecycleMBeanBase implements Service {
// Stop our defined Container once the Connectors are all paused
if (engine != null) {
- synchronized (engine) {
- engine.stop();
- }
+ engine.stop();
}
// Now stop the connectors
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 5b5c085d36..ad1562d805 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -112,10 +112,10 @@
from a simple synchronized lock to a ReentrantReadWriteLock to allow
multiple readers to operate simultaneously. Based upon a suggestion by
Markus Wolfe. (schultz)
- <fix>
- Improve Service connectors access sync using a ReentrantReadWriteLock.
- (remm)
</fix>
+ <fix>
+ Improve Service connectors, Container children and Service executors
+ access sync using a ReentrantReadWriteLock. (remm)
</fix>
</changelog>
</subsection>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org