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 <r...@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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org