Return-Path: Delivered-To: apmail-cocoon-cvs-archive@www.apache.org Received: (qmail 2982 invoked from network); 1 Apr 2008 06:22:19 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 1 Apr 2008 06:22:19 -0000 Received: (qmail 21445 invoked by uid 500); 1 Apr 2008 06:22:19 -0000 Delivered-To: apmail-cocoon-cvs-archive@cocoon.apache.org Received: (qmail 21334 invoked by uid 500); 1 Apr 2008 06:22:18 -0000 Mailing-List: contact cvs-help@cocoon.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@cocoon.apache.org list-help: list-unsubscribe: List-Post: List-Id: Delivered-To: mailing list cvs@cocoon.apache.org Received: (qmail 21323 invoked by uid 99); 1 Apr 2008 06:22:18 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 31 Mar 2008 23:22:18 -0700 X-ASF-Spam-Status: No, hits=-2000.0 required=10.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.3] (HELO eris.apache.org) (140.211.11.3) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 01 Apr 2008 06:21:45 +0000 Received: by eris.apache.org (Postfix, from userid 65534) id 042C41A983A; Mon, 31 Mar 2008 23:21:58 -0700 (PDT) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r643295 - in /cocoon/branches/BRANCH_2_1_X: src/java/org/apache/cocoon/components/flow/ContinuationsManager.java src/java/org/apache/cocoon/components/flow/ContinuationsManagerImpl.java status.xml Date: Tue, 01 Apr 2008 06:21:56 -0000 To: cvs@cocoon.apache.org From: joerg@apache.org X-Mailer: svnmailer-1.0.8 Message-Id: <20080401062158.042C41A983A@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: joerg Date: Mon Mar 31 23:21:53 2008 New Revision: 643295 URL: http://svn.apache.org/viewvc?rev=643295&view=rev Log: Fix synchronization issues in ContinuationsManager implementation. Modified: cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManager.java cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManagerImpl.java cocoon/branches/BRANCH_2_1_X/status.xml Modified: cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManager.java URL: http://svn.apache.org/viewvc/cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManager.java?rev=643295&r1=643294&r2=643295&view=diff ============================================================================== --- cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManager.java (original) +++ cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManager.java Mon Mar 31 23:21:53 2008 @@ -17,11 +17,12 @@ package org.apache.cocoon.components.flow; import java.util.List; +import java.util.Set; /** * The interface of the Continuations manager. * - * The continuation manager maintains a forrest of {@link + * The continuation manager maintains a forest of {@link * WebContinuation} trees. Each tree defines the flow of control for a * user within the application. * @@ -43,7 +44,7 @@ * Create a WebContinuation object given a native * continuation object and its parent. If the parent continuation is * null, the WebContinuation returned becomes the root - * of a tree in the forrest. + * of a tree in the forest. * * @param kont an Object value * @param parentKont a WebContinuation value @@ -83,18 +84,36 @@ * @return a WebContinuation object, null if no such * WebContinuation could be found. Also null if * WebContinuation was found but interpreter id does - * not match the one that the continuation was initialy created for. + * not match the one that the continuation was initially created for. */ public WebContinuation lookupWebContinuation(String id, String interpreterId); /** * Prints debug information about all web continuations into the log file. - * @see WebContinuation#display() + * + * @deprecated Use {@link #getForest()}. This method will be removed from + * the interface. */ public void displayAllContinuations(); /** * Get a list of all continuations as WebContinuationDataBean objects. + * + * @deprecated Use {@link #getForest()}. This method will be removed. */ - public List getWebContinuationsDataBeanList(); + public List getWebContinuationsDataBeanList(); + + /** + * Get a set of all web continuations. The set itself will only contain the + * root continuations. Those will provide access to their children. + * + * Since it should not be possible to mess up the actual managed + * continuations the returned list will contain clones of them. + * + * The purpose of this method is clearly monitoring or for debugging the + * application. It has no direct relationship to functionality of the + * ContinuationsManager. + */ + public Set getForest(); + } Modified: cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManagerImpl.java URL: http://svn.apache.org/viewvc/cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManagerImpl.java?rev=643295&r1=643294&r2=643295&view=diff ============================================================================== --- cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManagerImpl.java (original) +++ cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/flow/ContinuationsManagerImpl.java Mon Mar 31 23:21:53 2008 @@ -32,6 +32,9 @@ import org.apache.cocoon.environment.ObjectModelHelper; import org.apache.cocoon.environment.Request; import org.apache.cocoon.environment.Session; +import org.apache.cocoon.util.Deprecation; +import org.apache.commons.collections.Predicate; +import org.apache.commons.collections.iterators.FilterIterator; import org.apache.excalibur.instrument.CounterInstrument; import org.apache.excalibur.instrument.Instrument; @@ -79,53 +82,45 @@ implements ContinuationsManager, Component, Configurable, ThreadSafe, Instrumentable, Serviceable, Contextualizable { - static final int CONTINUATION_ID_LENGTH = 20; - static final String EXPIRE_CONTINUATIONS = "expire-continuations"; + private static final int CONTINUATION_ID_LENGTH = 20; /** * Random number generator used to create continuation ID */ protected SecureRandom random; - protected byte[] bytes; + protected final byte[] bytes = new byte[CONTINUATION_ID_LENGTH]; /** - * How long does a continuation exist in memory since the last - * access? The time is in milliseconds, and the default is 1 hour. + * Sorted set of WebContinuation instances, based on + * their expiration time. This is used by the background thread to + * invalidate continuations. */ - protected int defaultTimeToLive; + protected final SortedSet expirations = Collections.synchronizedSortedSet(new TreeSet()); + + protected ServiceManager serviceManager; + protected Context context; /** - * Maintains the forest of WebContinuation trees. - * This set is used only for debugging purposes by - * {@link #displayAllContinuations()} method. + * How long does a continuation exist in memory since the last + * access? The time is in milliseconds, and the default is 1 hour. */ - protected Set forest = Collections.synchronizedSet(new HashSet()); + protected int defaultTimeToLive; + protected boolean isContinuationSharingBugCompatible; + protected boolean bindContinuationsToSession; /** * Main continuations holder. Used unless continuations are stored in user * session. */ protected WebContinuationsHolder continuationsHolder; - - /** - * Sorted set of WebContinuation instances, based on - * their expiration time. This is used by the background thread to - * invalidate continuations. - */ - protected SortedSet expirations = Collections.synchronizedSortedSet(new TreeSet()); + // Instrumentation protected String instrumentableName; protected ValueInstrument continuationsCount; protected int continuationsCounter; - protected ValueInstrument forestSize; protected ValueInstrument expirationsSize; protected CounterInstrument continuationsCreated; protected CounterInstrument continuationsInvalidated; - protected boolean isContinuationSharingBugCompatible; - protected boolean bindContinuationsToSession; - - protected ServiceManager serviceManager; - protected Context context; public ContinuationsManagerImpl() throws Exception { @@ -136,11 +131,9 @@ random = SecureRandom.getInstance("IBMSecureRandom"); } random.setSeed(System.currentTimeMillis()); - bytes = new byte[CONTINUATION_ID_LENGTH]; continuationsCount = new ValueInstrument("count"); continuationsCounter = 0; - forestSize = new ValueInstrument("forest-size"); expirationsSize = new ValueInstrument("expirations-size"); continuationsCreated = new CounterInstrument("creates"); continuationsInvalidated = new CounterInstrument("invalidates"); @@ -213,8 +206,7 @@ return new Instrument[]{ continuationsCount, continuationsCreated, - continuationsInvalidated, - forestSize + continuationsInvalidated }; } @@ -233,20 +225,18 @@ int timeToLive, String interpreterId, ContinuationsDisposer disposer) { - int ttl = (timeToLive == 0 ? defaultTimeToLive : timeToLive); + int ttl = timeToLive == 0 ? defaultTimeToLive : timeToLive; WebContinuation wk = generateContinuation(kont, parent, ttl, interpreterId, disposer); - wk.enableLogging(getLogger()); - if (parent == null) { - forest.add(wk); - forestSize.setValue(forest.size()); - } else { - handleParentContinuationExpiration(parent); + synchronized (this.expirations) { + if (parent != null) { + expirations.remove(parent); + } + expirations.add(wk); + expirationsSize.setValue(expirations.size()); } - - handleLeafContinuationExpiration(wk); - + if (getLogger().isDebugEnabled()) { getLogger().debug("WK: Created continuation " + wk.getId()); } @@ -255,26 +245,6 @@ } /** - * When a new continuation is created in @link #createWebContinuation(Object, WebContinuation, int, String, ContinuationsDisposer), - * it is registered in the expiration set in order to be evaluated by the invalidation mechanism. - */ - protected void handleLeafContinuationExpiration(WebContinuation wk) { - expirations.add(wk); - expirationsSize.setValue(expirations.size()); - } - - /** - * When a new continuation is created in @link #createWebContinuation(Object, WebContinuation, int, String, ContinuationsDisposer), - * its parent continuation is removed from the expiration set. This way only leaf continuations are part of - * the expiration set. - */ - protected void handleParentContinuationExpiration(WebContinuation parent) { - if (parent.getChildren().size() < 2) { - expirations.remove(parent); - } - } - - /** * @see org.apache.cocoon.components.flow.ContinuationsManager#lookupWebContinuation(java.lang.String, java.lang.String) */ public WebContinuation lookupWebContinuation(String id, String interpreterId) { @@ -308,10 +278,11 @@ // COCOON-2109: Sorting in the TreeSet happens on insert. So in order to re-sort the // continuation has to be re-added. - expirations.remove(kont); - kont.updateLastAccessTime(); - expirations.add(kont); - + synchronized (this.expirations) { + this.expirations.remove(kont); + kont.updateLastAccessTime(); + this.expirations.add(kont); + } return kont; } @@ -329,10 +300,10 @@ * @return the generated WebContinuation with unique identifier */ protected WebContinuation generateContinuation(Object kont, - WebContinuation parent, - int ttl, - String interpreterId, - ContinuationsDisposer disposer) { + WebContinuation parent, + int ttl, + String interpreterId, + ContinuationsDisposer disposer) { char[] result = new char[bytes.length * 2]; WebContinuation wk = null; @@ -347,7 +318,7 @@ } final String id = new String(result); - synchronized (continuationsHolder) { + synchronized (continuationsHolder.holder) { if (!continuationsHolder.contains(id)) { if (this.bindContinuationsToSession) { wk = new HolderAwareWebContinuation(id, kont, parent, @@ -372,9 +343,6 @@ return wk; } - /** - * @see org.apache.cocoon.components.flow.ContinuationsManager#invalidateWebContinuation(org.apache.cocoon.components.flow.WebContinuation) - */ public void invalidateWebContinuation(WebContinuation wk) { WebContinuationsHolder continuationsHolder = lookupWebContinuationsHolder(false); if (!continuationsHolder.contains(wk)) { @@ -390,9 +358,10 @@ getLogger().debug("WK: Manual expire of continuation " + wk.getId()); } disposeContinuation(continuationsHolder, wk); - expirations.remove(wk); - expirationsSize.setValue(expirations.size()); - + synchronized (this.expirations) { + expirations.remove(wk); + expirationsSize.setValue(expirations.size()); + } // Invalidate all the children continuations as well List children = wk.getChildren(); int size = children.size(); @@ -403,17 +372,14 @@ /** * Detach this continuation from parent. This method removes - * continuation from {@link #forest} set, or, if it has parent, - * from parent's children collection. + * continuation from parent's children collection, if it has parent. * @param wk Continuation to detach from parent. */ private void _detach(WebContinuation wk) { WebContinuation parent = wk.getParentContinuation(); - if (parent == null) { - forest.remove(wk); - forestSize.setValue(forest.size()); - } else + if (parent != null) { wk.detachFromParent(); + } } /** @@ -446,7 +412,7 @@ return; } - // remove access to this contination + // remove access to this continuation disposeContinuation(continuationsHolder, wk); _detach(wk); @@ -463,42 +429,6 @@ } /** - * Dump to Log file the current contents of - * the expirations SortedSet - */ - protected void displayExpireSet() { - StringBuffer wkSet = new StringBuffer("\nWK; Expire set size: " + expirations.size()); - Iterator i = expirations.iterator(); - while (i.hasNext()) { - final WebContinuation wk = (WebContinuation) i.next(); - final long lat = wk.getLastAccessTime() + wk.getTimeToLive(); - wkSet.append("\nWK: ") - .append(wk.getId()) - .append(" ExpireTime ["); - - if (lat < System.currentTimeMillis()) { - wkSet.append("Expired"); - } else { - wkSet.append(lat); - } - wkSet.append("]"); - } - - getLogger().debug(wkSet.toString()); - } - - /** - * Dump to Log file all WebContinuations - * in the system - */ - public void displayAllContinuations() { - final Iterator i = forest.iterator(); - while (i.hasNext()) { - ((WebContinuation) i.next()).display(); - } - } - - /** * Remove all continuations which have already expired. */ protected void expireContinuations() { @@ -507,7 +437,6 @@ now = System.currentTimeMillis(); /* Continuations before clean up: */ - getLogger().debug("WK: Forest before cleanup: " + forest.size()); displayAllContinuations(); displayExpireSet(); @@ -515,31 +444,30 @@ // Clean up expired continuations int count = 0; - WebContinuation wk; - Iterator i = expirations.iterator(); - while(i.hasNext() && ((wk = (WebContinuation) i.next()).hasExpired())) { - i.remove(); - WebContinuationsHolder continuationsHolder = null; - if(wk instanceof HolderAwareWebContinuation) { - continuationsHolder = ((HolderAwareWebContinuation) wk).getContinuationsHolder(); - } - else { - continuationsHolder = this.continuationsHolder; + FilterIterator expirationIterator = new FilterIterator(); + Predicate expirationPredicate = new ExpirationPredicate(); + expirationIterator.setPredicate(expirationPredicate); + synchronized (this.expirations) { + expirationIterator.setIterator(this.expirations.iterator()); + while (expirationIterator.hasNext()) { + WebContinuation wk = (WebContinuation) expirationIterator.next(); + expirationIterator.remove(); + WebContinuationsHolder continuationsHolder = null; + if (wk instanceof HolderAwareWebContinuation) { + continuationsHolder = + ((HolderAwareWebContinuation) wk).getContinuationsHolder(); + } else { + continuationsHolder = this.continuationsHolder; + } + removeContinuation(continuationsHolder, wk); + count++; } - removeContinuation(continuationsHolder, wk); - count++; + expirationsSize.setValue(expirations.size()); } - expirationsSize.setValue(expirations.size()); - + if (getLogger().isDebugEnabled()) { getLogger().debug("WK Cleaned up " + count + " continuations in " + (System.currentTimeMillis() - now) + " ms"); - - /* Continuations after clean up: */ -// getLogger().debug("WK: Forest after cleanup: " + forest.size()); -// displayAllContinuations(); -// displayExpireSet(); - } } @@ -548,16 +476,10 @@ * about session invalidation. Invalidates all continuations held by passed * continuationsHolder. */ - protected void invalidateContinuations( - WebContinuationsHolder continuationsHolder) { - // TODO: this avoids ConcurrentModificationException, still this is not - // the best solution and should be changed - Object[] continuationIds = continuationsHolder.getContinuationIds() - .toArray(); - - for (int i = 0; i < continuationIds.length; i++) { - WebContinuation wk = continuationsHolder.get(continuationIds[i]); - if (wk != null) { + protected void invalidateContinuations(WebContinuationsHolder continuationsHolder) { + synchronized (continuationsHolder.holder) { + for (Iterator iter = continuationsHolder.holder.values().iterator(); iter.hasNext();) { + WebContinuation wk = (WebContinuation) iter.next(); _detach(wk); _invalidate(continuationsHolder, wk); } @@ -593,16 +515,96 @@ return holder; holder = new WebContinuationsHolder(); - session.setAttribute(WebContinuationsHolder.CONTINUATIONS_HOLDER, - holder); + session.setAttribute(WebContinuationsHolder.CONTINUATIONS_HOLDER, holder); return holder; } + public Set getForest() { + Set rootWebContinuations = new HashSet(); + // identify the root continuations, once done no more need to lock + synchronized (this.expirations) { + for (Iterator iter = this.expirations.iterator(); iter.hasNext();) { + WebContinuation webContinuation = (WebContinuation) iter.next(); + while (webContinuation.getParentContinuation() != null) { + webContinuation = webContinuation.getParentContinuation(); + } + rootWebContinuations.add(webContinuation); + } + } + + Set clonedRootWebContinuations = new HashSet(); + for (Iterator iter = rootWebContinuations.iterator(); iter.hasNext();) { + WebContinuation rootContinuation = (WebContinuation) iter.next(); + clonedRootWebContinuations.add(rootContinuation.clone()); + } + return clonedRootWebContinuations; + } + + /** + * Get a list of all web continuations (data only) + * + * @deprecated + */ + public List getWebContinuationsDataBeanList() { + if (Deprecation.logger.isWarnEnabled()) { + Deprecation.logger.warn("ContinuationsManager.getWebContinuationsDataBeanList()" + + " is deprecated and should be replaced with getForest()."); + } + List beanList = new ArrayList(); + for(Iterator it = getForest().iterator(); it.hasNext();) { + beanList.add(new WebContinuationDataBean((WebContinuation) it.next())); + } + return beanList; + } + + /** + * Dump to Log file the current contents of + * the expirations SortedSet + */ + protected void displayExpireSet() { + StringBuffer wkSet = new StringBuffer("\nWK; Expire set size: "); + + synchronized (this.expirations) { + wkSet.append(this.expirations.size()); + for (Iterator i = this.expirations.iterator(); i.hasNext();) { + final WebContinuation wk = (WebContinuation) i.next(); + wkSet.append("\nWK: ").append(wk.getId()).append(" ExpireTime ["); + if (wk.hasExpired()) { + wkSet.append("Expired"); + } else { + wkSet.append(wk.getLastAccessTime() + wk.getTimeToLive()); + } + wkSet.append("]"); + } + } + getLogger().debug(wkSet.toString()); + } + + /** + * Dump to Log file all WebContinuations + * in the system. + * + * This method will be changed to be an internal method solely for debugging + * purposes just like {@link #displayExpireSet()}. + */ + public void displayAllContinuations() { + if (getLogger().isDebugEnabled()) { + Set forest = getForest(); + getLogger().debug("WK: Forest size: " + forest.size()); + for (Iterator iter = forest.iterator(); iter.hasNext();) { + getLogger().debug(iter.next().toString()); + } + } + } + /** * A holder for WebContinuations. When bound to session notifies the * continuations manager of session invalidation. + * + * For thread-safe access you have to synchronize on the Map {@link #holder}! */ protected class WebContinuationsHolder implements HttpSessionBindingListener { + private final static String CONTINUATIONS_HOLDER = "o.a.c.c.f.SCMI.WebContinuationsHolder"; private Map holder = Collections.synchronizedMap(new HashMap()); @@ -637,6 +639,7 @@ public void valueUnbound(HttpSessionBindingEvent event) { invalidateContinuations(this); } + } /** @@ -663,15 +666,12 @@ } - /** - * Get a list of all web continuations (data only) - */ - public List getWebContinuationsDataBeanList() { - List beanList = new ArrayList(); - for(Iterator it = this.forest.iterator(); it.hasNext();) { - beanList.add(new WebContinuationDataBean((WebContinuation) it.next())); + protected static class ExpirationPredicate implements Predicate { + + public boolean evaluate(final Object obj) { + return ((WebContinuation)obj).hasExpired(); } - return beanList; + } } Modified: cocoon/branches/BRANCH_2_1_X/status.xml URL: http://svn.apache.org/viewvc/cocoon/branches/BRANCH_2_1_X/status.xml?rev=643295&r1=643294&r2=643295&view=diff ============================================================================== --- cocoon/branches/BRANCH_2_1_X/status.xml (original) +++ cocoon/branches/BRANCH_2_1_X/status.xml Mon Mar 31 23:21:53 2008 @@ -182,6 +182,9 @@ + + Core: Fix synchronization issues in ContinuationsManager implementation. + Forms: Dispatch only one TreeSelectionEvent on multiple selection rather than one event for each selected tree item.