Author: manik.surtani(a)jboss.com Date: 2009-03-18 05:45:27 -0400 (Wed, 18 Mar 2009) New Revision: 7908 Added: core/trunk/src/main/java/org/jboss/cache/mvcc/NullMarkerNodeForRemoval.java Modified: core/trunk/src/main/java/org/jboss/cache/AbstractNodeFactory.java core/trunk/src/main/java/org/jboss/cache/NodeFactory.java core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeFactory.java core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeHelper.java core/trunk/src/main/java/org/jboss/cache/mvcc/NullMarkerNode.java core/trunk/src/main/java/org/jboss/cache/mvcc/RepeatableReadNode.java core/trunk/src/test/java/org/jboss/cache/api/CacheAPITest.java core/trunk/src/test/java/org/jboss/cache/api/mvcc/repeatable_read/CacheAPIMVCCTest.java Log: JBCACHE-1493 - REPEATABLE_READ inconsistent with write skew disabled Modified: core/trunk/src/main/java/org/jboss/cache/AbstractNodeFactory.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/AbstractNodeFactory.java 2009-03-18 08:43:29 UTC (rev 7907) +++ core/trunk/src/main/java/org/jboss/cache/AbstractNodeFactory.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -125,6 +125,11 @@ throw new UnsupportedOperationException("Unsupported in this implementation (" + getClass().getSimpleName() + ")!"); } + public ReadCommittedNode createWrappedNodeForRemoval(Fqn fqn, InternalNode<K, V> node, InternalNode<K, V> parent) + { + throw new UnsupportedOperationException("Unsupported in this implementation (" + getClass().getSimpleName() + ")!"); + } + public NodeSPI<K, V> createRootNode() { return createNode(Fqn.ROOT, null); Modified: core/trunk/src/main/java/org/jboss/cache/NodeFactory.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/NodeFactory.java 2009-03-18 08:43:29 UTC (rev 7907) +++ core/trunk/src/main/java/org/jboss/cache/NodeFactory.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -38,6 +38,8 @@ { ReadCommittedNode createWrappedNode(InternalNode<K, V> node, InternalNode<K, V> parent); + ReadCommittedNode createWrappedNodeForRemoval(Fqn fqn, InternalNode<K, V> node, InternalNode<K, V> parent); + WorkspaceNode<K, V> createWrappedNode(NodeSPI<K, V> dataNode, TransactionWorkspace workspace); /** Modified: core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeFactory.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeFactory.java 2009-03-18 08:43:29 UTC (rev 7907) +++ core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeFactory.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -43,7 +43,6 @@ public class MVCCNodeFactory<K, V> extends AbstractNodeFactory<K, V> { private boolean useRepeatableRead; - private static final NullMarkerNode NULL_MARKER = new NullMarkerNode(); private static final Log log = LogFactory.getLog(MVCCNodeFactory.class); private static final boolean trace = log.isTraceEnabled(); private boolean lockChildForInsertRemove; @@ -68,11 +67,47 @@ * @return a ReadCommittedNode */ @Override - @SuppressWarnings("unchecked") public ReadCommittedNode createWrappedNode(InternalNode<K, V> node, InternalNode<K, V> parent) { - if (node == null) return useRepeatableRead ? NULL_MARKER : null; - ReadCommittedNode rcn = useRepeatableRead ? new RepeatableReadNode(node, parent) : new ReadCommittedNode(node, parent); + return createWrappedNode(null, node, parent, false); + } + + @Override + public ReadCommittedNode createWrappedNodeForRemoval(Fqn fqn, InternalNode<K, V> node, InternalNode<K, V> parent) + { + return createWrappedNode(fqn, node, parent, true); + } + + @SuppressWarnings("unchecked") + private ReadCommittedNode createWrappedNode(Fqn fqn, InternalNode<K, V> node, InternalNode<K, V> parent, boolean forRemoval) + { + ReadCommittedNode rcn; + + if (node == null) + { + if (useRepeatableRead) + { + if (forRemoval) + { + // create but do not return this just yet as it needs to be initialized + rcn = new NullMarkerNodeForRemoval(parent, fqn); + } + else + { + return NullMarkerNode.getInstance(); + } + } + else + { + // if we are using read-committed, just return a null + return null; + } + } + else + { + rcn = useRepeatableRead ? new RepeatableReadNode(node, parent) : new ReadCommittedNode(node, parent); + } + rcn.initialize(configuration, invocationContextContainer, componentRegistry, interceptorChain); rcn.injectDependencies(cache); return rcn; Modified: core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeHelper.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeHelper.java 2009-03-18 08:43:29 UTC (rev 7907) +++ core/trunk/src/main/java/org/jboss/cache/mvcc/MVCCNodeHelper.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -85,10 +85,8 @@ * Attempts to provide the context with a set of wrapped nodes based on the Collection of fqns passed in. If the * nodes already exist in the context then the node is not wrapped again. * <p/> - * {@link InternalNode}s are wrapped using {@link org.jboss.cache.NodeFactory#createWrappedNode(org.jboss.cache.InternalNode, - * org.jboss.cache.InternalNode)} and as such, null internal nodes are treated according to isolation level used. - * See {@link org.jboss.cache.NodeFactory#createWrappedNode(org.jboss.cache.InternalNode, - * org.jboss.cache.InternalNode)} for details on this behaviour. + * {@link InternalNode}s are wrapped using {@link org.jboss.cache.NodeFactory#createWrappedNode(org.jboss.cache.InternalNode, org.jboss.cache.InternalNode)} and as such, null internal nodes are treated according to isolation level used. + * See {@link org.jboss.cache.NodeFactory#createWrappedNode(org.jboss.cache.InternalNode, org.jboss.cache.InternalNode)} for details on this behaviour. * <p/> * Note that if the context has the {@link org.jboss.cache.config.Option#isForceWriteLock()} option set, then write * locks are acquired and the node is copied. @@ -404,9 +402,9 @@ @SuppressWarnings("unchecked") private ReadCommittedNode wrapAndPutInContext(InvocationContext ctx, Fqn fqn, boolean forUpdate) { ReadCommittedNode node = (ReadCommittedNode) ctx.lookUpNode(fqn); - if (node == null) { + if (node == null || node.isNullNode()) { InternalNode[] nodes = dataContainer.peekInternalNodeAndDirectParent(fqn, false); - node = nodeFactory.createWrappedNode(nodes[0], nodes[1]); + node = nodeFactory.createWrappedNodeForRemoval(fqn, nodes[0], nodes[1]); ctx.putLookedUpNode(fqn, node); } Modified: core/trunk/src/main/java/org/jboss/cache/mvcc/NullMarkerNode.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/mvcc/NullMarkerNode.java 2009-03-18 08:43:29 UTC (rev 7907) +++ core/trunk/src/main/java/org/jboss/cache/mvcc/NullMarkerNode.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -30,41 +30,21 @@ * @author Manik Surtani (<a href="mailto:manik AT jboss DOT org">manik AT jboss DOT org</a>) * @since 3.0 */ -public class NullMarkerNode extends RepeatableReadNode +public class NullMarkerNode extends NullMarkerNodeForRemoval { - public NullMarkerNode() + private static final NullMarkerNode INSTANCE = new NullMarkerNode(); + + private NullMarkerNode() { super(null, null); } - /** - * @return always returns true - */ - @Override - public boolean isNullNode() + public static NullMarkerNode getInstance() { - return true; + return INSTANCE; } /** - * @return always returns true so that any get commands, upon getting this node, will ignore the node as though it were removed. - */ - @Override - public boolean isDeleted() - { - return true; - } - - /** - * @return always returns true so that any get commands, upon getting this node, will ignore the node as though it were invalid. - */ - @Override - public boolean isValid() - { - return false; - } - - /** * A no-op. */ @Override Added: core/trunk/src/main/java/org/jboss/cache/mvcc/NullMarkerNodeForRemoval.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/mvcc/NullMarkerNodeForRemoval.java (rev 0) +++ core/trunk/src/main/java/org/jboss/cache/mvcc/NullMarkerNodeForRemoval.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -0,0 +1,90 @@ +package org.jboss.cache.mvcc; + +import org.jboss.cache.DataContainer; +import org.jboss.cache.Fqn; +import org.jboss.cache.InternalNode; +import org.jboss.cache.InvocationContext; + +import java.util.Collections; +import java.util.Map; +import java.util.Set; + +/** + * A specific type of null marker node, used for removal of nodes that don't exist + * + * @author Manik Surtani + * @since 3.1.0 + */ +public class NullMarkerNodeForRemoval extends RepeatableReadNode +{ + private Fqn fqn; + + public NullMarkerNodeForRemoval(InternalNode parent, Fqn fqn) + { + super(null, parent); + this.fqn = fqn; + } + + @Override + public Fqn getFqn() + { + return fqn; + } + + /** + * @return always returns true + */ + @Override + public boolean isNullNode() + { + return true; + } + + /** + * @return always returns true so that any get commands, upon getting this node, will ignore the node as though it were invalid. + */ + @Override + public boolean isValid() + { + return false; + } + + /** + * @return always returns true so that any get commands, upon getting this node, will ignore the node as though it were removed. + */ + @Override + public boolean isDeleted() + { + return true; + } + + @Override + protected void updateNode(Fqn fqn, InvocationContext ctx, DataContainer dataContainer) + { + // no-op since the only updates that are allowed to happen here are the removal of the node, which only affects the parent. + } + + @Override + public Map getDataDirect() + { + return Collections.emptyMap(); + } + + @Override + public Set getChildrenNamesDirect() + { + return Collections.emptySet(); + } + + @Override + public Set getChildrenDirect() + { + return Collections.emptySet(); + } + + @Override + public void setValid(boolean valid, boolean recursive) + { + // no-op + } +} Modified: core/trunk/src/main/java/org/jboss/cache/mvcc/RepeatableReadNode.java =================================================================== --- core/trunk/src/main/java/org/jboss/cache/mvcc/RepeatableReadNode.java 2009-03-18 08:43:29 UTC (rev 7907) +++ core/trunk/src/main/java/org/jboss/cache/mvcc/RepeatableReadNode.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -71,8 +71,13 @@ // make a backup copy backup = node; - node = backup.copy(); + node = copyNode(backup); } + + private InternalNode copyNode(InternalNode nodeToCopy) + { + return nodeToCopy == null ? null : nodeToCopy.copy(); + } @Override @SuppressWarnings("unchecked") Modified: core/trunk/src/test/java/org/jboss/cache/api/CacheAPITest.java =================================================================== --- core/trunk/src/test/java/org/jboss/cache/api/CacheAPITest.java 2009-03-18 08:43:29 UTC (rev 7907) +++ core/trunk/src/test/java/org/jboss/cache/api/CacheAPITest.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -35,7 +35,7 @@ @Test(groups = {"functional", "pessimistic"}, sequential = true, testName = "api.CacheAPITest") public class CacheAPITest extends AbstractSingleCacheTest { - private CacheSPI<String, String> cache; + protected CacheSPI<String, String> cache; private List<String> events; public CacheSPI createCache() Modified: core/trunk/src/test/java/org/jboss/cache/api/mvcc/repeatable_read/CacheAPIMVCCTest.java =================================================================== --- core/trunk/src/test/java/org/jboss/cache/api/mvcc/repeatable_read/CacheAPIMVCCTest.java 2009-03-18 08:43:29 UTC (rev 7907) +++ core/trunk/src/test/java/org/jboss/cache/api/mvcc/repeatable_read/CacheAPIMVCCTest.java 2009-03-18 09:45:27 UTC (rev 7908) @@ -6,7 +6,10 @@ import org.jboss.cache.lock.IsolationLevel; import org.testng.annotations.Test; +import javax.transaction.Transaction; +import javax.transaction.TransactionManager; + /** * MVCC version of {@link org.jboss.cache.api.CacheAPITest} */ @@ -25,4 +28,18 @@ { return NodeLockingScheme.MVCC; } + + public void testWriteSkewOnRemovalOfNullNode() throws Exception + { + TransactionManager tm = cache.getTransactionManager(); + tm.begin(); + cache.getNode("/a"); + Transaction tx = tm.suspend(); + cache.put("/a", "k", "v2"); + assert cache.get("/a", "k").equals("v2"); + tm.resume(tx); + cache.removeNode("/a"); + tx.commit(); + assert cache.getNode("/a") == null; // this fails + } } \ No newline at end of file