BUG-509: fix ModifiedNode locking 50/7350/2
authorRobert Varga <rovarga@cisco.com>
Thu, 22 May 2014 21:50:05 +0000 (23:50 +0200)
committerRobert Varga <rovarga@cisco.com>
Fri, 23 May 2014 10:00:50 +0000 (12:00 +0200)
This removes the 'sealed' flag from ModifiedNode, as it is not a
publicly-visible object. Also drop the synchronized blocks and
reintroduce them in InMemoryDataTreeModification, which is the user
entrypoint.

Change-Id: I6c8be61701134bd8f645b8d4f0a8ef3ae218f0ac
Signed-off-by: Robert Varga <rovarga@cisco.com>
opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/InMemoryDataTreeModification.java
opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/ModifiedNode.java

index fcb3ae0..d3e4bf0 100644 (file)
@@ -7,10 +7,9 @@
  */
 package org.opendaylight.controller.md.sal.dom.store.impl.tree.data;
 
-import static com.google.common.base.Preconditions.checkState;
-
 import java.util.Map.Entry;
-import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
+
+import javax.annotation.concurrent.GuardedBy;
 
 import org.opendaylight.controller.md.sal.dom.store.impl.tree.DataTreeModification;
 import org.opendaylight.controller.md.sal.dom.store.impl.tree.StoreUtils;
@@ -29,19 +28,13 @@ import com.google.common.base.Preconditions;
 
 final class InMemoryDataTreeModification implements DataTreeModification {
     private static final Logger LOG = LoggerFactory.getLogger(InMemoryDataTreeModification.class);
-
-    /*
-     * FIXME: the thread safety of concurrent write/delete/read/seal operations
-     *        needs to be evaluated.
-     */
-    private static final AtomicIntegerFieldUpdater<InMemoryDataTreeModification> SEALED_UPDATER =
-            AtomicIntegerFieldUpdater.newUpdater(InMemoryDataTreeModification.class, "sealed");
-    private volatile int sealed = 0;
-
     private final ModificationApplyOperation strategyTree;
     private final InMemoryDataTreeSnapshot snapshot;
     private final ModifiedNode rootNode;
 
+    @GuardedBy("this")
+    private boolean sealed = false;
+
     InMemoryDataTreeModification(final InMemoryDataTreeSnapshot snapshot, final ModificationApplyOperation resolver) {
         this.snapshot = Preconditions.checkNotNull(snapshot);
         this.strategyTree = Preconditions.checkNotNull(resolver);
@@ -57,13 +50,13 @@ final class InMemoryDataTreeModification implements DataTreeModification {
     }
 
     @Override
-    public void write(final InstanceIdentifier path, final NormalizedNode<?, ?> value) {
+    public synchronized void write(final InstanceIdentifier path, final NormalizedNode<?, ?> value) {
         checkSealed();
         resolveModificationFor(path).write(value);
     }
 
     @Override
-    public void merge(final InstanceIdentifier path, final NormalizedNode<?, ?> data) {
+    public synchronized void merge(final InstanceIdentifier path, final NormalizedNode<?, ?> data) {
         checkSealed();
         mergeImpl(resolveModificationFor(path),data);
     }
@@ -82,13 +75,13 @@ final class InMemoryDataTreeModification implements DataTreeModification {
     }
 
     @Override
-    public void delete(final InstanceIdentifier path) {
+    public synchronized void delete(final InstanceIdentifier path) {
         checkSealed();
         resolveModificationFor(path).delete();
     }
 
     @Override
-    public Optional<NormalizedNode<?, ?>> readNode(final InstanceIdentifier path) {
+    public synchronized Optional<NormalizedNode<?, ?>> readNode(final InstanceIdentifier path) {
         /*
          * Walk the tree from the top, looking for the first node between root and
          * the requested path which has been modified. If no such node exists,
@@ -139,14 +132,15 @@ final class InMemoryDataTreeModification implements DataTreeModification {
     }
 
     @Override
-    public void seal() {
-        final boolean success = SEALED_UPDATER.compareAndSet(this, 0, 1);
-        Preconditions.checkState(success, "Attempted to seal an already-sealed Data Tree.");
+    public synchronized void seal() {
+        Preconditions.checkState(!sealed, "Attempted to seal an already-sealed Data Tree.");
+        sealed = true;
         rootNode.seal();
     }
 
+    @GuardedBy("this")
     private void checkSealed() {
-        checkState(sealed == 0, "Data Tree is sealed. No further modifications allowed.");
+        Preconditions.checkState(!sealed, "Data Tree is sealed. No further modifications allowed.");
     }
 
     @Override
@@ -155,7 +149,9 @@ final class InMemoryDataTreeModification implements DataTreeModification {
     }
 
     @Override
-    public DataTreeModification newModification() {
+    public synchronized DataTreeModification newModification() {
+        Preconditions.checkState(sealed, "Attempted to chain on an unsealed modification");
+
         // FIXME: transaction chaining
         throw new UnsupportedOperationException("Implement this as part of transaction chaining");
     }
index 0ff64e6..f83ea1a 100644 (file)
@@ -20,7 +20,6 @@ import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.PathArgument;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
 
 import com.google.common.base.Optional;
-import com.google.common.base.Preconditions;
 import com.google.common.base.Predicate;
 
 /**
@@ -50,25 +49,17 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
             throw new IllegalArgumentException(String.format("Unhandled modification type %s", input.getType()));
         }
     };
-    private final PathArgument identifier;
-    private ModificationType modificationType = ModificationType.UNMODIFIED;
-
 
+    private final Map<PathArgument, ModifiedNode> children = new LinkedHashMap<>();
     private final Optional<TreeNode> original;
-
-    private NormalizedNode<?, ?> value;
-
+    private final PathArgument identifier;
+    private ModificationType modificationType = ModificationType.UNMODIFIED;
     private Optional<TreeNode> snapshotCache;
-
-    private final Map<PathArgument, ModifiedNode> childModification;
-
-    @GuardedBy("this")
-    private boolean sealed = false;
+    private NormalizedNode<?, ?> value;
 
     private ModifiedNode(final PathArgument identifier, final Optional<TreeNode> original) {
         this.identifier = identifier;
         this.original = original;
-        childModification = new LinkedHashMap<>();
     }
 
     /**
@@ -91,7 +82,7 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
      * @return original store metadata
      */
     @Override
-    public final Optional<TreeNode> getOriginal() {
+    public Optional<TreeNode> getOriginal() {
         return original;
     }
 
@@ -101,7 +92,7 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
      * @return modification type
      */
     @Override
-    public final ModificationType getType() {
+    public ModificationType getType() {
         return modificationType;
     }
 
@@ -115,7 +106,7 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
      */
     @Override
     public Optional<ModifiedNode> getChild(final PathArgument child) {
-        return Optional.<ModifiedNode> fromNullable(childModification.get(child));
+        return Optional.<ModifiedNode> fromNullable(children.get(child));
     }
 
     /**
@@ -130,13 +121,12 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
      * @return {@link ModifiedNode} for specified child, with {@link #getOriginal()}
      *         containing child metadata if child was present in original data.
      */
-    public synchronized ModifiedNode modifyChild(final PathArgument child) {
-        checkSealed();
+    public ModifiedNode modifyChild(final PathArgument child) {
         clearSnapshot();
         if (modificationType == ModificationType.UNMODIFIED) {
             updateModificationType(ModificationType.SUBTREE_MODIFIED);
         }
-        final ModifiedNode potential = childModification.get(child);
+        final ModifiedNode potential = children.get(child);
         if (potential != null) {
             return potential;
         }
@@ -150,7 +140,7 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
         }
 
         ModifiedNode newlyCreated = new ModifiedNode(child, currentMetadata);
-        childModification.put(child, newlyCreated);
+        children.put(child, newlyCreated);
         return newlyCreated;
     }
 
@@ -162,7 +152,7 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
      */
     @Override
     public Iterable<ModifiedNode> getChildren() {
-        return childModification.values();
+        return children.values();
     }
 
     /**
@@ -170,11 +160,10 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
      * Records a delete for associated node.
      *
      */
-    public synchronized void delete() {
-        checkSealed();
+    public void delete() {
         clearSnapshot();
         updateModificationType(ModificationType.DELETE);
-        childModification.clear();
+        children.clear();
         this.value = null;
     }
 
@@ -184,31 +173,23 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
      *
      * @param value
      */
-    public synchronized void write(final NormalizedNode<?, ?> value) {
-        checkSealed();
+    public void write(final NormalizedNode<?, ?> value) {
         clearSnapshot();
         updateModificationType(ModificationType.WRITE);
-        childModification.clear();
+        children.clear();
         this.value = value;
     }
 
-    public synchronized void merge(final NormalizedNode<?, ?> data) {
-        checkSealed();
+    public void merge(final NormalizedNode<?, ?> data) {
         clearSnapshot();
         updateModificationType(ModificationType.MERGE);
         // FIXME: Probably merge with previous value.
         this.value = data;
     }
 
-    @GuardedBy("this")
-    private void checkSealed() {
-        Preconditions.checkState(!sealed, "Node Modification is sealed. No further changes allowed.");
-    }
-
-    public synchronized void seal() {
-        sealed = true;
+    void seal() {
         clearSnapshot();
-        for(ModifiedNode child : childModification.values()) {
+        for (ModifiedNode child : children.values()) {
             child.seal();
         }
     }
@@ -235,7 +216,7 @@ final class ModifiedNode implements StoreTreeNode<ModifiedNode>, Identifiable<Pa
     @Override
     public String toString() {
         return "NodeModification [identifier=" + identifier + ", modificationType="
-                + modificationType + ", childModification=" + childModification + "]";
+                + modificationType + ", childModification=" + children + "]";
     }
 
     public static ModifiedNode createUnmodified(final TreeNode metadataTree) {