[jbosscache-commits] JBoss Cache SVN: r5371 - in core/trunk/src: test/java/org/jboss/cache/lock and 1 other directory.

[jbosscache-commits at lists.jboss.org]

Author: mircea.markus
Date: 2008-02-26 07:39:28 -0500 (Tue, 26 Feb 2008)
New Revision: 5371

Modified:
   core/trunk/src/main/java/org/jboss/cache/lock/IdentityLock.java
   core/trunk/src/main/java/org/jboss/cache/lock/LockMap.java
   core/trunk/src/test/java/org/jboss/cache/lock/IdentityLockTest.java
Log:
Fixed an concurrency bug introduced in 2.1.4.CR4.
The issues is caused by the fact that the list of readLockOwners returned by LockMap is not guarded against modifications. This might cause ConcurrentModificationExceptions being raised at different points in code (basically on any usage of  IdentityLock.getReadOwners)

Modified: core/trunk/src/main/java/org/jboss/cache/lock/IdentityLock.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/lock/IdentityLock.java	2008-02-21 04:28:29 UTC (rev 5370)
+++ core/trunk/src/main/java/org/jboss/cache/lock/IdentityLock.java	2008-02-26 12:39:28 UTC (rev 5371)
@@ -145,6 +145,11 @@
       return map_.writerOwner();
    }
 
+   public LockMap getLockMap()
+   {
+      return map_;
+   }
+
    /**
     * Acquire a write lock with a timeout of <code>timeout</code> milliseconds.
     * Note that if the current owner owns a read lock, it will be upgraded
@@ -456,10 +461,6 @@
          sb.append("read owners=").append(read_owners);
          printed_read_owners = true;
       }
-      else
-      {
-         read_owners = null;
-      }
 
       Object write_owner = lock_ != null ? getWriterOwner() : null;
       if (write_owner != null)

Modified: core/trunk/src/main/java/org/jboss/cache/lock/LockMap.java
===================================================================
--- core/trunk/src/main/java/org/jboss/cache/lock/LockMap.java	2008-02-21 04:28:29 UTC (rev 5370)
+++ core/trunk/src/main/java/org/jboss/cache/lock/LockMap.java	2008-02-26 12:39:28 UTC (rev 5371)
@@ -6,10 +6,7 @@
  */
 package org.jboss.cache.lock;
 
-import java.util.Collection;
-import java.util.Collections;
-import java.util.LinkedList;
-import java.util.List;
+import java.util.*;
 
 /**
  * Provide lock ownership mapping.
@@ -116,8 +113,16 @@
     */
    public Collection<Object> readerOwners()
    {
-      return readOwnerList_;
-//      return Collections.unmodifiableSet(readOwnerList_);
+      //make a defense copy, otherwise ConcurrentModificationException might appear while client code is iterating
+      // the original map (see IdentityLockTest.testConcurrentModificationOfReadLocksAndToString)
+      List readOwnersListCopy;
+      synchronized (readOwnerList_) //readOwnerList_ is a sync list, make sure noone modifies it while we make a copy of it.
+      {
+        readOwnersListCopy = new ArrayList(readOwnerList_);
+        readOwnersListCopy.addAll(readOwnerList_);
+      }
+      return Collections.unmodifiableList(readOwnersListCopy);
+//      return readOwnerList_;
    }
 
    public void releaseReaderOwners(LockStrategy lock)

Modified: core/trunk/src/test/java/org/jboss/cache/lock/IdentityLockTest.java
===================================================================
--- core/trunk/src/test/java/org/jboss/cache/lock/IdentityLockTest.java	2008-02-21 04:28:29 UTC (rev 5370)
+++ core/trunk/src/test/java/org/jboss/cache/lock/IdentityLockTest.java	2008-02-26 12:39:28 UTC (rev 5371)
@@ -532,4 +532,66 @@
       logger_.info("-- [" + Thread.currentThread() + "]: " + msg);
    }
 
+   /**
+    * When IdentityLock.toString is called and readLockOwners are modified an ConcurrentModificationException exception
+    * might be thrown.  
+    */
+   public void testConcurrentModificationOfReadLocksAndToString() throws Exception
+   {
+      final IdentityLock iLock = (IdentityLock) lock_;
+      final LockMap lockMap = iLock.getLockMap();
+      final int opCount = 100000;
+      final Thread readLockChanger = new Thread()
+      {
+         public void run()
+         {
+            for (int i = 0; i < opCount; i++)
+            {
+               if (i%10 ==0) System.out.println("readLockChanger loop# " + i);
+               lockMap.addReader(new Object());
+            }
+         }
+      };
+
+      final boolean[] flags = new boolean[]{false, false}; //{testFailure, stopTheThread}
+      Thread toStringProcessor = new Thread()
+      {
+         public void run()
+         {
+            long initialTime = System.currentTimeMillis();
+            for (int i = 0; i < opCount; i++)
+            {
+               if (i%10 ==0)
+               {
+                  System.out.println("toStringProcessor loop# " + i + ", " + (System.currentTimeMillis() - initialTime));
+                  initialTime = System.currentTimeMillis();
+               }
+               try
+               {
+                  iLock.toString(new StringBuffer(), false);
+               } catch (Exception e)
+               {
+                  e.printStackTrace();
+                  flags[0] = true;
+                  break;
+               }
+               if (flags[1]) break;
+            }
+         }
+      };
+      toStringProcessor.start();
+      System.out.println("toStringProcessor started");
+      readLockChanger.start();
+      System.out.println("readLockChanger started");
+
+      readLockChanger.join();
+      flags[1] = true;//stopping the toStringProcessor
+      System.out.println("readLockChanger stopped");
+      
+      toStringProcessor.join();
+      System.out.println("toStringProcessor stopped");
+
+      System.out.println("flags[0]=" + flags[0]);
+      assertFalse(flags[0]);
+   }
 }