Author: stefan
Date: Fri Nov  3 08:00:56 2006
New Revision: 470869

URL: http://svn.apache.org/viewvc?view=rev&rev=470869
Log:
JJCR-584: Improve handling of concurrent node modifications

Added:
    jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java   (with props)
    jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/ConcurrentNodeModificationTest.java   (with props)
Modified:
    jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/ChangeLog.java
    jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java
    jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java
    jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/TestAll.java

Modified: jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/ChangeLog.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/ChangeLog.java?view=diff&rev=470869&r1=470868&r2=470869
==============================================================================
--- jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/ChangeLog.java (original)
+++ jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/ChangeLog.java Fri Nov  3 08:00:56 2006
@@ -131,6 +131,17 @@
     }
 
     /**
+     * Return a flag indicating whether a given item state is marked as
+     * deleted in this log.
+     *
+     * @return <code>true</code> if item state is marked as deleted in this
+     *         log; <code>false</code> otherwise
+     */
+    public boolean deleted(ItemId id) {
+        return deletedStates.containsKey(id);
+    }
+
+    /**
      * Return a node references object given its id. Returns
      * <code>null</code> if the node reference is not in the modified
      * section.

Added: jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java?view=auto&rev=470869
==============================================================================
--- jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java (added)
+++ jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java Fri Nov  3 08:00:56 2006
@@ -0,0 +1,211 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.core.state;
+
+import org.apache.jackrabbit.core.PropertyId;
+import org.apache.jackrabbit.core.ItemId;
+import org.apache.jackrabbit.name.QName;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.HashSet;
+
+/**
+ * Internal utility class used for merging concurrent changes that occured
+ * on different <code>NodeState</code> instances representing the same node.
+ * <p/>
+ * See http://issues.apache.org/jira/browse/JCR-584.
+ */
+class NodeStateMerger {
+
+    /**
+     * Tries to silently merge the given <code>state</code> with its
+     * externally (e.g. through another session) modified overlayed state
+     * in order to avoid an <code>InvalidItemStateException</code>.
+     * <p/>
+     * See http://issues.apache.org/jira/browse/JCR-584.
+     *
+     * @param state node state whose modified overlayed state should be
+     *        merged
+     * @param context used for analyzing the context of the modifications
+     * @return true if the changes could be successfully merged into the
+     *         given node state; false otherwise
+     */
+    static boolean merge(NodeState state, MergeContext context) {
+
+        NodeState overlayedState = (NodeState) state.getOverlayedState();
+        if (overlayedState == null
+                || state.getModCount() == overlayedState.getModCount()) {
+            return false;
+        }
+
+        /**
+         * some examples for trivial non-conflicting changes:
+         * - s1 added child node a, s2 removes child node b
+         * - s1 adds child node a, s2 adds child node b
+         * - s1 adds child node a, s2 adds mixin type
+         *
+         * conflicting changes causing staleness:
+         * - s1 added non-sns child node or property a,
+         *   s2 added non-sns child node or property a => name clash
+         * - either session reordered child nodes
+         *   (some combinations could possibly be merged)
+         * - either session moved node
+         */
+
+        // compare current transient state with externally modified
+        // overlayed state and determine what has been changed by whom
+
+        // child node entries order
+        if (!state.getReorderedChildNodeEntries().isEmpty()) {
+            // for now we don't even try to merge the result of
+            // a reorder operation
+            return false;
+        }
+
+        // mixin types
+        if (!state.getMixinTypeNames().equals(overlayedState.getMixinTypeNames())) {
+            // the mixins have been modified but by just looking at the diff we
+            // can't determine where the change happened since the diffs of either
+            // removing a mixin from the overlayed or adding a mixin to the
+            // transient state would look identical...
+            return false;
+        }
+
+        // parent id
+        if (state.getParentId() != null
+                && !state.getParentId().equals(overlayedState.getParentId())) {
+            return false;
+        }
+
+        // child node entries
+        if (!state.getChildNodeEntries().equals(
+                overlayedState.getChildNodeEntries())) {
+            ArrayList added = new ArrayList();
+            ArrayList removed = new ArrayList();
+
+            for (Iterator iter = state.getAddedChildNodeEntries().iterator();
+                 iter.hasNext();) {
+                NodeState.ChildNodeEntry cne =
+                        (NodeState.ChildNodeEntry) iter.next();
+
+                if (context.isAdded(cne.getId())) {
+                    // a new child node entry has been added to this state;
+                    // check for name collisions with other state
+                    if (overlayedState.hasPropertyName(cne.getName())
+                            && !context.isDeleted(new PropertyId(state.getNodeId(), cne.getName()))) {
+                        // conflicting names
+                        return false;
+                    }
+                    if (overlayedState.hasChildNodeEntry(cne.getName())) {
+                        // conflicting names
+                        if (cne.getIndex() < 2) {
+                            // todo check if same-name siblings are allowed
+                            return false;
+                        }
+                        // assume same-name siblings are allowed since index is >= 2
+                    }
+
+                    added.add(cne);
+                }
+            }
+
+            for (Iterator iter = state.getRemovedChildNodeEntries().iterator();
+                 iter.hasNext();) {
+                NodeState.ChildNodeEntry cne =
+                        (NodeState.ChildNodeEntry) iter.next();
+                if (context.isDeleted(cne.getId())) {
+                    // a child node entry has been removed from this node state
+                    removed.add(cne);
+                }
+            }
+
+            // copy child node antries from other state and
+            // re-apply changes made on this state
+            state.setChildNodeEntries(overlayedState.getChildNodeEntries());
+            for (Iterator iter = added.iterator(); iter.hasNext();) {
+                NodeState.ChildNodeEntry cne =
+                        (NodeState.ChildNodeEntry) iter.next();
+                state.addChildNodeEntry(cne.getName(), cne.getId());
+            }
+            for (Iterator iter = removed.iterator(); iter.hasNext();) {
+                NodeState.ChildNodeEntry cne =
+                        (NodeState.ChildNodeEntry) iter.next();
+                state.removeChildNodeEntry(cne.getId());
+            }
+        }
+
+        // property names
+        if (!state.getPropertyNames().equals(
+                overlayedState.getPropertyNames())) {
+            HashSet added = new HashSet();
+            HashSet removed = new HashSet();
+
+            for (Iterator iter = state.getAddedPropertyNames().iterator();
+                 iter.hasNext();) {
+                QName name = (QName) iter.next();
+                PropertyId propId =
+                        new PropertyId(state.getNodeId(), name);
+                if (context.isAdded(propId)) {
+                    // a new property name has been added to this state;
+                    // check for name collisions
+                    if (overlayedState.hasPropertyName(name)
+                            || overlayedState.hasChildNodeEntry(name)) {
+                        // conflicting names
+                        return false;
+                    }
+
+                    added.add(name);
+                }
+            }
+            for (Iterator iter = state.getRemovedPropertyNames().iterator();
+                 iter.hasNext();) {
+                QName name = (QName) iter.next();
+                PropertyId propId =
+                        new PropertyId(state.getNodeId(), name);
+                if (context.isDeleted(propId)) {
+                    // a property name has been removed from this state
+                    removed.add(name);
+                }
+            }
+
+            // copy property names from other and
+            // re-apply changes made on this state
+            state.setPropertyNames(overlayedState.getPropertyNames());
+            for (Iterator iter = added.iterator(); iter.hasNext();) {
+                QName name = (QName) iter.next();
+                state.addPropertyName(name);
+            }
+            for (Iterator iter = removed.iterator(); iter.hasNext();) {
+                QName name = (QName) iter.next();
+                state.removePropertyName(name);
+            }
+        }
+
+        // finally sync modification count
+        state.setModCount(overlayedState.getModCount());
+
+        return true;
+    }
+
+    //-----------------------------------------------------< inner interfaces >
+
+    static interface MergeContext {
+        boolean isAdded(ItemId id);
+        boolean isDeleted(ItemId id);
+    }
+}

Propchange: jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java
------------------------------------------------------------------------------
    svn:eol-style = native

Propchange: jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java
------------------------------------------------------------------------------
    svn:keywords = author date id revision url

Modified: jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java?view=diff&rev=470869&r1=470868&r2=470869
==============================================================================
--- jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java (original)
+++ jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java Fri Nov  3 08:00:56 2006
@@ -112,6 +112,7 @@
     }
 
     //-------------------------------------------------------------< Dumpable >
+
     /**
      * {@inheritDoc}
      */
@@ -135,6 +136,7 @@
     }
 
     //-----------------------------------------------------< ItemStateManager >
+
     /**
      * {@inheritDoc}
      */
@@ -204,6 +206,7 @@
     }
 
     //--------------------------------------------< UpdatableItemStateManager >
+
     /**
      * {@inheritDoc}
      */
@@ -516,6 +519,7 @@
     }
 
     //------< methods for creating & discarding transient ItemState instances >
+
     /**
      * @param id
      * @param nodeTypeName
@@ -708,6 +712,7 @@
 
     /**
      * Add an <code>ItemStateListener</code>
+     *
      * @param listener the new listener to be informed on modifications
      */
     public void addListener(ItemStateListener listener) {
@@ -716,6 +721,7 @@
 
     /**
      * Remove an <code>ItemStateListener</code>
+     *
      * @param listener an existing listener
      */
     public void removeListener(ItemStateListener listener) {
@@ -772,6 +778,25 @@
             // local state was modified
             ItemState transientState = transientStore.get(modified.getId());
             if (transientState != null) {
+                if (transientState.isNode() && !transientState.isStale()) {
+                    // try to silently merge non-conflicting changes (JCR-584)
+                    NodeStateMerger.MergeContext context =
+                            new NodeStateMerger.MergeContext() {
+                                public boolean isAdded(ItemId id) {
+                                    ItemState is = transientStore.get(id);
+                                    return is != null
+                                            && is.getStatus() == ItemState.STATUS_NEW;
+                                }
+
+                                public boolean isDeleted(ItemId id) {
+                                    return atticStore.contains(id);
+                                }
+                            };
+                    if (NodeStateMerger.merge((NodeState) transientState, context)) {
+                        // merge succeeded
+                        return;
+                    }
+                }
                 transientState.setStatus(ItemState.STATUS_STALE_MODIFIED);
                 visibleState = transientState;
             }

Modified: jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java?view=diff&rev=470869&r1=470868&r2=470869
==============================================================================
--- jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java (original)
+++ jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/core/state/SharedItemStateManager.java Fri Nov  3 08:00:56 2006
@@ -543,9 +543,33 @@
                     ItemState state = (ItemState) iter.next();
                     state.connect(getItemState(state.getId()));
                     if (state.isStale()) {
-                        String msg = state.getId() + " has been modified externally";
-                        log.debug(msg);
-                        throw new StaleItemStateException(msg);
+                        boolean merged = false;
+                        if (state.isNode()) {
+                            NodeStateMerger.MergeContext context =
+                                    new NodeStateMerger.MergeContext() {
+                                        public boolean isAdded(ItemId id) {
+                                            try {
+                                                ItemState is = local.get(id);
+                                                return is != null
+                                                        && is.getStatus() == ItemState.STATUS_NEW;
+                                            } catch (NoSuchItemStateException e) {
+                                                return false;
+                                            }
+                                        }
+
+                                        public boolean isDeleted(ItemId id) {
+                                            return local.deleted(id);
+                                        }
+                                    };
+
+                            merged = NodeStateMerger.merge((NodeState) state, context);
+                        }
+                        if (!merged) {
+                            String msg = state.getId() + " has been modified externally";
+                            log.debug(msg);
+                            throw new StaleItemStateException(msg);
+                        }
+                        // merge succeeded, fall through
                     }
 
                     // update modification count (will be persisted as well)

Added: jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/ConcurrentNodeModificationTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/ConcurrentNodeModificationTest.java?view=auto&rev=470869
==============================================================================
--- jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/ConcurrentNodeModificationTest.java (added)
+++ jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/ConcurrentNodeModificationTest.java Fri Nov  3 08:00:56 2006
@@ -0,0 +1,140 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.core;
+
+import org.apache.jackrabbit.test.AbstractJCRTest;
+
+import javax.jcr.Node;
+import javax.jcr.Session;
+import javax.jcr.Value;
+import java.util.Random;
+import java.util.ArrayList;
+import java.util.Iterator;
+
+/**
+ * Performs a test with n sessions concurrently performing non-conflicting
+ * modifications on the <i>same</i> node.
+ * <p/>
+ * See http://issues.apache.org/jira/browse/JCR-584.
+ */
+public class ConcurrentNodeModificationTest extends AbstractJCRTest {
+
+    private static final int NUM_ITERATIONS = 1;
+    private static final int NUM_SESSIONS = 10;
+    private static final int NUM_NODES = 100;
+
+    final ArrayList exceptions = new ArrayList();
+
+    /**
+     * Runs the test.
+     */
+    public void testConcurrentNodeModificationSessions() throws Exception {
+        int n = NUM_ITERATIONS;
+        while (n-- > 0) {
+            Thread[] threads = new Thread[NUM_SESSIONS];
+            for (int i = 0; i < threads.length; i++) {
+                // create new session
+                Session session = helper.getSuperuserSession();
+                TestSession ts = new TestSession("s" + i, session);
+                Thread t = new Thread(ts);
+                t.setName((NUM_ITERATIONS - n) + "-s" + i);
+                t.start();
+                log.println("Thread#" + i + " started");
+                threads[i] = t;
+                Thread.sleep(100);
+            }
+            for (int i = 0; i < threads.length; i++) {
+                threads[i].join();
+            }
+        }
+
+        if (!exceptions.isEmpty()) {
+            Exception e = null;
+            for (Iterator it = exceptions.iterator(); it.hasNext();) {
+                e = (Exception) it.next();
+                e.printStackTrace(log);
+            }
+            throw e;
+            //fail();
+        }
+    }
+
+    //--------------------------------------------------------< inner classes >
+    class TestSession implements Runnable {
+
+        Session session;
+        String identity;
+        Random r;
+
+        TestSession(String identity, Session s) {
+            session = s;
+            this.identity = identity;
+            r = new Random();
+        }
+
+        private void randomSleep() {
+            long l = r.nextInt(900) + 200;
+            try {
+                Thread.sleep(l);
+            } catch (InterruptedException ie) {
+            }
+        }
+
+        public void run() {
+
+            log.println("started.");
+            String state = "";
+            try {
+                Node n = session.getRootNode().getNode(testPath);
+
+                String propName = "testprop" + Math.random();
+
+                state = "setting property " + propName;
+                n.setProperty(propName, "Hello World!");
+                session.save();
+                randomSleep();
+
+                state = "removing property " + propName;
+                n.setProperty(propName, (Value) null);
+                session.save();
+                randomSleep();
+
+                for (int i = 0; i < NUM_NODES; i++) {
+                    state = "adding subnode " + i;
+                    //Node n1 = n.addNode("x" + i, "nt:unstructured");
+                    Node n1 = n.addNode("x" + identity + i, "nt:unstructured");
+                    state = "adding property to subnode " + i;
+                    n1.setProperty("testprop", "xxx");
+                    if (i % 10 == 0) {
+                        state = "saving pending subnodes";
+                        session.save();
+                    }
+                    randomSleep();
+                }
+                session.save();
+            } catch (Exception e) {
+                log.println("Exception while " + state + ": " + e.getMessage());
+                //e.printStackTrace();
+                exceptions.add(e);
+            } finally {
+                session.logout();
+            }
+
+            log.println("ended.");
+        }
+    }
+}

Propchange: jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/ConcurrentNodeModificationTest.java
------------------------------------------------------------------------------
    svn:eol-style = native

Propchange: jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/ConcurrentNodeModificationTest.java
------------------------------------------------------------------------------
    svn:keywords = author date id revision url

Modified: jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/TestAll.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/TestAll.java?view=diff&rev=470869&r1=470868&r2=470869
==============================================================================
--- jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/TestAll.java (original)
+++ jackrabbit/trunk/jackrabbit/src/test/java/org/apache/jackrabbit/core/TestAll.java Fri Nov  3 08:00:56 2006
@@ -37,6 +37,7 @@
 
         //suite.addTestSuite(ConcurrencyTest.class);
         //suite.addTestSuite(ConcurrentSaveTest.class);
+        //suite.addTestSuite(ConcurrentNodeModificationTest.class);
         //suite.addTestSuite(LockTest.class);
         suite.addTestSuite(NamespaceRegistryImplTest.class);
         suite.addTestSuite(TransientRepositoryTest.class);