cocoon-cvs mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jo...@apache.org
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 GMT
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 <code>WebContinuation</code> object given a native
      * continuation object and its parent. If the parent continuation is
      * null, the <code>WebContinuation</code> returned becomes the root
-     * of a tree in the forrest.
+     * of a tree in the forest.
      *
      * @param kont an <code>Object</code> value
      * @param parentKont a <code>WebContinuation</code> value
@@ -83,18 +84,36 @@
      * @return a <code>WebContinuation</code> object, null if no such
      * <code>WebContinuation</code> could be found. Also null if 
      * <code>WebContinuation</code> 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 <code>WebContinuationDataBean</code>
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 <code>WebContinuation</code> 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 <code>WebContinuation</code> 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 <code>WebContinuation</code> 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 <code>WebContinuation</code> 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 <code>SortedSet</code>
-     */
-    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 <code>WebContinuation</code>s
-     * 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 <code>SortedSet</code>
+     */
+    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 <code>WebContinuation</code>s
+     * 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 @@
 
   <changes>
   <release version="2.1.12" date="TBD">
+    <action dev="JH" type="fix">
+      Core: Fix synchronization issues in ContinuationsManager implementation.
+    </action>
     <action dev="JH" type="fix" fixes-bug="COCOON-2178" due-to="Harald Entner" due-to-email="harald.entner@workflow.at">
       Forms: Dispatch only one TreeSelectionEvent on multiple selection rather than one event
for each selected tree item.
     </action>



Mime
View raw message