8:51 a.m.
Author: manik.surtani(a)jboss.com
Date: 2008-05-29 09:51:25 -0400 (Thu, 29 May 2008)
New Revision: 5915
Added:
core/trunk/src/test/java/org/jboss/cache/loader/ConcurrentPutRemoveEvictTest.java
Modified:
core/trunk/src/main/java/org/jboss/cache/interceptors/CacheLoaderInterceptor.java
core/trunk/src/main/java/org/jboss/cache/lock/LockManager.java
core/trunk/src/main/java/org/jboss/cache/lock/NodeBasedLockManager.java
Log:
JBCACHE-1355 - race conditions in the CacheLoaderInterceptor
Modified:
core/trunk/src/main/java/org/jboss/cache/interceptors/CacheLoaderInterceptor.java
===================================================================
---
core/trunk/src/main/java/org/jboss/cache/interceptors/CacheLoaderInterceptor.java 2008-05-29
13:02:52 UTC (rev 5914)
+++
core/trunk/src/main/java/org/jboss/cache/interceptors/CacheLoaderInterceptor.java 2008-05-29
13:51:25 UTC (rev 5915)
@@ -28,6 +28,7 @@
import org.jboss.cache.loader.CacheLoaderManager;
import org.jboss.cache.lock.LockManager;
import org.jboss.cache.lock.LockType;
+import org.jboss.cache.lock.TimeoutException;
import org.jboss.cache.notifications.NotifierImpl;
import org.jboss.cache.transaction.TransactionEntry;
import org.jboss.cache.transaction.TransactionTable;
@@ -243,8 +244,23 @@
private void loadIfNeeded(InvocationContext ctx, Fqn fqn, Object key, boolean allKeys,
boolean initNode, boolean acquireLock, TransactionEntry entry, boolean recursive, boolean
isMove, boolean bypassLoadingData) throws Throwable
{
NodeSPI n = dataContainer.peek(fqn, true, true);
+ Object lockOwner = lockManager.getLockOwner(ctx);
+ boolean needLock = n != null && !lockManager.ownsLock(fqn, lockOwner);
+ boolean mustLoad = false;
+ try
+ {
+ if (needLock)
+ {
+ if (!lockManager.lock(n, LockType.READ, lockOwner))
+ throw new TimeoutException("Unable to acquire lock on " + fqn +
". Lock info: " + lockManager.printLockInfo(n));
+ }
+ mustLoad = mustLoad(n, key, allKeys || isMove);
+ }
+ finally
+ {
+ if (needLock) lockManager.unlock(n, lockOwner);
+ }
- boolean mustLoad = mustLoad(n, key, allKeys || isMove);
if (trace)
{
log.trace("load element " + fqn + " mustLoad=" + mustLoad);
Modified: core/trunk/src/main/java/org/jboss/cache/lock/LockManager.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/lock/LockManager.java 2008-05-29 13:02:52 UTC
(rev 5914)
+++ core/trunk/src/main/java/org/jboss/cache/lock/LockManager.java 2008-05-29 13:51:25 UTC
(rev 5915)
@@ -47,6 +47,16 @@
boolean lock(Fqn fqn, LockType lockType, Object owner, long timeout);
/**
+ * As {@link #lock(org.jboss.cache.Fqn, LockType, Object)} except that a NodeSPI is
passed in instead of an Fqn.
+ *
+ * @param node node to lock
+ * @param lockType type of lock to acquire
+ * @param owner owner to acquire the lock for
+ * @return true if the lock was acquired, false otherwise.
+ */
+ boolean lock(NodeSPI node, LockType lockType, Object owner);
+
+ /**
* As {@link #lock(org.jboss.cache.Fqn, LockType, Object, long)} except that a NodeSPI
is passed in instead of an Fqn.
*
* @param node node to lock
@@ -233,6 +243,15 @@
boolean ownsLock(Fqn fqn, Object owner);
/**
+ * Tests whether a given owner owns any sort of lock on a particular Fqn.
+ *
+ * @param node to test
+ * @param owner owner
+ * @return true if the owner does own the specified lock type on the specified node,
false otherwise.
+ */
+ boolean ownsLock(NodeSPI node, Object owner);
+
+ /**
* Returns true if the node is locked (either for reading or writing) by anyone, and
false otherwise.
*
* @param n node to inspect
Modified: core/trunk/src/main/java/org/jboss/cache/lock/NodeBasedLockManager.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/lock/NodeBasedLockManager.java 2008-05-29
13:02:52 UTC (rev 5914)
+++ core/trunk/src/main/java/org/jboss/cache/lock/NodeBasedLockManager.java 2008-05-29
13:51:25 UTC (rev 5915)
@@ -93,6 +93,11 @@
return acquireLock(fqn, lockType, owner, timeout) != null;
}
+ public boolean lock(NodeSPI node, LockType lockType, Object owner)
+ {
+ return acquireLock(node, lockType, owner, lockAcquisitionTimeout) != null;
+ }
+
public boolean lock(NodeSPI node, LockType lockType, Object owner, long timeout)
{
return acquireLock(node, lockType, owner, timeout) != null;
@@ -156,6 +161,7 @@
public void unlock(NodeSPI node, Object owner)
{
+ if (node == null) return;
unlock(node.getLock(), owner);
}
@@ -255,7 +261,11 @@
public boolean ownsLock(Fqn fqn, Object owner)
{
- NodeSPI node = dataContainer.peek(fqn);
+ return ownsLock(dataContainer.peek(fqn), owner);
+ }
+
+ public boolean ownsLock(NodeSPI node, Object owner)
+ {
return node != null && node.getLock().isOwner(owner);
}
Added: core/trunk/src/test/java/org/jboss/cache/loader/ConcurrentPutRemoveEvictTest.java
===================================================================
--- core/trunk/src/test/java/org/jboss/cache/loader/ConcurrentPutRemoveEvictTest.java
(rev 0)
+++
core/trunk/src/test/java/org/jboss/cache/loader/ConcurrentPutRemoveEvictTest.java 2008-05-29
13:51:25 UTC (rev 5915)
@@ -0,0 +1,131 @@
+package org.jboss.cache.loader;
+
+import org.jboss.cache.Cache;
+import org.jboss.cache.DefaultCacheFactory;
+import org.jboss.cache.Fqn;
+import org.jboss.cache.config.CacheLoaderConfig;
+import org.jboss.cache.config.Configuration;
+import org.jboss.cache.util.TestingUtil;
+import org.testng.annotations.AfterTest;
+import org.testng.annotations.BeforeTest;
+import org.testng.annotations.Test;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * To test JBCACHE-1355
+ *
+ * @author Manik Surtani (<a
href="mailto:manik@jboss.org">manik@jboss.org</a>)
+ * @since 2.2.0
+ */
+@Test(groups = "functional")
+public class ConcurrentPutRemoveEvictTest extends AbstractCacheLoaderTestBase
+{
+ Cache<String, String> cache;
+ Fqn fqn = Fqn.fromString("/a");
+ String key = "key";
+ boolean run = true;
+ Set<Exception> exceptions = new HashSet<Exception>();
+
+ @BeforeTest
+ public void setUp() throws Exception
+ {
+ CacheLoaderConfig cacheLoaderConfig = getSingleCacheLoaderConfig("",
DummyInMemoryCacheLoader.class.getName(), "", false, false, false);
+ Configuration cfg = new Configuration();
+ cfg.setCacheLoaderConfig(cacheLoaderConfig);
+ cache = new DefaultCacheFactory<String, String>().createCache(cfg);
+ cache.put(fqn, key, "value");
+ }
+
+ @AfterTest
+ public void tearDown()
+ {
+ TestingUtil.killCaches(cache);
+ }
+
+ public void doTest() throws Exception
+ {
+ List<Thread> threads = new ArrayList<Thread>();
+
+ threads.add(new Getter());
+ threads.add(new RandomAdder());
+ threads.add(new Evicter());
+
+ for (Thread t : threads) t.start();
+
+ // let these run for a while.
+ TestingUtil.sleepThread(10000);
+
+ run = false;
+
+ for (Thread t : threads) t.join();
+
+ if (!exceptions.isEmpty())
+ {
+ for (Exception e : exceptions) throw e;
+ }
+ }
+
+ private class RandomAdder extends Thread
+ {
+ public void run()
+ {
+ int i = 0;
+ while (run)
+ {
+ try
+ {
+ cache.put(fqn, key + (i++), "");
+ }
+ catch (Exception e)
+ {
+ // ignore
+ }
+ }
+ }
+ }
+
+
+ private class Getter extends Thread
+ {
+ public void run()
+ {
+ while (run)
+ {
+ try
+ {
+ // note that we sometimes get a null back. This is incorrect and
inconsistent, but has to do with locks being held
+ // on nodes. Very similar to
http://jira.jboss.org/jira/browse/JBCACHE-1165
+ String value = cache.get(fqn, key);
+ System.out.println("Thread " + getName() + " got value
" + value);
+ }
+ catch (Exception e)
+ {
+ exceptions.add(e);
+ }
+ }
+ }
+ }
+
+ private class Evicter extends Thread
+ {
+ public void run()
+ {
+ while (run)
+ {
+ try
+ {
+ cache.evict(fqn);
+ }
+ catch (Exception e)
+ {
+ // who cares
+ }
+ }
+ }
+ }
+
+}