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 cf05b38e5e Revert "Further locking harmonizations for main components" cf05b38e5e is described below commit cf05b38e5ede77bc46ca02128b8a1fe0d0168494 Author: remm <r...@apache.org> AuthorDate: Thu Apr 4 16:02:45 2024 +0200 Revert "Further locking harmonizations for main components" This reverts commit fba822229a6f05223768e7d7026cd886d7f8356d. --- java/org/apache/catalina/core/ContainerBase.java | 86 +++++++++------------- java/org/apache/catalina/core/StandardService.java | 22 +++--- webapps/docs/changelog.xml | 5 +- 3 files changed, 47 insertions(+), 66 deletions(-) diff --git a/java/org/apache/catalina/core/ContainerBase.java b/java/org/apache/catalina/core/ContainerBase.java index 846f4e27d8..4edd4edf9d 100644 --- a/java/org/apache/catalina/core/ContainerBase.java +++ b/java/org/apache/catalina/core/ContainerBase.java @@ -129,7 +129,6 @@ 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(); /** @@ -633,30 +632,26 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai log.debug(sm.getString("containerBase.child.add", child, this)); } - childrenLock.writeLock().lock(); - try { + synchronized (children) { 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(); - } - } catch (LifecycleException e) { - throw new IllegalStateException(sm.getString("containerBase.child.start"), e); + // 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(); } - } finally { - childrenLock.writeLock().unlock(); + } catch (LifecycleException e) { + throw new IllegalStateException(sm.getString("containerBase.child.start"), e); } - } @@ -693,11 +688,8 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai if (name == null) { return null; } - childrenLock.readLock().lock(); - try { + synchronized (children) { return children.get(name); - } finally { - childrenLock.readLock().unlock(); } } @@ -708,11 +700,8 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai */ @Override public Container[] findChildren() { - childrenLock.readLock().lock(); - try { + synchronized (children) { return children.values().toArray(new Container[0]); - } finally { - childrenLock.readLock().unlock(); } } @@ -739,40 +728,36 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai return; } - childrenLock.writeLock().lock(); try { - try { - if (child.getState().isAvailable()) { - child.stop(); - } - } catch (LifecycleException e) { - log.error(sm.getString("containerBase.child.stop"), e); + if (child.getState().isAvailable()) { + child.stop(); } + } 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; - } - } catch (LifecycleException e) { - log.error(sm.getString("containerBase.child.destroy"), 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; } + } 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(); - } } @@ -1181,16 +1166,13 @@ public abstract class ContainerBase extends LifecycleMBeanBase implements Contai public ObjectName[] getChildren() { List<ObjectName> names; - childrenLock.readLock().lock(); - try { + synchronized (children) { 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 63eb38e57f..dbb32e55ce 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 0727329c3f..368ccfa673 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -139,9 +139,8 @@ Markus Wolfe. (schultz) </fix> <fix> - Improve Service connectors and Container children access sync using a - ReentrantReadWriteLock. Harmonize locking for their lifecycle - operations after add and remove. (remm) + Improve Service connectors 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