On Thu, Apr 4, 2024 at 3:58 PM Konstantin Kolinko <knst.koli...@gmail.com> wrote: > > чт, 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.
Yes, it's all synced. This is rather inconsistent overall, and I thought first about harmonizing the other way. I don't understand why the lifecycle operation needs to be under the sync (and the notification also seemed like a bad idea) so it could be better. Rémy > 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 > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
