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 7ee8ca1f0e Revert "Further locking harmonizations for main components" 7ee8ca1f0e is described below commit 7ee8ca1f0ea6a0984aba1ecf34b24f717886f329 Author: remm <r...@apache.org> AuthorDate: Thu Apr 4 16:03:33 2024 +0200 Revert "Further locking harmonizations for main components" This reverts commit b241b08c9d5b92a183eb686886e8c5bbed9b9835. --- java/org/apache/catalina/core/ContainerBase.java | 86 +++++++++------------- java/org/apache/catalina/core/StandardService.java | 22 +++--- webapps/docs/changelog.xml | 7 +- 3 files changed, 48 insertions(+), 67 deletions(-) diff --git a/java/org/apache/catalina/core/ContainerBase.java b/java/org/apache/catalina/core/ContainerBase.java index fd295cb52f..c15298a359 100644 --- a/java/org/apache/catalina/core/ContainerBase.java +++ b/java/org/apache/catalina/core/ContainerBase.java @@ -152,7 +152,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(); /** @@ -666,30 +665,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); } - } @@ -726,11 +721,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(); } } @@ -741,11 +733,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(); } } @@ -772,40 +761,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(); - } } @@ -1214,16 +1199,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 fd89e5dc7a..5b5c085d36 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -112,11 +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> <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> </fix> </changelog> </subsection> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org