[gatein-commits] gatein SVN: r7614 - in portal/trunk/component/common/src: test/java/org/exoplatform/commons/cache/future and 1 other directory.

[do-not-reply at jboss.org]

Author: julien_viet
Date: 2011-10-03 12:09:34 -0400 (Mon, 03 Oct 2011)
New Revision: 7614

Modified:
   portal/trunk/component/common/src/main/java/org/exoplatform/commons/cache/future/FutureCache.java
   portal/trunk/component/common/src/test/java/org/exoplatform/commons/cache/future/GetTestCase.java
Log:
GTNPORTAL-2148 : Detect reentrancy in future cache to prevent self deadlock


Modified: portal/trunk/component/common/src/main/java/org/exoplatform/commons/cache/future/FutureCache.java
===================================================================
--- portal/trunk/component/common/src/main/java/org/exoplatform/commons/cache/future/FutureCache.java	2011-10-03 16:02:25 UTC (rev 7613)
+++ portal/trunk/component/common/src/main/java/org/exoplatform/commons/cache/future/FutureCache.java	2011-10-03 16:09:34 UTC (rev 7614)
@@ -49,10 +49,10 @@
 {
 
    /** . */
-   private final Loader<K, V, C> loader;
+   final Loader<K, V, C> loader;
 
    /** . */
-   private final ConcurrentMap<K, FutureTask<V>> futureEntries;
+   private final ConcurrentMap<K, Retrieval<K, V, C>> futureEntries;
 
    /** . */
    private final Logger log = LoggerFactory.getLogger(FutureCache.class);
@@ -60,7 +60,7 @@
    public FutureCache(Loader<K, V, C> loader)
    {
       this.loader = loader;
-      this.futureEntries = new ConcurrentHashMap<K, FutureTask<V>>();
+      this.futureEntries = new ConcurrentHashMap<K, Retrieval<K, V, C>>();
    }
 
    /**
@@ -99,65 +99,67 @@
       if (value == null)
       {
          // Create our future
-         FutureTask<V> future = new FutureTask<V>(new Callable<V>()
-         {
-            public V call() throws Exception
-            {
-               // Retrieve the value from the loader
-               V value = loader.retrieve(context, key);
+         Retrieval<K, V, C> retrieval = new Retrieval<K, V, C>(context, key, this);
 
-               //
-               if (value != null)
-               {
-                  // Cache it, it is made available to other threads (unless someone removes it)
-                  put(key, value);
-
-                  // Return value
-                  return value;
-               }
-               else
-               {
-                  return null;
-               }
-            }
-         });
-
          // This boolean means we inserted in the local
          boolean inserted = true;
 
          //
          try
          {
-            FutureTask<V> phantom = futureEntries.putIfAbsent(key, future);
+            Retrieval<K, V, C> phantom = futureEntries.putIfAbsent(key, retrieval);
 
             // Use the value that could have been inserted by another thread
             if (phantom != null)
             {
-               future = phantom;
+               retrieval = phantom;
                inserted = false;
             }
             else
             {
-               future.run();
+               try
+               {
+                  retrieval.current = Thread.currentThread();
+                  retrieval.future.run();
+               }
+               catch (Exception e)
+               {
+                  log.error("Retrieval of resource " + key + " threw an exception", e);
+               }
+               finally
+               {
+                  retrieval.current = null;
+               }
             }
 
             // Returns the value
-            value = future.get();
+            if (retrieval.current == Thread.currentThread())
+            {
+               throw new IllegalStateException("Reentrancy detected when obtaining key " + key + " with context " + context + " detected");
+            }
+            else
+            {
+               try
+               {
+                  value = retrieval.future.get();
+               }
+               catch (ExecutionException e)
+               {
+                  log.error("Computing of resource " + key + " threw an exception", e.getCause());
+               }
+               catch (InterruptedException e)
+               {
+                  // We should handle interruped exception in some manner
+                  log.error("Retrieval of resource " + key + " threw an exception", e);
+               }
+            }
          }
-         catch (ExecutionException e)
-         {
-            log.error("Computing of resource " + key + " threw an exception", e.getCause());
-         }
-         catch (Exception e)
-         {
-            log.error("Retrieval of resource " + key + " threw an exception", e);
-         }
          finally
          {
             // Clean up the per key map but only if our insertion succeeded and with our future
             if (inserted)
             {
-               futureEntries.remove(key, future);
+               futureEntries.remove(key, retrieval);
             }
          }
       }

Modified: portal/trunk/component/common/src/test/java/org/exoplatform/commons/cache/future/GetTestCase.java
===================================================================
--- portal/trunk/component/common/src/test/java/org/exoplatform/commons/cache/future/GetTestCase.java	2011-10-03 16:02:25 UTC (rev 7613)
+++ portal/trunk/component/common/src/test/java/org/exoplatform/commons/cache/future/GetTestCase.java	2011-10-03 16:09:34 UTC (rev 7614)
@@ -23,6 +23,7 @@
 import junit.framework.TestCase;
 
 import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * @author <a href="mailto:julien.viet at exoplatform.com">Julien Viet</a>
@@ -68,4 +69,32 @@
       }, "foo"));
       Assert.assertFalse(futureCache.data.containsKey("foo"));
    }
+
+   public void testReentrancy()
+   {
+      final FutureMap<Callable<String>> futureCache = new FutureMap<Callable<String>>(new StringLoader());
+      String res = futureCache.get(new Callable<String>()
+      {
+         public String call() throws Exception
+         {
+            try
+            {
+               futureCache.get(new Callable<String>()
+               {
+                  public String call() throws Exception
+                  {
+                     // Should not go there
+                     throw new AssertionError();
+                  }
+               }, "foo");
+               return "fail";
+            }
+            catch (IllegalStateException expected)
+            {
+               return "pass";
+            }
+         }
+      }, "foo");
+      assertEquals("pass", res);
+   }
 }