Mailing-List: contact commits-help@jackrabbit.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@jackrabbit.apache.org
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: svn commit: r1036117 - in
 /jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state:
 LocalItemStateManager.java SharedItemStateManager.java
Date: Wed, 17 Nov 2010 17:14:15 -0000
To: commits@jackrabbit.apache.org
From: stefan@apache.org
Message-Id: <20101117171415.656852388903@eris.apache.org>

Author: stefan
Date: Wed Nov 17 17:14:14 2010
New Revision: 1036117

URL: http://svn.apache.org/viewvc?rev=1036117&view=rev
Log:
Revert: JCR-2699: Improve read/write concurrency  It's better to allow the occasional cache overwrite than to block concurrent threads. The locking in SessionState and SISM already prevent concurrent read/write operations that could possibly lead to inconsistencies.

=> the log warnings "overwriting cached entry" do indicate that, at any given time, 
there might be multiple ItemState instances in the 'shared' layer representing the same 
item. there's a risk of data loss/inconsistency since all but one instance are stale copies.

Modified:
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/LocalItemStateManager.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/LocalItemStateManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/LocalItemStateManager.java?rev=1036117&r1=1036116&r2=1036117&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/LocalItemStateManager.java (original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/LocalItemStateManager.java Wed Nov 17 17:14:14 2010
@@ -163,17 +163,19 @@ public class LocalItemStateManager
             return state;
         }
 
-        // check cache
-        state = cache.retrieve(id);
-        if (state == null) {
-            // regular behaviour
-            if (id.denotesNode()) {
-                state = getNodeState((NodeId) id);
-            } else {
-                state = getPropertyState((PropertyId) id);
+        // check cache. synchronized to ensure an entry is not created twice.
+        synchronized (this) {
+            state = cache.retrieve(id);
+            if (state == null) {
+                // regular behaviour
+                if (id.denotesNode()) {
+                    state = getNodeState((NodeId) id);
+                } else {
+                    state = getPropertyState((PropertyId) id);
+                }
             }
+            return state;
         }
-        return state;
     }
 
     /**

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=1036117&r1=1036116&r2=1036117&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 Wed Nov 17 17:14:14 2010
@@ -1703,8 +1703,21 @@ public class SharedItemStateManager
             // not found in cache, load from persistent storage
             state = loadItemState(id);
             state.setStatus(ItemState.STATUS_EXISTING);
-            state.setContainer(this);
-            cache.cache(state);
+            synchronized (this) {
+                // Use a double check to ensure that the cache entry is
+                // not created twice. We don't synchronize the entire
+                // method to allow the first cache retrieval to proceed
+                // even when another thread is loading a new item state.
+                ItemState cachedState = cache.retrieve(id);
+                if (cachedState == null) {
+                    // put it in cache
+                    cache.cache(state);
+                    // set parent container
+                    state.setContainer(this);
+                } else {
+                    state = cachedState;
+                }
+            }
         }
         return state;
     }