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@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); + } }