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 b241b08c9d Further locking harmonizations for main components b241b08c9d is described below commit b241b08c9d5b92a183eb686886e8c5bbed9b9835 Author: remm <r...@apache.org> AuthorDate: Thu Apr 4 14:43:02 2024 +0200 Further locking harmonizations for main components Also for lifecycle operations. --- java/org/apache/catalina/core/ContainerBase.java | 86 +++++++++++++--------- java/org/apache/catalina/core/StandardService.java | 22 +++--- webapps/docs/changelog.xml | 7 +- 3 files changed, 67 insertions(+), 48 deletions(-) diff --git a/java/org/apache/catalina/core/ContainerBase.java b/java/org/apache/catalina/core/ContainerBase.java index c15298a359..fd295cb52f 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(); /** @@ -665,26 +666,30 @@ 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); - } - fireContainerEvent(ADD_CHILD_EVENT, child); + fireContainerEvent(ADD_CHILD_EVENT, child); - // Start child - // Don't do this inside sync block - start can be a slow process and - // locking the children object can cause problems elsewhere - try { - if ((getState().isAvailable() || LifecycleState.STARTING_PREP.equals(getState())) && startChildren) { - child.start(); + // Start child + // Don't do this inside sync block - start can be a slow process and + // locking the children object can cause problems elsewhere + try { + if ((getState().isAvailable() || LifecycleState.STARTING_PREP.equals(getState())) && startChildren) { + child.start(); + } + } catch (LifecycleException e) { + throw new IllegalStateException(sm.getString("containerBase.child.start"), e); } - } catch (LifecycleException e) { - throw new IllegalStateException(sm.getString("containerBase.child.start"), e); + } finally { + childrenLock.writeLock().unlock(); } + } @@ -721,8 +726,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 +741,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(); } } @@ -761,36 +772,40 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai return; } + childrenLock.writeLock().lock(); try { - if (child.getState().isAvailable()) { - child.stop(); + try { + if (child.getState().isAvailable()) { + child.stop(); + } + } catch (LifecycleException e) { + log.error(sm.getString("containerBase.child.stop"), e); } - } catch (LifecycleException e) { - log.error(sm.getString("containerBase.child.stop"), e); - } - boolean destroy = false; - try { - // child.destroy() may have already been called which would have - // triggered this call. If that is the case, no need to destroy the - // child again. - if (!LifecycleState.DESTROYING.equals(child.getState())) { - child.destroy(); - destroy = true; + boolean destroy = false; + try { + // child.destroy() may have already been called which would have + // triggered this call. If that is the case, no need to destroy the + // child again. + if (!LifecycleState.DESTROYING.equals(child.getState())) { + child.destroy(); + destroy = true; + } + } catch (LifecycleException e) { + log.error(sm.getString("containerBase.child.destroy"), e); } - } catch (LifecycleException e) { - log.error(sm.getString("containerBase.child.destroy"), e); - } - if (!destroy) { - fireContainerEvent(REMOVE_CHILD_EVENT, child); - } + if (!destroy) { + fireContainerEvent(REMOVE_CHILD_EVENT, child); + } - synchronized (children) { if (children.get(child.getName()) == null) { return; } children.remove(child.getName()); + } finally { + childrenLock.writeLock().unlock(); + } } @@ -1199,13 +1214,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/StandardService.java b/java/org/apache/catalina/core/StandardService.java index dbb32e55ce..63eb38e57f 100644 --- a/java/org/apache/catalina/core/StandardService.java +++ b/java/org/apache/catalina/core/StandardService.java @@ -230,20 +230,20 @@ public class StandardService extends LifecycleMBeanBase implements Service { System.arraycopy(connectors, 0, results, 0, connectors.length); results[connectors.length] = connector; connectors = results; + try { + if (getState().isAvailable()) { + connector.start(); + } + } catch (LifecycleException e) { + throw new IllegalArgumentException(sm.getString("standardService.connector.startFailed", connector), e); + } } finally { writeLock.unlock(); } - try { - if (getState().isAvailable()) { - connector.start(); - } - } catch (LifecycleException e) { - throw new IllegalArgumentException(sm.getString("standardService.connector.startFailed", connector), e); - } - // Report this property change to interested listeners support.firePropertyChange("connector", null, connector); + } @@ -326,13 +326,13 @@ public class StandardService extends LifecycleMBeanBase implements Service { } } connectors = results; - - // Report this property change to interested listeners - support.firePropertyChange("connector", connector, null); } finally { writeLock.unlock(); } + // Report this property change to interested listeners + support.firePropertyChange("connector", connector, null); + } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 5b5c085d36..fd89e5dc7a 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -112,10 +112,11 @@ 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 and Container children access sync using a + ReentrantReadWriteLock. Harmonize locking for their lifecycle + operations after add and remove. (remm) </fix> </changelog> </subsection> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org