From 066845485745859ffd16fdceee0c8cb264b1bd7f Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Thu, 22 May 2014 23:50:05 +0200 Subject: [PATCH] BUG-509: fix ModifiedNode locking 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 --- .../data/InMemoryDataTreeModification.java | 38 ++++++------- .../store/impl/tree/data/ModifiedNode.java | 57 +++++++------------ 2 files changed, 36 insertions(+), 59 deletions(-) diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/InMemoryDataTreeModification.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/InMemoryDataTreeModification.java index fcb3ae0a10..d3e4bf0714 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/InMemoryDataTreeModification.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/InMemoryDataTreeModification.java @@ -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 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> readNode(final InstanceIdentifier path) { + public synchronized Optional> 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"); } diff --git a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/ModifiedNode.java b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/ModifiedNode.java index 0ff64e6a72..f83ea1a2de 100644 --- a/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/ModifiedNode.java +++ b/opendaylight/md-sal/sal-dom-broker/src/main/java/org/opendaylight/controller/md/sal/dom/store/impl/tree/data/ModifiedNode.java @@ -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, Identifiable children = new LinkedHashMap<>(); private final Optional original; - - private NormalizedNode value; - + private final PathArgument identifier; + private ModificationType modificationType = ModificationType.UNMODIFIED; private Optional snapshotCache; - - private final Map childModification; - - @GuardedBy("this") - private boolean sealed = false; + private NormalizedNode value; private ModifiedNode(final PathArgument identifier, final Optional original) { this.identifier = identifier; this.original = original; - childModification = new LinkedHashMap<>(); } /** @@ -91,7 +82,7 @@ final class ModifiedNode implements StoreTreeNode, Identifiable getOriginal() { + public Optional getOriginal() { return original; } @@ -101,7 +92,7 @@ final class ModifiedNode implements StoreTreeNode, Identifiable, Identifiable getChild(final PathArgument child) { - return Optional. fromNullable(childModification.get(child)); + return Optional. fromNullable(children.get(child)); } /** @@ -130,13 +121,12 @@ final class ModifiedNode implements StoreTreeNode, Identifiable, Identifiable, Identifiable getChildren() { - return childModification.values(); + return children.values(); } /** @@ -170,11 +160,10 @@ final class ModifiedNode implements StoreTreeNode, Identifiable, Identifiable 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, Identifiable