Return-Path: Delivered-To: apmail-jackrabbit-commits-archive@www.apache.org Received: (qmail 42035 invoked from network); 7 Mar 2008 14:11:12 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.2) by minotaur.apache.org with SMTP; 7 Mar 2008 14:11:12 -0000 Received: (qmail 61231 invoked by uid 500); 7 Mar 2008 14:11:09 -0000 Delivered-To: apmail-jackrabbit-commits-archive@jackrabbit.apache.org Received: (qmail 61198 invoked by uid 500); 7 Mar 2008 14:11:09 -0000 Mailing-List: contact commits-help@jackrabbit.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@jackrabbit.apache.org Delivered-To: mailing list commits@jackrabbit.apache.org Received: (qmail 61188 invoked by uid 99); 7 Mar 2008 14:11:09 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 07 Mar 2008 06:11:09 -0800 X-ASF-Spam-Status: No, hits=-2000.0 required=10.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.3] (HELO eris.apache.org) (140.211.11.3) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 07 Mar 2008 14:10:32 +0000 Received: by eris.apache.org (Postfix, from userid 65534) id 726D31A9832; Fri, 7 Mar 2008 06:10:37 -0800 (PST) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r634681 - in /jackrabbit/trunk/jackrabbit-core/src: main/java/org/apache/jackrabbit/core/ main/java/org/apache/jackrabbit/core/state/ test/java/org/apache/jackrabbit/core/integration/ Date: Fri, 07 Mar 2008 14:10:28 -0000 To: commits@jackrabbit.apache.org From: stefan@apache.org X-Mailer: svnmailer-1.0.8 Message-Id: <20080307141041.726D31A9832@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: stefan Date: Fri Mar 7 06:10:27 2008 New Revision: 634681 URL: http://svn.apache.org/viewvc?rev=634681&view=rev Log: JCR-1359: Adding nodes from concurrently running sessions cause exceptions - added synchronization to NodeStateMerger.merge method - fixed @todo issue in NodeStateMerger.merge method (checking node type sameNameSibling setting) - enabling execution of ConcurrentNodeModificationTest Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SessionImpl.java jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.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/integration/MultiThreadingTest.java Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SessionImpl.java URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SessionImpl.java?rev=634681&r1=634680&r2=634681&view=diff ============================================================================== --- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SessionImpl.java (original) +++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SessionImpl.java Fri Mar 7 06:10:27 2008 @@ -270,7 +270,8 @@ * @return session item state manager */ protected SessionItemStateManager createSessionItemStateManager(LocalItemStateManager manager) { - return new SessionItemStateManager(rep.getRootNodeId(), manager, this); + return new SessionItemStateManager( + rep.getRootNodeId(), manager, this, rep.getNodeTypeRegistry()); } /** Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java?rev=634681&r1=634680&r2=634681&view=diff ============================================================================== --- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java (original) +++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeStateMerger.java Fri Mar 7 06:10:27 2008 @@ -18,6 +18,7 @@ import org.apache.jackrabbit.core.ItemId; import org.apache.jackrabbit.core.PropertyId; +import org.apache.jackrabbit.core.NodeId; import org.apache.jackrabbit.spi.Name; import java.util.ArrayList; @@ -53,148 +54,154 @@ 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; - } + synchronized (overlayedState) { + synchronized (state) { + /** + * 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; - } + // 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; - } + // 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.hasChildNodeEntry(cne.getName())) { + // conflicting names + if (cne.getIndex() < 2) { + // check if same-name siblings are allowed + if (!context.allowsSameNameSiblings(cne.getId())) { + return false; + } + } + // assume same-name siblings are allowed since index is >= 2 + } - // 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.hasChildNodeEntry(cne.getName())) { - // conflicting names - if (cne.getIndex() < 2) { - // todo check if same-name siblings are allowed - return false; + added.add(cne); } - // 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); + } + } - 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()); + } } - } - // 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();) { + Name name = (Name) 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; + } - // property names - if (!state.getPropertyNames().equals( - overlayedState.getPropertyNames())) { - HashSet added = new HashSet(); - HashSet removed = new HashSet(); - - for (Iterator iter = state.getAddedPropertyNames().iterator(); - iter.hasNext();) { - Name name = (Name) 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();) { + Name name = (Name) 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); + } } - added.add(name); - } - } - for (Iterator iter = state.getRemovedPropertyNames().iterator(); - iter.hasNext();) { - Name name = (Name) 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();) { + Name name = (Name) iter.next(); + state.addPropertyName(name); + } + for (Iterator iter = removed.iterator(); iter.hasNext();) { + Name name = (Name) iter.next(); + state.removePropertyName(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();) { - Name name = (Name) iter.next(); - state.addPropertyName(name); - } - for (Iterator iter = removed.iterator(); iter.hasNext();) { - Name name = (Name) iter.next(); - state.removePropertyName(name); + // finally sync modification count + state.setModCount(overlayedState.getModCount()); + + return true; } } - - // finally sync modification count - state.setModCount(overlayedState.getModCount()); - - return true; } //-----------------------------------------------------< inner interfaces > @@ -202,5 +209,6 @@ static interface MergeContext { boolean isAdded(ItemId id); boolean isDeleted(ItemId id); + boolean allowsSameNameSiblings(NodeId id); } } Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java?rev=634681&r1=634680&r2=634681&view=diff ============================================================================== --- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java (original) +++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/SessionItemStateManager.java Fri Mar 7 06:10:27 2008 @@ -23,6 +23,8 @@ import org.apache.jackrabbit.core.NodeId; import org.apache.jackrabbit.core.PropertyId; import org.apache.jackrabbit.core.ZombieHierarchyManager; +import org.apache.jackrabbit.core.nodetype.NodeTypeRegistry; +import org.apache.jackrabbit.core.nodetype.NodeDef; import org.apache.jackrabbit.core.util.Dumpable; import org.apache.jackrabbit.spi.commons.conversion.PathResolver; import org.apache.jackrabbit.spi.Name; @@ -76,6 +78,11 @@ private AtticItemStateManager attic; /** + * Node Type Registry + */ + private final NodeTypeRegistry ntReg; + + /** * State change dispatcher. */ private final transient StateChangeDispatcher dispatcher = new StateChangeDispatcher(); @@ -86,10 +93,12 @@ * @param rootNodeId the root node id * @param stateMgr the local item state manager * @param resolver path resolver for outputting user-friendly paths + * @param ntReg node type registry */ public SessionItemStateManager(NodeId rootNodeId, LocalItemStateManager stateMgr, - PathResolver resolver) { + PathResolver resolver, + NodeTypeRegistry ntReg) { transientStore = new ItemStateMap(); atticStore = new ItemStateMap(); @@ -99,6 +108,8 @@ // create hierarchy manager that uses both transient and persistent state hierMgr = new CachingHierarchyManager(rootNodeId, this, resolver); addListener(hierMgr); + + this.ntReg = ntReg; } /** @@ -815,6 +826,17 @@ public boolean isDeleted(ItemId id) { return atticStore.contains(id); + } + + public boolean allowsSameNameSiblings(NodeId id) { + NodeState ns; + try { + ns = (NodeState) getItemState(id); + } catch (ItemStateException e) { + return false; + } + NodeDef def = ntReg.getNodeDef(ns.getDefinitionId()); + return def != null ? def.allowsSameNameSiblings() : false; } }; if (NodeStateMerger.merge((NodeState) transientState, context)) { 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=634681&r1=634680&r2=634681&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 Mar 7 06:10:27 2008 @@ -28,6 +28,7 @@ import org.apache.jackrabbit.core.nodetype.NodeTypeConflictException; import org.apache.jackrabbit.core.nodetype.NodeTypeRegistry; import org.apache.jackrabbit.core.nodetype.PropDef; +import org.apache.jackrabbit.core.nodetype.NodeDef; import org.apache.jackrabbit.core.observation.EventStateCollection; import org.apache.jackrabbit.core.observation.EventStateCollectionFactory; import org.apache.jackrabbit.core.util.Dumpable; @@ -614,6 +615,21 @@ public boolean isDeleted(ItemId id) { return local.deleted(id); + } + + public boolean allowsSameNameSiblings(NodeId id) { + NodeState ns; + try { + if (local.has(id)) { + ns = (NodeState) local.get(id); + } else { + ns = (NodeState) getItemState(id); + } + } catch (ItemStateException e) { + return false; + } + NodeDef def = ntReg.getNodeDef(ns.getDefinitionId()); + return def != null ? def.allowsSameNameSiblings() : false; } }; Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/integration/MultiThreadingTest.java URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/integration/MultiThreadingTest.java?rev=634681&r1=634680&r2=634681&view=diff ============================================================================== --- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/integration/MultiThreadingTest.java (original) +++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/integration/MultiThreadingTest.java Fri Mar 7 06:10:27 2008 @@ -41,7 +41,7 @@ suite.addTestSuite(ConcurrencyTest.class); suite.addTestSuite(ConcurrentLoginTest.class); - //suite.addTestSuite(ConcurrentNodeModificationTest.class); + suite.addTestSuite(ConcurrentNodeModificationTest.class); suite.addTestSuite(ConcurrentReadWriteTest.class); suite.addTestSuite(ConcurrentSaveTest.class); suite.addTestSuite(ConcurrentVersioningTest.class);