[jbosscache-commits] JBoss Cache SVN: r4993 - in core/trunk/src: main/java/org/jboss/cache/invocation and 2 other directories.
jbosscache-commits at lists.jboss.org
jbosscache-commits at lists.jboss.org
Fri Jan 4 14:03:15 EST 2008
Author: manik.surtani at jboss.com
Date: 2008-01-04 14:03:15 -0500 (Fri, 04 Jan 2008)
New Revision: 4993
Modified:
core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java
core/trunk/src/main/java/org/jboss/cache/invocation/RemoteCacheInvocationDelegate.java
core/trunk/src/test/java/org/jboss/cache/api/CacheAPIOptimisticTest.java
core/trunk/src/test/java/org/jboss/cache/api/CacheAPITest.java
core/trunk/src/test/java/org/jboss/cache/buddyreplication/GravitationCleanupTest.java
Log:
JBCACHE-1256 - PessimisticLockInterceptor does not clean up all temporary nodes created during a removeNode() call
Modified: core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java 2008-01-04 18:52:20 UTC (rev 4992)
+++ core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java 2008-01-04 19:03:15 UTC (rev 4993)
@@ -118,7 +118,7 @@
}
else
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, true, false, false, true);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, true, false, false, true, null, false);
}
return nextInterceptor(ctx);
}
@@ -130,7 +130,7 @@
protected Object handleLockMethod(InvocationContext ctx, Fqn fqn, NodeLock.LockType lockType, boolean recursive) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, lockType, false, false, false, false);
+ acquireLocksWithTimeout(ctx, fqn, lockType, false, false, false, false, null, false);
if (recursive)
{
//acquireLocksOnChildren(cache.peek(fqn, false), lockType, ctx);
@@ -204,7 +204,7 @@
if (trace) log.trace("Attempting to get WL on node to be moved [" + from + "]");
if (from != null && !(configuration.getIsolationLevel() == IsolationLevel.NONE))
{
- lock(ctx, from, NodeLock.LockType.WRITE, false, timeout, true, false);
+ lock(ctx, from, NodeLock.LockType.WRITE, false, timeout, true, false, null, false);
if (ctx.getGlobalTransaction() != null)
{
cache.getTransactionTable().get(ctx.getGlobalTransaction()).addRemovedNode(from);
@@ -215,7 +215,7 @@
{
//now for an RL for the new parent.
if (trace) log.trace("Attempting to get RL on new parent [" + to + "]");
- lock(ctx, to, NodeLock.LockType.READ, false, timeout, false, false);
+ lock(ctx, to, NodeLock.LockType.READ, false, timeout, false, false, null, false);
acquireLocksOnChildren(peekNode(ctx, to, false, true, false), NodeLock.LockType.READ, ctx);
}
Object retValue = nextInterceptor(ctx);
@@ -230,16 +230,27 @@
protected Object handleRemoveNodeMethod(InvocationContext ctx, GlobalTransaction tx, Fqn fqn, boolean createUndoOps) throws Throwable
{
- boolean created = acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, true, false, true, false);
+ // need to make a note of ALL nodes created here!!
+ List<Fqn> createdNodes = new LinkedList<Fqn>();
+ // we need to mark new nodes created as deleted since they are only created to form a path to the node being removed, to
+ // create a lock.
+ boolean created = acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, true, false, true, false, createdNodes, true);
if (ctx.getGlobalTransaction() != null)
{
- cache.getTransactionTable().get(ctx.getGlobalTransaction()).addRemovedNode(fqn);
+ TransactionEntry entry = tx_table.get(ctx.getGlobalTransaction());
+ entry.addRemovedNode(fqn);
+ for (Fqn f : createdNodes) entry.addRemovedNode(f);
}
- acquireLocksOnChildren(rootNode.getChildDirect(fqn), NodeLock.LockType.WRITE, ctx);
+ acquireLocksOnChildren(peekNode(ctx, fqn, false, false, false), NodeLock.LockType.WRITE, ctx);
Object retVal = nextInterceptor(ctx);
+
+ // and make sure we remove all nodes we've created for the sake of later removal.
if (ctx.getGlobalTransaction() == null)
{
+
+ for (Fqn f : createdNodes) cacheImpl.realRemove(f, true);
cacheImpl.realRemove(fqn, true);
+
NodeSPI n = peekNode(ctx, fqn, false, true, false);
if (n != null)
{
@@ -252,7 +263,7 @@
protected Object handlePutForExternalReadMethod(InvocationContext ctx, GlobalTransaction tx, Fqn fqn, Object key, Object value) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, true, true, false, true);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, true, true, false, true, null, false);
return nextInterceptor(ctx);
}
@@ -263,61 +274,61 @@
protected Object handleRemoveDataMethod(InvocationContext ctx, GlobalTransaction tx, Fqn fqn, boolean createUndoOps) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, false, false, false, false);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, false, false, false, false, null, false);
return nextInterceptor(ctx);
}
protected Object handleAddChildMethod(InvocationContext ctx, GlobalTransaction tx, Fqn parentFqn, Object childName, Node cn, boolean createUndoOps) throws Throwable
{
- acquireLocksWithTimeout(ctx, parentFqn, NodeLock.LockType.READ, false, false, false, false);
+ acquireLocksWithTimeout(ctx, parentFqn, NodeLock.LockType.READ, false, false, false, false, null, false);
return nextInterceptor(ctx);
}
protected Object handleEvictMethod(InvocationContext ctx, Fqn fqn) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, false, true, false, false);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, false, true, false, false, null, false);
return nextInterceptor(ctx);
}
protected Object handleGetKeyValueMethod(InvocationContext ctx, Fqn fqn, Object key, boolean sendNodeEvent) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false);
return nextInterceptor(ctx);
}
protected Object handleGetNodeMethod(InvocationContext ctx, Fqn fqn) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false);
return nextInterceptor(ctx);
}
protected Object handleGetKeysMethod(InvocationContext ctx, Fqn fqn) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false);
return nextInterceptor(ctx);
}
protected Object handleGetChildrenNamesMethod(InvocationContext ctx, Fqn fqn) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false);
return nextInterceptor(ctx);
}
protected Object handlePrintMethod(InvocationContext ctx, Fqn fqn) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false);
return nextInterceptor(ctx);
}
protected Object handleReleaseAllLocksMethod(InvocationContext ctx, Fqn fqn) throws Throwable
{
- acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false);
+ acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false);
return nextInterceptor(ctx);
}
private boolean acquireLocksWithTimeout(InvocationContext ctx, Fqn fqn, NodeLock.LockType lockType,
boolean createIfNotExists, boolean zeroLockTimeout,
- boolean acquireLockOnParent, boolean reverseRemoveCheck)
+ boolean acquireLockOnParent, boolean reverseRemoveCheck, List<Fqn> createdNodes, boolean markNewNodesAsDeleted)
throws InterruptedException
{
if (fqn == null || configuration.getIsolationLevel() == IsolationLevel.NONE) return false;
@@ -334,7 +345,7 @@
{
throw new TimeoutException("Unable to acquire lock on Fqn " + fqn + " after " + timeout + " millis");
}
- created = lock(ctx, fqn, lockType, createIfNotExists, timeout, acquireLockOnParent, reverseRemoveCheck);
+ created = lock(ctx, fqn, lockType, createIfNotExists, timeout, acquireLockOnParent, reverseRemoveCheck, createdNodes, markNewNodesAsDeleted);
firstTry = false;
}
while (createIfNotExists && peekNode(ctx, fqn, false, true, false) == null);// keep trying until we have the lock (fixes concurrent remove())
@@ -347,12 +358,14 @@
* 2) acquireWriteLockOnParent is true. If so AND {@link org.jboss.cache.Node#isLockForChildInsertRemove()} then a read
* lock will be aquired for the parent of the node.
*
- * @param createIfNotExists if true, then missing nodes will be cretaed on the fly. If false, method returns if we
- * reach a node that does not exists
- * @param reverseRemoveCheck see {@link #manageReverseRemove(org.jboss.cache.transaction.GlobalTransaction, org.jboss.cache.NodeSPI, boolean)}
+ * @param createIfNotExists if true, then missing nodes will be cretaed on the fly. If false, method returns if we
+ * reach a node that does not exists
+ * @param reverseRemoveCheck see {@link #manageReverseRemove(org.jboss.cache.transaction.GlobalTransaction, org.jboss.cache.NodeSPI, boolean)}
+ * @param createdNodes a list to which any nodes created can register their Fqns so that calling code is aware of which nodes have been newly created.
+ * @param markNewNodesAsDeleted
*/
private boolean lock(InvocationContext ctx, Fqn fqn, NodeLock.LockType lockType, boolean createIfNotExists, long timeout,
- boolean acquireWriteLockOnParent, boolean reverseRemoveCheck)
+ boolean acquireWriteLockOnParent, boolean reverseRemoveCheck, List<Fqn> createdNodes, boolean markNewNodesAsDeleted)
throws TimeoutException, LockingException, InterruptedException
{
Thread currentThread = Thread.currentThread();
@@ -380,6 +393,8 @@
currentNode = parent.addChildDirect(new Fqn(childName));
created = true;
if (trace) log.trace("Child node was null, so created child node " + childName);
+ if (createdNodes != null) createdNodes.add(currentNode.getFqn());
+ if (markNewNodesAsDeleted) currentNode.markAsDeleted(true);
}
else
{
@@ -478,7 +493,7 @@
}
/**
- * Used by {@link #lock(org.jboss.cache.InvocationContext, org.jboss.cache.Fqn, org.jboss.cache.lock.NodeLock.LockType, boolean, long, boolean, boolean)}.
+ * Used by lock()
* Determins whter an arbitrary node from the supplied fqn needs an write lock.
*/
private boolean writeLockNeeded(InvocationContext ctx, NodeLock.LockType lockType, int currentNodeIndex, boolean acquireWriteLockOnParent, boolean createIfNotExists, Fqn targetFqn, NodeSPI currentNode)
Modified: core/trunk/src/main/java/org/jboss/cache/invocation/RemoteCacheInvocationDelegate.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/invocation/RemoteCacheInvocationDelegate.java 2008-01-04 18:52:20 UTC (rev 4992)
+++ core/trunk/src/main/java/org/jboss/cache/invocation/RemoteCacheInvocationDelegate.java 2008-01-04 19:03:15 UTC (rev 4993)
@@ -73,12 +73,14 @@
{
if (log.isTraceEnabled())
log.trace("DataGravitationCleanup: Removing primary (" + primary + ") and backup (" + backup + ")");
- //primaryDataCleanup = MethodCallFactory.create(MethodDeclarations.removeNodeMethodLocal, primary, false);
+
getInvocationContext().getOptionOverrides().setCacheModeLocal(true);
- removeNode(primary);
- //backupDataCleanup = MethodCallFactory.create(MethodDeclarations.removeNodeMethodLocal, backup, false);
- getInvocationContext().getOptionOverrides().setCacheModeLocal(true);
- removeNode(backup);
+ if (!removeNode(primary))
+ {
+ // only attempt to clean up the backup if the primary did not exist - a waste of a call otherwise.
+ getInvocationContext().getOptionOverrides().setCacheModeLocal(true);
+ removeNode(backup);
+ }
}
else
{
Modified: core/trunk/src/test/java/org/jboss/cache/api/CacheAPIOptimisticTest.java
===================================================================
--- core/trunk/src/test/java/org/jboss/cache/api/CacheAPIOptimisticTest.java 2008-01-04 18:52:20 UTC (rev 4992)
+++ core/trunk/src/test/java/org/jboss/cache/api/CacheAPIOptimisticTest.java 2008-01-04 19:03:15 UTC (rev 4993)
@@ -1,9 +1,12 @@
package org.jboss.cache.api;
+import org.testng.annotations.Test;
+
/**
* Optimistically locked version of {@link org.jboss.cache.api.CacheAPITest}
*/
+ at Test(groups = "functional")
public class CacheAPIOptimisticTest extends CacheAPITest
{
public CacheAPIOptimisticTest()
Modified: core/trunk/src/test/java/org/jboss/cache/api/CacheAPITest.java
===================================================================
--- core/trunk/src/test/java/org/jboss/cache/api/CacheAPITest.java 2008-01-04 18:52:20 UTC (rev 4992)
+++ core/trunk/src/test/java/org/jboss/cache/api/CacheAPITest.java 2008-01-04 19:03:15 UTC (rev 4993)
@@ -2,6 +2,7 @@
import org.jboss.cache.Cache;
import org.jboss.cache.CacheFactory;
+import org.jboss.cache.CacheSPI;
import org.jboss.cache.DefaultCacheFactory;
import org.jboss.cache.Fqn;
import org.jboss.cache.Node;
@@ -12,11 +13,13 @@
import org.jboss.cache.notifications.annotation.NodeCreated;
import org.jboss.cache.notifications.event.Event;
import org.jboss.cache.transaction.GenericTransactionManagerLookup;
+import org.jboss.cache.util.CachePrinter;
import static org.testng.AssertJUnit.*;
import org.testng.annotations.AfterMethod;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
+import javax.transaction.TransactionManager;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
@@ -28,7 +31,7 @@
* @author <a href="mailto:manik at jboss.org">Manik Surtani</a>
*/
- at Test(groups = {"functional"})
+ at Test(groups = "functional")
public class CacheAPITest
{
private Cache<String, String> cache;
@@ -185,6 +188,8 @@
assertFalse(cache.getRoot().hasChild(fqn));
assertEquals(false, cache.removeNode(fqn));
+ System.out.println("Cache: " + CachePrinter.printCacheDetails(cache));
+
// Check that it's removed if it has a child
Fqn<String> child = Fqn.fromString("/test/fqn/child");
cache.getRoot().addChild(child);
@@ -350,6 +355,29 @@
assertTrue(cache.getRoot().getChildren().isEmpty());
}
+ public void testPhantomStructuralNodesOnRemove()
+ {
+ CacheSPI spi = (CacheSPI) cache;
+ assert spi.peek(Fqn.fromString("/a/b/c"), true, true) == null;
+ assert !spi.removeNode("/a/b/c");
+ assert spi.peek(Fqn.fromString("/a/b/c"), true, true) == null;
+ assert spi.peek(Fqn.fromString("/a/b"), true, true) == null;
+ assert spi.peek(Fqn.fromString("/a"), true, true) == null;
+ }
+
+ public void testPhantomStructuralNodesOnRemoveTransactional() throws Exception
+ {
+ CacheSPI spi = (CacheSPI) cache;
+ TransactionManager tm = spi.getTransactionManager();
+ assert spi.peek(Fqn.fromString("/a/b/c"), true, true) == null;
+ tm.begin();
+ assert !spi.removeNode("/a/b/c");
+ tm.commit();
+ assert spi.peek(Fqn.fromString("/a/b/c"), true, true) == null;
+ assert spi.peek(Fqn.fromString("/a/b"), true, true) == null;
+ assert spi.peek(Fqn.fromString("/a"), true, true) == null;
+ }
+
@CacheListener
public class Listener
{
Modified: core/trunk/src/test/java/org/jboss/cache/buddyreplication/GravitationCleanupTest.java
===================================================================
--- core/trunk/src/test/java/org/jboss/cache/buddyreplication/GravitationCleanupTest.java 2008-01-04 18:52:20 UTC (rev 4992)
+++ core/trunk/src/test/java/org/jboss/cache/buddyreplication/GravitationCleanupTest.java 2008-01-04 19:03:15 UTC (rev 4993)
@@ -15,10 +15,30 @@
Fqn fqn = Fqn.fromString("/a/b/c");
Object key = "key", value = "value";
- public void testStaleRegionOnDataOwner() throws Exception
+ public void testStaleRegionOnDataOwnerPessimistic() throws Exception
{
- caches = createCaches(1, 2, false, true, false);
+ testDataOwner(false);
+ }
+ public void testStaleRegionOnDataOwnerOptimistic() throws Exception
+ {
+ testDataOwner(true);
+ }
+
+ public void testStaleRegionOnBuddyPessimistic() throws Exception
+ {
+ testBuddy(false);
+ }
+
+ public void testStaleRegionOnBuddyOptimistic() throws Exception
+ {
+ testBuddy(true);
+ }
+
+ private void testDataOwner(boolean optimistic) throws Exception
+ {
+ caches = createCaches(1, 2, false, true, optimistic);
+
// add some stuff on the primary
CacheSPI dataOwner = caches.get(0);
CacheSPI buddy = caches.get(1);
@@ -52,4 +72,54 @@
assert dataOwner.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(buddy.getLocalAddress())), false) != null : "Should have backup node for buddy";
assert dataOwner.peek(BuddyManager.getBackupFqn(buddy.getLocalAddress(), fqn), false) != null : "Should have backup data";
}
+
+ private void testBuddy(boolean optimistic) throws Exception
+ {
+ caches = createCaches(1, 3, false, true, optimistic);
+
+ // add some stuff on the primary
+ CacheSPI dataOwner = caches.get(0);
+ CacheSPI buddy = caches.get(1);
+ CacheSPI thirdInstance = caches.get(2);
+
+ assertIsBuddy(dataOwner, buddy, true);
+ assertIsBuddy(buddy, thirdInstance, true);
+ assertIsBuddy(thirdInstance, dataOwner, true);
+
+ dataOwner.put(fqn, key, value);
+
+ System.out.println("dataOwner: " + CachePrinter.printCacheLockingInfo(dataOwner));
+ System.out.println("buddy: " + CachePrinter.printCacheLockingInfo(buddy));
+ System.out.println("thirdInstance: " + CachePrinter.printCacheLockingInfo(thirdInstance));
+
+ assert dataOwner.peek(fqn, false) != null : "Should have data";
+ assert dataOwner.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(thirdInstance.getLocalAddress())), false) != null : "Should have backup node for buddy";
+ assert dataOwner.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(dataOwner.getLocalAddress())), false) == null : "Should NOT have backup node for self!";
+ assert dataOwner.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(buddy.getLocalAddress())), false) == null : "Should NOT have backup node for 2nd instance!";
+
+ assert buddy.peek(fqn, false) == null : "Should not have data";
+ assert buddy.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(buddy.getLocalAddress())), false) == null : "Should NOT have backup node for self!";
+ assert buddy.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(dataOwner.getLocalAddress())), false) != null : "Should have backup node for buddy";
+ assert buddy.peek(BuddyManager.getBackupFqn(dataOwner.getLocalAddress(), fqn), false) != null : "Should have backup data";
+
+ // now do a gravitate call.
+ assert thirdInstance.get(fqn, key).equals(value) : "Data should have gravitated!";
+
+ System.out.println("dataOwner: " + CachePrinter.printCacheLockingInfo(dataOwner));
+ System.out.println("buddy: " + CachePrinter.printCacheLockingInfo(buddy));
+ System.out.println("thirdInstance: " + CachePrinter.printCacheLockingInfo(thirdInstance));
+
+ assert thirdInstance.peek(fqn, false) != null : "Should have data";
+ assert thirdInstance.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(buddy.getLocalAddress())), false) != null : "Should have backup node for buddy";
+ assert thirdInstance.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(thirdInstance.getLocalAddress())), false) == null : "Should NOT have backup node for self!";
+
+ assert dataOwner.peek(fqn, false) == null : "Should not have data";
+ assert dataOwner.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(dataOwner.getLocalAddress())), false) == null : "Should NOT have backup node for self!";
+ assert dataOwner.peek(new Fqn(BuddyManager.BUDDY_BACKUP_SUBTREE_FQN, BuddyManager.getGroupNameFromAddress(thirdInstance.getLocalAddress())), false) != null : "Should have backup node for buddy";
+ assert dataOwner.peek(BuddyManager.getBackupFqn(thirdInstance.getLocalAddress(), fqn), false) != null : "Should have backup data";
+ assert buddy.peek(fqn, false) == null : "Should not have data";
+ assert buddy.peek(fqn.getParent(), false) == null : "Should not have any part of the data";
+ assert buddy.peek(BuddyManager.getBackupFqn(dataOwner.getLocalAddress(), fqn), false) == null : "Should NOT have backup data";
+
+ }
}
More information about the jbosscache-commits
mailing list