jackrabbit-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ju...@apache.org
Subject svn commit: r955307 - in /jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core: state/ChildNodeEntries.java state/NodeState.java version/InternalVersionHistoryImpl.java
Date Wed, 16 Jun 2010 17:16:20 GMT
Author: jukka
Date: Wed Jun 16 17:16:20 2010
New Revision: 955307

URL: http://svn.apache.org/viewvc?rev=955307&view=rev
Log:
JCR-2579: InvalidItemStateException when attempting concurrent, non conflicting writes

The problem was caused by NodeState returning the underlying ChildNodeEntries as an unsynchronized
List<> instance. I've refactored the code so that whenever a list of ChildNodeEntries
is returned from NodeState, that list is independent of the internal mutable state of the
NodeState instance.

Modified:
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ChildNodeEntries.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeState.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/version/InternalVersionHistoryImpl.java

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ChildNodeEntries.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ChildNodeEntries.java?rev=955307&r1=955306&r2=955307&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ChildNodeEntries.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/ChildNodeEntries.java
Wed Jun 16 17:16:20 2010
@@ -17,8 +17,6 @@
 package org.apache.jackrabbit.core.state;
 
 import org.apache.commons.collections.map.LinkedMap;
-import org.apache.commons.collections.MapIterator;
-import org.apache.commons.collections.OrderedMapIterator;
 import org.apache.jackrabbit.core.id.NodeId;
 import org.apache.jackrabbit.core.util.EmptyLinkedMap;
 import org.apache.jackrabbit.spi.Name;
@@ -27,20 +25,14 @@ import java.util.List;
 import java.util.HashMap;
 import java.util.Collections;
 import java.util.ArrayList;
-import java.util.Iterator;
-import java.util.Collection;
-import java.util.ListIterator;
 import java.util.Map;
 
 /**
  * <code>ChildNodeEntries</code> represents an insertion-ordered
  * collection of <code>ChildNodeEntry</code>s that also maintains
  * the index values of same-name siblings on insertion and removal.
- * <p/>
- * <code>ChildNodeEntries</code> also provides an unmodifiable
- * <code>List</code> view.
  */
-class ChildNodeEntries implements List<ChildNodeEntry>, Cloneable {
+class ChildNodeEntries implements Cloneable {
 
     /**
      * Insertion-ordered map of entries
@@ -264,11 +256,12 @@ class ChildNodeEntries implements List<C
             return Collections.emptyList();
         }
         if (other.isEmpty()) {
-            return this;
+            return list();
         }
 
         List<ChildNodeEntry> result = new ArrayList<ChildNodeEntry>();
-        for (ChildNodeEntry entry : this) {
+        for (Object e : entries.values()) {
+            ChildNodeEntry entry = (ChildNodeEntry) e;
             ChildNodeEntry otherEntry = other.get(entry.getId());
             if (entry == otherEntry) {
                 continue;
@@ -301,7 +294,8 @@ class ChildNodeEntries implements List<C
         }
 
         List<ChildNodeEntry> result = new ArrayList<ChildNodeEntry>();
-        for (ChildNodeEntry entry : this) {
+        for (Object e : entries.values()) {
+            ChildNodeEntry entry = (ChildNodeEntry) e;
             ChildNodeEntry otherEntry = other.get(entry.getId());
             if (entry == otherEntry) {
                 result.add(entry);
@@ -314,138 +308,37 @@ class ChildNodeEntries implements List<C
     }
 
     //-----------------------------------------------< unmodifiable List view >
-    public boolean contains(Object o) {
-        if (o instanceof ChildNodeEntry) {
-            return entries.containsKey(((ChildNodeEntry) o).getId());
-        } else {
-            return false;
-        }
-    }
-
-    public boolean containsAll(Collection<?> c) {
-        for (Object entry : c) {
-            if (!contains(entry)) {
-                return false;
-            }
-        }
-        return true;
-    }
-
-    public ChildNodeEntry get(int index) {
-        return (ChildNodeEntry) entries.getValue(index);
-    }
-
-    public int indexOf(Object o) {
-        if (o instanceof ChildNodeEntry) {
-            return entries.indexOf(((ChildNodeEntry) o).getId());
-        } else {
-            return -1;
-        }
-    }
 
     public boolean isEmpty() {
         return entries.isEmpty();
     }
 
-    public int lastIndexOf(Object o) {
-        // entries are unique
-        return indexOf(o);
-    }
-
-    public Iterator<ChildNodeEntry> iterator() {
-        return new EntriesIterator();
-    }
-
-    public ListIterator<ChildNodeEntry> listIterator() {
-        return new EntriesIterator();
+    @SuppressWarnings("unchecked")
+    public List<ChildNodeEntry> list() {
+        return new ArrayList<ChildNodeEntry>(entries.values());
     }
 
-    public ListIterator<ChildNodeEntry> listIterator(int index) {
-        if (index < 0 || index >= entries.size()) {
-            throw new IndexOutOfBoundsException();
-        }
-        ListIterator<ChildNodeEntry> iter = new EntriesIterator();
-        while (index-- > 0) {
-            iter.next();
+    public List<ChildNodeEntry> getRenamedEntries(ChildNodeEntries that) {
+        List<ChildNodeEntry> renamed = Collections.emptyList();
+        for (Object e : entries.values()) {
+            ChildNodeEntry entry = (ChildNodeEntry) e;
+            ChildNodeEntry other = that.get(entry.getId());
+            if (other != null && !entry.getName().equals(other.getName())) {
+                // child node entry with same id but different name exists in
+                // overlaid and this state => renamed entry detected
+                if (renamed.isEmpty()) {
+                    renamed = new ArrayList<ChildNodeEntry>();
+                }
+                renamed.add(entry);
+            }
         }
-        return iter;
+        return renamed;
     }
 
     public int size() {
         return entries.size();
     }
 
-    public List<ChildNodeEntry> subList(int fromIndex, int toIndex) {
-        // @todo FIXME does not fulfill the contract of List.subList(int,int)
-        return Collections.unmodifiableList(new ArrayList<ChildNodeEntry>(this).subList(fromIndex,
toIndex));
-    }
-
-    public Object[] toArray() {
-        ChildNodeEntry[] array = new ChildNodeEntry[size()];
-        return toArray(array);
-    }
-
-    @SuppressWarnings("unchecked")
-    public Object[] toArray(Object[] a) {
-        if (!a.getClass().getComponentType().isAssignableFrom(ChildNodeEntry.class)) {
-            throw new ArrayStoreException();
-        }
-        if (a.length < size()) {
-            a = new ChildNodeEntry[size()];
-        }
-        MapIterator iter = entries.mapIterator();
-        int i = 0;
-        while (iter.hasNext()) {
-            iter.next();
-            a[i] = entries.getValue(i);
-            i++;
-        }
-        while (i < a.length) {
-            a[i++] = null;
-        }
-        return a;
-    }
-
-    public void add(int index, ChildNodeEntry element) {
-        throw new UnsupportedOperationException();
-    }
-
-    public boolean add(ChildNodeEntry o) {
-        throw new UnsupportedOperationException();
-    }
-
-    public boolean addAll(Collection<? extends ChildNodeEntry> c) {
-        throw new UnsupportedOperationException();
-    }
-
-    public boolean addAll(int index, Collection<? extends ChildNodeEntry> c) {
-        throw new UnsupportedOperationException();
-    }
-
-    public void clear() {
-        throw new UnsupportedOperationException();
-    }
-
-    public ChildNodeEntry remove(int index) {
-        throw new UnsupportedOperationException();
-    }
-
-    public boolean remove(Object o) {
-        throw new UnsupportedOperationException();
-    }
-
-    public boolean removeAll(Collection<?> c) {
-        throw new UnsupportedOperationException();
-    }
-
-    public boolean retainAll(Collection<?> c) {
-        throw new UnsupportedOperationException();
-    }
-
-    public ChildNodeEntry set(int index, ChildNodeEntry element) {
-        throw new UnsupportedOperationException();
-    }
-
     //-------------------------------------------< java.lang.Object overrides >
     public boolean equals(Object obj) {
         if (this == obj) {
@@ -527,51 +420,4 @@ class ChildNodeEntries implements List<C
         }
     }
 
-    //--------------------------------------------------------< inner classes >
-    class EntriesIterator implements ListIterator<ChildNodeEntry> {
-
-        private final OrderedMapIterator mapIter;
-
-        EntriesIterator() {
-            mapIter = entries.orderedMapIterator();
-        }
-
-        public boolean hasNext() {
-            return mapIter.hasNext();
-        }
-
-        public ChildNodeEntry next() {
-            mapIter.next();
-            return (ChildNodeEntry) mapIter.getValue();
-        }
-
-        public boolean hasPrevious() {
-            return mapIter.hasPrevious();
-        }
-
-        public int nextIndex() {
-            return entries.indexOf(mapIter.getKey()) + 1;
-        }
-
-        public ChildNodeEntry previous() {
-            mapIter.previous();
-            return (ChildNodeEntry) mapIter.getValue();
-        }
-
-        public int previousIndex() {
-            return entries.indexOf(mapIter.getKey()) - 1;
-        }
-
-        public void add(ChildNodeEntry o) {
-            throw new UnsupportedOperationException();
-        }
-
-        public void remove() {
-            throw new UnsupportedOperationException();
-        }
-
-        public void set(ChildNodeEntry o) {
-            throw new UnsupportedOperationException();
-        }
-    }
 }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeState.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeState.java?rev=955307&r1=955306&r2=955307&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeState.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/state/NodeState.java
Wed Jun 16 17:16:20 2010
@@ -23,7 +23,6 @@ import org.apache.jackrabbit.spi.Name;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
@@ -298,7 +297,7 @@ public class NodeState extends ItemState
      * @see #removeChildNodeEntry
      */
     public synchronized List<ChildNodeEntry> getChildNodeEntries() {
-        return childNodeEntries;
+        return childNodeEntries.list();
     }
 
     /**
@@ -606,7 +605,7 @@ public class NodeState extends ItemState
      */
     public synchronized List<ChildNodeEntry> getAddedChildNodeEntries() {
         if (!hasOverlayedState()) {
-            return childNodeEntries;
+            return childNodeEntries.list();
         }
 
         NodeState other = (NodeState) getOverlayedState();
@@ -655,32 +654,10 @@ public class NodeState extends ItemState
      */
     public synchronized List<ChildNodeEntry> getRenamedChildNodeEntries() {
         if (!hasOverlayedState()) {
-            return Collections.emptyList();
-        }
-
-        ChildNodeEntries otherChildNodeEntries =
-                ((NodeState) overlayedState).childNodeEntries;
-
-        // do a lazy init
-        List<ChildNodeEntry> renamed = null;
-
-        for (Iterator<ChildNodeEntry> iter = childNodeEntries.iterator(); iter.hasNext();)
{
-            ChildNodeEntry cne = iter.next();
-            ChildNodeEntry cneOther = otherChildNodeEntries.get(cne.getId());
-            if (cneOther != null && !cne.getName().equals(cneOther.getName())) {
-                // child node entry with same id but different name exists in
-                // overlayed and this state => renamed entry detected
-                if (renamed == null) {
-                    renamed = new ArrayList<ChildNodeEntry>();
-                }
-                renamed.add(cne);
-            }
-        }
-
-        if (renamed == null) {
-            return Collections.emptyList();
+            return childNodeEntries.getRenamedEntries(
+                    ((NodeState) overlayedState).childNodeEntries);
         } else {
-            return renamed;
+            return Collections.emptyList();
         }
     }
 
@@ -735,8 +712,8 @@ public class NodeState extends ItemState
         // both entry lists now contain the set of nodes that have not
         // been removed or added, but they may have changed their position.
         for (int i = 0; i < ours.size();) {
-            ChildNodeEntry entry = (ChildNodeEntry) ours.get(i);
-            ChildNodeEntry other = (ChildNodeEntry) others.get(i);
+            ChildNodeEntry entry = ours.get(i);
+            ChildNodeEntry other = others.get(i);
             if (entry == other || entry.getId().equals(other.getId())) {
                 // no reorder, move to next child entry
                 i++;
@@ -753,10 +730,10 @@ public class NodeState extends ItemState
                 if (i + 1 < ours.size()) {
                     // if entry is the next in the other list then probably
                     // the other entry at position <code>i</code> was reordered
-                    if (entry.getId().equals(((ChildNodeEntry) others.get(i + 1)).getId()))
{
+                    if (entry.getId().equals(others.get(i + 1).getId())) {
                         // scan for the uuid of the other entry in our list
                         for (int j = i; j < ours.size(); j++) {
-                            if (((ChildNodeEntry) ours.get(j)).getId().equals(other.getId()))
{
+                            if (ours.get(j).getId().equals(other.getId())) {
                                 // found it
                                 entry = (ChildNodeEntry) ours.get(j);
                                 break;
@@ -769,12 +746,12 @@ public class NodeState extends ItemState
                 // remove the entry from both lists
                 // entries > i are already cleaned
                 for (int j = i; j < ours.size(); j++) {
-                    if (((ChildNodeEntry) ours.get(j)).getId().equals(entry.getId())) {
+                    if (ours.get(j).getId().equals(entry.getId())) {
                         ours.remove(j);
                     }
                 }
                 for (int j = i; j < ours.size(); j++) {
-                    if (((ChildNodeEntry) others.get(j)).getId().equals(entry.getId())) {
+                    if (others.get(j).getId().equals(entry.getId())) {
                         others.remove(j);
                     }
                 }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/version/InternalVersionHistoryImpl.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/version/InternalVersionHistoryImpl.java?rev=955307&r1=955306&r2=955307&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/version/InternalVersionHistoryImpl.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/version/InternalVersionHistoryImpl.java
Wed Jun 16 17:16:20 2010
@@ -154,9 +154,7 @@ class InternalVersionHistoryImpl extends
         rootVersion = createVersionInstance(NameConstants.JCR_ROOTVERSION);
 
         // get version entries
-        ChildNodeEntry[] children = (ChildNodeEntry[])
-            node.getState().getChildNodeEntries().toArray();
-        for (ChildNodeEntry child : children) {
+        for (ChildNodeEntry child : node.getState().getChildNodeEntries()) {
             if (child.getName().equals(NameConstants.JCR_VERSIONLABELS)) {
                 continue;
             }



Mime
View raw message