[infinispan-commits] Infinispan SVN: r1141 - in trunk/core/src: main/java/org/infinispan/container/entries and 1 other directories.

[infinispan-commits at lists.jboss.org]

Author: manik.surtani at jboss.com
Date: 2009-11-11 14:29:25 -0500 (Wed, 11 Nov 2009)
New Revision: 1141

Modified:
   trunk/core/src/main/java/org/infinispan/container/EntryFactoryImpl.java
   trunk/core/src/main/java/org/infinispan/container/entries/RepeatableReadEntry.java
   trunk/core/src/test/java/org/infinispan/api/mvcc/repeatable_read/WriteSkewTest.java
Log:
[ISPN-259] (Concurrent puts fail with writeSkew) - Fixed incorrect write skew check routine in RepeatableReadEntry

Modified: trunk/core/src/main/java/org/infinispan/container/EntryFactoryImpl.java
===================================================================
--- trunk/core/src/main/java/org/infinispan/container/EntryFactoryImpl.java	2009-11-11 19:21:00 UTC (rev 1140)
+++ trunk/core/src/main/java/org/infinispan/container/EntryFactoryImpl.java	2009-11-11 19:29:25 UTC (rev 1141)
@@ -87,20 +87,20 @@
          // do not bother wrapping though if this is not in a tx.  repeatable read etc are all meaningless unless there is a tx.
          if (useRepeatableRead && ctx.isInTxScope()) {
             MVCCEntry mvccEntry = cacheEntry == null ?
-                     createWrappedEntry(key, null, false, false, -1) :
-                     createWrappedEntry(key, cacheEntry.getValue(), false, false, cacheEntry.getLifespan());
-               if (mvccEntry != null) ctx.putLookedUpEntry(key, mvccEntry);
-               return mvccEntry;
+                  createWrappedEntry(key, null, false, false, -1) :
+                  createWrappedEntry(key, cacheEntry.getValue(), false, false, cacheEntry.getLifespan());
+            if (mvccEntry != null) ctx.putLookedUpEntry(key, mvccEntry);
+            return mvccEntry;
          }
          // if not in transaction and repeatable read, or simply read committed (regardless of whether in TX or not), do not wrap
          if (cacheEntry != null) ctx.putLookedUpEntry(key, cacheEntry);
-            return cacheEntry;
+         return cacheEntry;
       } else {
          if (trace) log.trace("Key is already in context");
          return cacheEntry;
       }
    }
-   
+
    public final MVCCEntry wrapEntryForWriting(InvocationContext ctx, Object key, boolean createIfAbsent, boolean forceLockIfAbsent, boolean alreadyLocked, boolean forRemoval) throws InterruptedException {
       return wrapEntryForWriting(ctx, key, null, createIfAbsent, forceLockIfAbsent, alreadyLocked, forRemoval);
    }
@@ -203,12 +203,20 @@
       if (!ctx.hasLockedKey(key) && !shouldSkipLocking) {
          if (lockManager.lockAndRecord(key, ctx)) {
             // successfully locked!
+            if (trace) log.trace("Successfully acquired lock!");
             return true;
          } else {
             Object owner = lockManager.getOwner(key);
             throw new TimeoutException("Unable to acquire lock after [" + Util.prettyPrintTime(getLockAcquisitionTimeout(ctx)) + "] on key [" + key + "] for requestor [" +
                   ctx.getLockOwner() + "]! Lock held by [" + owner + "]");
          }
+      } else {
+         if (trace) {
+            if (shouldSkipLocking)
+               log.trace("SKIP_LOCKING flag used!");
+            else
+               log.trace("Already own lock for entry");
+         }
       }
 
       return false;

Modified: trunk/core/src/main/java/org/infinispan/container/entries/RepeatableReadEntry.java
===================================================================
--- trunk/core/src/main/java/org/infinispan/container/entries/RepeatableReadEntry.java	2009-11-11 19:21:00 UTC (rev 1140)
+++ trunk/core/src/main/java/org/infinispan/container/entries/RepeatableReadEntry.java	2009-11-11 19:29:25 UTC (rev 1141)
@@ -47,16 +47,18 @@
       setChanged();
 
       if (writeSkewCheck) {
-         // check for write skew.
-         Object actualValue = container.get(key);
+      // check for write skew.
+         InternalCacheEntry ice = container.get(key);
+         Object actualValue = ice == null ? null : ice.getValue();
 
+         // Note that this identity-check is intentional.  We don't *want* to call actualValue.equals() since that defeats the purpose.
+         // the implicit "versioning" we have in R_R creates a new wrapper "value" instance for every update.
          if (actualValue != null && actualValue != value) {
             String errormsg = new StringBuilder().append("Detected write skew on key [").append(getKey()).append("].  Another process has changed the entry since we last read it!").toString();
             if (log.isWarnEnabled()) log.warn(errormsg + ".  Unable to copy entry for update.");
             throw new CacheException(errormsg);
          }
       }
-
       // make a backup copy
       oldValue = value;
    }

Modified: trunk/core/src/test/java/org/infinispan/api/mvcc/repeatable_read/WriteSkewTest.java
===================================================================
--- trunk/core/src/test/java/org/infinispan/api/mvcc/repeatable_read/WriteSkewTest.java	2009-11-11 19:21:00 UTC (rev 1140)
+++ trunk/core/src/test/java/org/infinispan/api/mvcc/repeatable_read/WriteSkewTest.java	2009-11-11 19:29:25 UTC (rev 1141)
@@ -1,6 +1,7 @@
 package org.infinispan.api.mvcc.repeatable_read;
 
 import org.infinispan.Cache;
+import org.infinispan.CacheException;
 import org.infinispan.api.mvcc.LockAssert;
 import org.infinispan.config.Configuration;
 import org.infinispan.context.InvocationContextContainer;
@@ -32,7 +33,7 @@
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 
- at Test(groups = {"functional", "mvcc"}, testName = "api.mvcc.repeatable_read.WriteSkewTest")
+ at Test(groups = "functional", testName = "api.mvcc.repeatable_read.WriteSkewTest")
 public class WriteSkewTest extends AbstractInfinispanTest {
    private static final Log log = LogFactory.getLog(WriteSkewTest.class);
    protected TransactionManager tm;
@@ -201,6 +202,12 @@
          assert "v3".equals(cache.get("k")) : "W2 should have overwritten W1's work!";
 
          assertNoLocks();
+      } else {
+         Collection<Exception> combined = new HashSet<Exception>(w1exceptions);
+         combined.addAll(w2exceptions);
+         assert !combined.isEmpty();
+         assert combined.size() == 1;
+         assert combined.iterator().next() instanceof CacheException;
       }
    }