This is an automated email from the ASF dual-hosted git repository.
remm pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 63eb105bf1 Further locking harmonizations for main components
63eb105bf1 is described below
commit 63eb105bf14fed5fdf29dc9171e46f73fae28128
Author: remm <[email protected]>
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 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 c7fbb6e342..5b363ec96f 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -131,8 +131,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: [email protected]
For additional commands, e-mail: [email protected]
