чт, 4 апр. 2024 г. в 15:45, <r...@apache.org>:
>
> 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();
It is useless to have the above "children" field as protected.
Nobody will be able to access it in a safe manner, because they have
no access to childrenLock.
> @@ -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);
> - }
Change of behaviour!
The old sync block ended with the "}" above.
> - 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
The above says "Don't do this inside sync block",
but your change puts it into a sync block.
(And I see similar changes of behaviour further in this commit.)
The lock was intended to protect changes of the "children" hash table,
but now it starts to protect
a) state changes of those children (start/stop/destroy)
b) notifications.
I think that the state changes (in LifecycleBase) already have their
own protection.
While stating/stopping/destroying is in progress,
that child still remains ours and findChildren() etc. should be
allowed to be called by other threads.
A write lock prevents other threads from reading the value.
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html
Thus I am -1.
I think it is easier to revert and work on a new patch rather than try
to amend this one.
> + // 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();
Why add locking here? There was none.
The code does not modify the "children" map here. It happens later,
after the child has been stopped and destroyed.
> 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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
