Author: mircea.markus Date: 2008-01-22 09:42:59 -0500 (Tue, 22 Jan 2008) New Revision: 5190 Modified: core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java core/trunk/src/main/java/org/jboss/cache/transaction/TransactionTable.java core/trunk/src/test/java/org/jboss/cache/lock/pessimistic/ConcurrentPutRemoveTest.java Log: http://jira.jboss.com/jira/browse/JBCACHE-1165 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-22 13:30:36 UTC (rev 5189) +++ core/trunk/src/main/java/org/jboss/cache/interceptors/PessimisticLockInterceptor.java 2008-01-22 14:42:59 UTC (rev 5190) @@ -118,7 +118,7 @@ } else { - acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, true, false, false, true, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, true, false, false, true, null); } 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, null, false); + acquireLocksWithTimeout(ctx, fqn, lockType, false, false, false, false, null); 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, null, false); + lock(ctx, from, NodeLock.LockType.WRITE, false, timeout, true, false, null); 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, null, false); + lock(ctx, to, NodeLock.LockType.READ, false, timeout, false, false, null); acquireLocksOnChildren(peekNode(ctx, to, false, true, false), NodeLock.LockType.READ, ctx); } Object retValue = nextInterceptor(ctx); @@ -234,7 +234,7 @@ 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); + boolean created = acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, true, false, true, false, createdNodes); if (ctx.getGlobalTransaction() != null) { TransactionEntry entry = tx_table.get(ctx.getGlobalTransaction()); @@ -263,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, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, true, true, false, true, null); return nextInterceptor(ctx); } @@ -274,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, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, false, false, false, false, null); 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, null, false); + acquireLocksWithTimeout(ctx, parentFqn, NodeLock.LockType.READ, false, false, false, false, null); return nextInterceptor(ctx); } protected Object handleEvictMethod(InvocationContext ctx, Fqn fqn) throws Throwable { - acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, false, true, false, false, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.WRITE, false, true, false, false, null); 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, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null); return nextInterceptor(ctx); } protected Object handleGetNodeMethod(InvocationContext ctx, Fqn fqn) throws Throwable { - acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null); return nextInterceptor(ctx); } protected Object handleGetKeysMethod(InvocationContext ctx, Fqn fqn) throws Throwable { - acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null); return nextInterceptor(ctx); } protected Object handleGetChildrenNamesMethod(InvocationContext ctx, Fqn fqn) throws Throwable { - acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null); return nextInterceptor(ctx); } protected Object handlePrintMethod(InvocationContext ctx, Fqn fqn) throws Throwable { - acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null); return nextInterceptor(ctx); } protected Object handleReleaseAllLocksMethod(InvocationContext ctx, Fqn fqn) throws Throwable { - acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null, false); + acquireLocksWithTimeout(ctx, fqn, NodeLock.LockType.READ, false, false, false, false, null); return nextInterceptor(ctx); } private boolean acquireLocksWithTimeout(InvocationContext ctx, Fqn fqn, NodeLock.LockType lockType, boolean createIfNotExists, boolean zeroLockTimeout, - boolean acquireLockOnParent, boolean reverseRemoveCheck, List<Fqn> createdNodes, boolean markNewNodesAsDeleted) + boolean acquireLockOnParent, boolean reverseRemoveCheck, List<Fqn> createdNodes) throws InterruptedException { if (fqn == null || configuration.getIsolationLevel() == IsolationLevel.NONE) return false; @@ -345,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, createdNodes, markNewNodesAsDeleted); + created = lock(ctx, fqn, lockType, createIfNotExists, timeout, acquireLockOnParent, reverseRemoveCheck, createdNodes); firstTry = false; } while (createIfNotExists && peekNode(ctx, fqn, false, true, false) == null);// keep trying until we have the lock (fixes concurrent remove()) @@ -362,10 +362,9 @@ * 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, List<Fqn> createdNodes, boolean markNewNodesAsDeleted) + boolean acquireWriteLockOnParent, boolean reverseRemoveCheck, List<Fqn> createdNodes) throws TimeoutException, LockingException, InterruptedException { Thread currentThread = Thread.currentThread(); @@ -391,11 +390,10 @@ if (createIfNotExists) { // if the new node is to be marked as deleted, do not notify! - currentNode = parent.addChildDirect(new Fqn(childName), !markNewNodesAsDeleted); + currentNode = parent.addChildDirect(new Fqn(childName), true); created = true; - if (trace) log.trace("Child node was null, so created child node " + childName); + if (trace) log.trace("Child node was null, so created child node " + childName + System.identityHashCode(currentNode)); if (createdNodes != null) createdNodes.add(currentNode.getFqn()); - if (markNewNodesAsDeleted) currentNode.markAsDeleted(true); } else { @@ -414,11 +412,10 @@ lockTypeRequired = NodeLock.LockType.WRITE; } - manageReverseRemove(gtx, currentNode, reverseRemoveCheck); - // actually acquire the lock we need. This method blocks. acquireNodeLock(currentNode, owner, gtx, lockTypeRequired, timeout); + manageReverseRemove(gtx, currentNode, reverseRemoveCheck); // make sure the lock we acquired isn't on a deleted node/is an orphan!! // look into invalidated nodes as well NodeSPI repeek = peekNode(ctx, currentNode.getFqn(), true, true, true); @@ -433,14 +430,14 @@ if (peekNode(ctx, parent.getFqn(), true, true, true) == null) { // crap! - if (trace) - log.trace("Parent has been deleted again. Go through the lock method all over again."); + if (trace) log.trace("Parent has been deleted again. Go through the lock method all over again."); currentNode = rootNode; parent = null; } else { - // do the loop again, but don't assign child_node to currentNode so that child_node is processed again. + currentNode = parent; + parent = null; if (System.currentTimeMillis() > expiryTime) { throw new TimeoutException("Unable to acquire lock on child node " + new Fqn(currentNode.getFqn(), childName) + " after " + timeout + " millis."); @@ -565,7 +562,7 @@ */ private void manageReverseRemove(GlobalTransaction gtx, NodeSPI childNode, boolean reverseRemoveCheck) { - boolean needToReverseRemove = reverseRemoveCheck && childNode.isDeleted() && tx_table.get(gtx).getRemovedNodes().contains(childNode.getFqn()); + boolean needToReverseRemove = reverseRemoveCheck && childNode.isDeleted() && tx_table.isNodeRemovedInTx(gtx,childNode.getFqn()); if (gtx != null && needToReverseRemove) { childNode.markAsDeleted(false); Modified: core/trunk/src/main/java/org/jboss/cache/transaction/TransactionTable.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/transaction/TransactionTable.java 2008-01-22 13:30:36 UTC (rev 5189) +++ core/trunk/src/main/java/org/jboss/cache/transaction/TransactionTable.java 2008-01-22 14:42:59 UTC (rev 5190) @@ -9,6 +9,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.jboss.cache.CacheException; +import org.jboss.cache.Fqn; import org.jboss.cache.lock.NodeLock; import org.jboss.cache.marshall.MethodCall; @@ -286,4 +287,10 @@ throw e; } } + + public boolean isNodeRemovedInTx(GlobalTransaction gtx, Fqn fqn) + { + TransactionEntry te = get(gtx); + return te != null && te.getRemovedNodes().contains(fqn); + } } Modified: core/trunk/src/test/java/org/jboss/cache/lock/pessimistic/ConcurrentPutRemoveTest.java =================================================================== --- core/trunk/src/test/java/org/jboss/cache/lock/pessimistic/ConcurrentPutRemoveTest.java 2008-01-22 13:30:36 UTC (rev 5189) +++ core/trunk/src/test/java/org/jboss/cache/lock/pessimistic/ConcurrentPutRemoveTest.java 2008-01-22 14:42:59 UTC (rev 5190) @@ -16,6 +16,7 @@ import javax.transaction.TransactionManager; import java.util.ArrayList; import java.util.List; +import java.net.URL; // This is disabled because the fix is not absolute and will require pretty bug architectural changes. // There is an edge case where a NodeNotFoundException may occur, and this is due to parent nodes not being @@ -24,7 +25,7 @@ // The problem is in the way READ_COMMITTED is implemented, i.e., writers are not blocked by readers and this // allows a reader to hold a lock when a writer comes in and deletes the node in question. -@Test(groups = {"functional"}, enabled = false) +@Test(groups = {"functional"}, enabled = true) // Known issue - See JBCACHE-1164 and JBCACHE-1165 public class ConcurrentPutRemoveTest { @@ -60,7 +61,7 @@ } } - @Test(invocationCount = 50, enabled = false) + @Test(invocationCount = 10, enabled = true) public void testLock() throws Exception { for (int x = 0; x < 2; x++)