You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2024/04/04 14:03:53 UTC
(tomcat) branch 10.1.x updated: Revert "Further locking harmonizations for main components"
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 <re...@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org