jackrabbit-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ju...@apache.org
Subject svn commit: r1000947 - in /jackrabbit/trunk/jackrabbit-core/src: main/java/org/apache/jackrabbit/core/state/ test/java/org/apache/jackrabbit/core/state/
Date Fri, 24 Sep 2010 16:38:46 GMT
Author: jukka
Date: Fri Sep 24 16:38:45 2010
New Revision: 1000947

URL: http://svn.apache.org/viewvc?rev=1000947&view=rev
Log:
JCR-2753: Deadlock in DefaultISMLocking

Restore the earlier revisions 995411 and 995412, and added the proposed patch that makes the
lock re-entrant for related readers even after a write lock has been downgraded.

Documented this reentrancy requirement in ISMLocking javadocs and modified the DefaultISMLockingDeadlockTest
case to check for this case.

Modified:
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/DefaultISMLocking.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ISMLocking.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/state/DefaultISMLockingDeadlockTest.java

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/DefaultISMLocking.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/DefaultISMLocking.java?rev=1000947&r1=1000946&r2=1000947&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/DefaultISMLocking.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/DefaultISMLocking.java
Fri Sep 24 16:38:45 2010
@@ -23,203 +23,163 @@ import javax.transaction.xa.Xid;
 import org.apache.jackrabbit.core.TransactionContext;
 import org.apache.jackrabbit.core.id.ItemId;
 
-import EDU.oswego.cs.dl.util.concurrent.ReadWriteLock;
-import EDU.oswego.cs.dl.util.concurrent.ReentrantWriterPreferenceReadWriteLock;
-import EDU.oswego.cs.dl.util.concurrent.Sync;
-
 /**
- * <code>DefaultISMLocking</code> implements the default locking strategy using
- * coarse grained locking on an ItemStateManager wide read-write lock. E.g.
- * while a write lock is held, no read lock can be acquired.
+ * Default item state locking strategy. The default strategy is simply to use
+ * a single coarse-grained read-write lock over the entire workspace.
  */
 public class DefaultISMLocking implements ISMLocking {
 
     /**
-     * The internal read-write lock.
-	 * Thread concerning ReentrantWriterPreferenceReadWriteLock
+     * The read lock instance used by readers to release the acquired lock.
      */
-    private final ReadWriteLock rwLock = new ReentrantWriterPreferenceReadWriteLock();
-    
+    private final ReadLock readLock = new ReadLock() {
+        public void release() {
+            releaseReadLock();
+        }
+    };
+
     /**
-     * The internal Xid aware read-write lock.
+     * The write lock instance used by writers to release or downgrade the
+     * acquired lock.
      */
-    private final ReadWriteLock xidRwLock = new XidRWLock();
-    
+    private final WriteLock writeLock = new WriteLock() {
+        public void release() {
+            releaseWriteLock(false);
+        }
+        public ReadLock downgrade() {
+            releaseWriteLock(true);
+            return readLock;
+        }
+    };
+
     /**
-     * {@inheritDoc}
+     * Number of writer threads waiting. While greater than zero, no new
+     * (unrelated) readers are allowed to proceed.
      */
-    public ReadLock acquireReadLock(ItemId id) throws InterruptedException {
-    	if (TransactionContext.getCurrentXid() == null) {
-            return new ReadLockImpl(rwLock.readLock());
-    	} else {
-            return new ReadLockImpl(xidRwLock.readLock());
-    	}
-    }
+    private int writersWaiting = 0;
 
     /**
-     * {@inheritDoc}
+     * The thread identifier of the current writer, or <code>null</code> if
+     * no write is in progress. A thread with the same identifier (i.e. the
+     * same thread or another thread in the same transaction) can re-acquire
+     * read or write locks without limitation, while all other readers and
+     * writers remain blocked. Note that a downgraded write lock still retains
+     * the writer thread identifier, which allows related threads to reacquire
+     * read or write locks even when there are concurrent writers waiting.
      */
-    public WriteLock acquireWriteLock(ChangeLog changeLog) throws InterruptedException {
-    	if (TransactionContext.getCurrentXid() == null) {
-    		return new WriteLockImpl(rwLock);
-    	} else {
-    		return new WriteLockImpl(xidRwLock);
-    	}
-    }
+    private Object writerId = null;
 
-    private static final class WriteLockImpl implements WriteLock {
-    	
-    	private ReadWriteLock readWriteLock;
-    	
-    	private WriteLockImpl(ReadWriteLock readWriteLock) throws InterruptedException {
-    		this.readWriteLock = readWriteLock;
-    		this.readWriteLock.writeLock().acquire();
-		}
+    /**
+     * Number of acquired write locks. All the concurrent write locks are
+     * guaranteed to share the same thread identifier (see {@link #writerId}).
+     */
+    private int writerCount = 0;
 
-		/**
-		 * {@inheritDoc}
-		 */
-		public void release() {
-		    this.readWriteLock.writeLock().release();
-		}
+    /**
+     * Number of acquired read locks.
+     */
+    private int readerCount = 0;
 
-		/**
-		 * {@inheritDoc}
-		 */
-		public ReadLock downgrade() throws InterruptedException {
-		    ReadLock rLock = new ReadLockImpl(this.readWriteLock.readLock());
-		    release();
-		    return rLock;
-		}
-	}
+    /**
+     * Increments the reader count and returns the acquired read lock once
+     * there are no more writers or the current writer shares the thread id
+     * with this reader.
+     */
+    public synchronized ReadLock acquireReadLock(ItemId id)
+            throws InterruptedException {
+        while (writerId != null
+                ? (writerCount > 0 && !isSameId(writerId, getCurrentId()))
+                : writersWaiting > 0) {
+            wait();
+        }
 
-	private static final class ReadLockImpl implements ReadLock {
+        readerCount++;
+        return readLock;
+    }
 
-        private final Sync readLock;
+    /**
+     * Decrements the reader count and notifies all pending threads if the
+     * lock is now available. Used by the {@link #readLock} instance.
+     */
+    private synchronized void releaseReadLock() {
+        readerCount--;
+        if (readerCount == 0 && writerCount == 0) {
+            writerId = null;
+            notifyAll();
+        }
+    }
 
-        private ReadLockImpl(Sync readLock) throws InterruptedException {
-            this.readLock = readLock;
-            this.readLock.acquire();
+    /**
+     * Increments the writer count, sets the writer identifier and returns
+     * the acquired read lock once there are no other active readers or
+     * writers or the current writer shares the thread id with this writer.
+     */
+    public synchronized WriteLock acquireWriteLock(ChangeLog changeLog)
+            throws InterruptedException {
+        Object currentId = getCurrentId();
+
+        writersWaiting++;
+        try {
+            while (writerId != null
+                    ? !isSameId(writerId, currentId) : readerCount > 0) {
+                wait();
+            }
+        } finally {
+            writersWaiting--;
         }
 
-        /**
-         * {@inheritDoc}
-         */
-        public void release() {
-            readLock.release();
+        if (writerCount++ == 0) {
+            writerId = currentId;
         }
+        return writeLock;
     }
 
-	/**
-	 * Xid concerning ReentrantWriterPreferenceReadWriteLock
-	 */
-    private static final class XidRWLock extends ReentrantWriterPreferenceReadWriteLock {
-
-        private Xid activeXid;
-
-        /**
-         * Check if the given Xid comes from the same globalTX
-         * @param otherXid
-         * @return true if same globalTX otherwise false
-         */
-        boolean isSameGlobalTx(Xid otherXid) {
-    	    return (activeXid == otherXid) || Arrays.equals(activeXid.getGlobalTransactionId(),
otherXid.getGlobalTransactionId());
-    	}
-
-        /**
-         * Allow reader when there is no active Xid, or current Xid owns
-         * the write lock (reentrant).
-         */
-        protected boolean allowReader() {
-        	Xid currentXid = TransactionContext.getCurrentXid();
-        	return (activeXid == null && waitingWriters_ == 0) || isSameGlobalTx(currentXid);
-        }
-
-        /**
-         * {@inheritDoc}
-         */  
-        protected synchronized boolean startWrite() {
-        	Xid currentXid = TransactionContext.getCurrentXid();
-            if (activeXid != null && isSameGlobalTx(currentXid)) { // already held;
re-acquire
-            	++writeHolds_;
-                return true;
-            } else if (writeHolds_ == 0) {
-            	if (activeReaders_ == 0 || (readers_.size() == 1 && readers_.get(currentXid)
!= null)) {
-            		activeXid = currentXid;
-            		writeHolds_ = 1;
-            		return true;
-            	} else {
-            		return false;
-            	}
-            } else {
-            	return false;
+    /**
+     * Decrements the writer count (and possibly clears the writer identifier)
+     * and notifies all pending threads if the lock is now available. If the
+     * downgrade argument is true, then the reader count is incremented before
+     * notifying any pending threads. Used by the {@link #writeLock} instance.
+     */
+    private synchronized void releaseWriteLock(boolean downgrade) {
+        writerCount--;
+        if (downgrade) {
+            readerCount++;
+        }
+        if (writerCount == 0) {
+            if (readerCount == 0) {
+                writerId = null;
             }
+            notifyAll();
         }
+    }
 
-        /**
-         * {@inheritDoc}
-         */
-        protected synchronized Signaller endWrite() {
-            --writeHolds_;
-            if (writeHolds_ > 0) {  // still being held
-            	return null;
-            } else {
-            	activeXid = null;
-                if (waitingReaders_ > 0 && allowReader()) {
-                    return readerLock_;
-                } else if (waitingWriters_ > 0) {
-                    return writerLock_;
-                } else {
-                    return null;
-                }
-            }
+    /**
+     * Returns the current thread identifier. The identifier is either the
+     * current thread instance or the global transaction identifier when
+     * running under a transaction.
+     *
+     * @return current thread identifier
+     */
+    private Object getCurrentId() {
+        Xid xid = TransactionContext.getCurrentXid();
+        if (xid != null) {
+            return xid.getGlobalTransactionId();
+        } else {
+            return Thread.currentThread();
         }
+    }
 
-        /**
-         * {@inheritDoc}
-         */
-		protected synchronized boolean startRead() {
-			Xid currentXid = TransactionContext.getCurrentXid();
-		    Object c = readers_.get(currentXid);
-		    if (c != null) { // already held -- just increment hold count
-		    	readers_.put(currentXid, new Integer(((Integer)(c)).intValue()+1));
-		    	++activeReaders_;
-		    	return true;
-		    } else if (allowReader()) {
-		    	readers_.put(currentXid, IONE);
-		    	++activeReaders_;
-		    	return true;
-		    } else {
-		    	return false;
-		    }
-		}
-
-        /**
-         * {@inheritDoc}
-         */
-		protected synchronized Signaller endRead() {
-			Xid currentXid = TransactionContext.getCurrentXid();
-		    Object c = readers_.get(currentXid);
-		    if (c == null) {
-		    	throw new IllegalStateException();
-		    }
-		    --activeReaders_;
-		    if (c != IONE) { // more than one hold; decrement count
-		    	int h = ((Integer)(c)).intValue()-1;
-		    	Integer ih = (h == 1)? IONE : new Integer(h);
-		    	readers_.put(currentXid, ih);
-		    	return null;
-		    } else {
-		    	readers_.remove(currentXid);
-		    
-		    	if (writeHolds_ > 0) { // a write lock is still held
-		    		return null;
-		    	} else if (activeReaders_ == 0 && waitingWriters_ > 0) {
-		    		return writerLock_;
-		    	} else  {
-		    		return null;
-		    	}
-		    }
-		}
+    /**
+     * Compares the given thread identifiers for equality.
+     */
+    private boolean isSameId(Object a, Object b) {
+        if (a == b) {
+            return true;
+        } else if (a instanceof byte[] && b instanceof byte[]) {
+            return Arrays.equals((byte[]) a, (byte[]) b);
+        } else {
+            return false;
+        }
     }
+
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ISMLocking.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ISMLocking.java?rev=1000947&r1=1000946&r2=1000947&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ISMLocking.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ISMLocking.java
Fri Sep 24 16:38:45 2010
@@ -39,6 +39,10 @@ import org.apache.jackrabbit.core.id.Ite
  * is free to block requests entirely for additional write lock while a write
  * lock is active. It is not a requirement to support concurrent write locks.
  * </li>
+ * <li>While a write lock is held for a change log <code>C</code>, the
holder
+ * of the write lock (and any related threads) needs to be able to acquire
+ * a read lock even if other writers are waiting for the lock. This behaviour
+ * must continue also when the write lock has been downgraded.</li>
  * </ul>
  */
 public interface ISMLocking {
@@ -81,10 +85,8 @@ public interface ISMLocking {
          * used to further release the read lock.
          *
          * @return the read lock downgraded from this write lock.
-         * @throws InterruptedException if the current thread is interrupted
-         *                              while downgrading the write lock.
          */
-        ReadLock downgrade() throws InterruptedException;
+        ReadLock downgrade();
 
     }
 

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java?rev=1000947&r1=1000946&r2=1000947&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java
Fri Sep 24 16:38:45 2010
@@ -787,9 +787,6 @@ public class SharedItemStateManager
                     String path = events.getSession().getUserID() + "@" + events.getCommonPath();
                     eventChannel.updateCommitted(this, path);
                 }
-
-            } catch (InterruptedException e) {
-                throw new ItemStateException("Interrupted while downgrading to read lock");
             } finally {
                 if (writeLock != null) {
                     // exception occurred before downgrading lock
@@ -1512,9 +1509,6 @@ public class SharedItemStateManager
                 holdingWriteLock = false;
                 events.dispatch();
             }
-        } catch (InterruptedException e) {
-            String msg = "Unable to downgrade to read lock.";
-            log.error(msg);
         } finally {
             if (holdingWriteLock) {
                 if (wLock != null) {

Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/state/DefaultISMLockingDeadlockTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/state/DefaultISMLockingDeadlockTest.java?rev=1000947&r1=1000946&r2=1000947&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/state/DefaultISMLockingDeadlockTest.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/state/DefaultISMLockingDeadlockTest.java
Fri Sep 24 16:38:45 2010
@@ -19,6 +19,7 @@ package org.apache.jackrabbit.core.state
 import org.apache.jackrabbit.core.state.DefaultISMLocking;
 import org.apache.jackrabbit.core.state.ISMLocking;
 import org.apache.jackrabbit.core.state.ISMLocking.ReadLock;
+import org.apache.jackrabbit.core.state.ISMLocking.WriteLock;
 import org.apache.jackrabbit.test.JUnitTest;
 
 /**
@@ -28,7 +29,8 @@ public class DefaultISMLockingDeadlockTe
 
     public void test() throws InterruptedException {
         final ISMLocking lock = new DefaultISMLocking();
-        ReadLock r1 = lock.acquireReadLock(null);
+        WriteLock w1 = lock.acquireWriteLock(null);
+        ReadLock r1 = w1.downgrade();
         final InterruptedException[] ex = new InterruptedException[1];
         Thread thread = new Thread() {
             public void run() {



Mime
View raw message