This is an automated email from the ASF dual-hosted git repository. remm pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push: new fba822229a Further locking harmonizations for main components fba822229a is described below commit fba822229a6f05223768e7d7026cd886d7f8356d 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 | 5 +- 3 files changed, 66 insertions(+), 47 deletions(-) diff --git a/java/org/apache/catalina/core/ContainerBase.java b/java/org/apache/catalina/core/ContainerBase.java index 4edd4edf9d..846f4e27d8 100644 --- a/java/org/apache/catalina/core/ContainerBase.java +++ b/java/org/apache/catalina/core/ContainerBase.java @@ -129,6 +129,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(); /** @@ -632,26 +633,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(); } + } @@ -688,8 +693,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(); } } @@ -700,8 +708,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(); } } @@ -728,36 +739,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(); + } } @@ -1166,13 +1181,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 368ccfa673..0727329c3f 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -139,8 +139,9 @@ Markus Wolfe. (schultz) </fix> <fix> - Improve Service connectors access sync using a ReentrantReadWriteLock. - (remm) + 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