From 1ec69e62136c573b11af47c37e33a107ee52c9ea Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Tue, 7 Jan 2025 15:09:56 +0100 Subject: [PATCH] Synchronize InMemoryDataTreeModification transitions The three methods marked here all mutate ModifiedNode, with newModification() happening concurrently with validate() and prepare(). This patch introduces a heavy-weight guard to prevent concurrent access, so we serialize access between the threads. It is not a complete solution, as newModification() can still interfere wreck state inside a prepare()d candidate -- and we will deal with that in a follow-up patch. JIRA: YANGTOOLS-1651 Change-Id: If2132b3a6ac51c72dd8380b2ffdfb49158f023e8 Signed-off-by: Robert Varga --- .../impl/InMemoryDataTreeModification.java | 7 +-- .../yang/data/tree/impl/ModifiedNode.java | 48 +++++++++++++++++-- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModification.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModification.java index 16421c294e..c8fb9b4c22 100644 --- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModification.java +++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/InMemoryDataTreeModification.java @@ -216,7 +216,7 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements } @Override - public InMemoryDataTreeModification newModification() { + public synchronized InMemoryDataTreeModification newModification() { checkState(isSealed(), "Attempted to chain on an unsealed modification"); if (rootNode.getOperation() == LogicalOperation.NONE) { @@ -345,7 +345,8 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements * @throws DataValidationFailedException if modification would result in an inconsistent data tree */ @NonNullByDefault - void validate(final YangInstanceIdentifier path, final TreeNode current) throws DataValidationFailedException { + synchronized void validate(final YangInstanceIdentifier path, final TreeNode current) + throws DataValidationFailedException { if (!isSealed()) { // FIXME: this should be an IllegalStateException throw new IllegalArgumentException("Attempted to validate unsealed modification " + this); @@ -364,7 +365,7 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements * @throws DataValidationFailedException if modification would result in inconsistent data tree */ @NonNullByDefault - DataTreeCandidateTip prepare(final YangInstanceIdentifier path, final TreeNode current) { + synchronized DataTreeCandidateTip prepare(final YangInstanceIdentifier path, final TreeNode current) { if (!isSealed()) { // FIXME: this should be an IllegalStateException throw new IllegalArgumentException("Attempted to prepare unsealed modification " + this); diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java index babc42670c..a4d5c64fd6 100644 --- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java +++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java @@ -39,18 +39,58 @@ import org.opendaylight.yangtools.yang.data.tree.impl.node.Version; * the tree. */ final class ModifiedNode extends NodeModification implements StoreTreeNode { - private final Map children; + // This class has rather funky state management and thread safety rules, closely aligned with + // InMemoryDataTreeModification's lifecycle, which holds the pointer to the root of a tree of ModifiedNodes. Here + // we document our fields, but all inter-thread synchronization needs to be done in InMemoryDataTreeModification. + // + // Every ModifiedNode has two invariants, guaranteed to be immutable throughout its existence: + // - 'identifier', which is the PathArgument identifying the underlying NormalizedNode data + // - 'original', which is the TreeNode which is being modified -- or null if this is a modification of previously + // non-existent data private final @Nullable TreeNode original; private final @NonNull PathArgument identifier; + // Two other bits start off as potentially-mutable state: + // - 'operation', which is the LogicalOperation this ModifiedNode performs on the corresponding data + // - 'children', which tracks any nested modifications + // + // Both start as being mutable and assuming non-concurrent access, as is the case when a DataTreeModification is + // being built up from its constituent logical operations. + // + // Once DataTreeModification.ready() is invoked by the user we process the subtree, perform initial logical + // operation algebra and prune the tree so it records the effective logical operations the user intends to perform + // on top of 'original' -- see the seal() method below. + // + // After that transition the two fields are mostly stable, but can undergo further evolution as part of modification + // lifecycle: each of validate(), prepare() and newModification() can end up calling + // SchemaAwareApplyOperation.apply(), at which both of these can be updated. + // + // 'children' also serves a secondary view, as it is used by AbstractModifiedNodeBasedCandidateNode along with + // 'modType' below to provide DataTreeCandidateNode interface. + // + // NOTE: Original design called for 'children' to be effectively immutable after seal(), we can end up expanding + // them when a MERGE encounters pre-existing data: see how + // AbstractNodeContainerModificationStrategy.applyMerge() ends up calling to modifyChild() to turn the MERGE + // into effectively a TOUCH and with a series of child MERGE operations. + // + // If we ever want to restore this design, we need to replace AbstractModifiedNodeBasedCandidateNode with a + // tree of ImmutableCandidateNodes. That is a tough cookie, as we currently populate the effective state as + // a side-effect of apply(), but the DataTreeCandidateNode view is only needed in after the prepare() stage. + private final Map children; private LogicalOperation operation = LogicalOperation.NONE; - private Optional snapshotCache; - private NormalizedNode value; - private ModificationType modType; + // The argument to LogicalOperation.{MERGE,WRITE}, invalid otherwise + private NormalizedNode value; // Alternative history introduced in WRITE nodes. Instantiated when we touch any child underneath such a node. private TreeNode writtenOriginal; + // Cached result of the last SchemaAwareApplyOperation.apply() operation, for example if the user calls + // DataTreeSnapshot.readNode() multiple times on the same node. + private Optional snapshotCache; + + // Effective ModificationType, as resolved by the last executed SchemaAwareApplyOperation.apply() + private ModificationType modType; + // Internal cache for TreeNodes created as part of validation private ModificationApplyOperation validatedOp; private @Nullable TreeNode validatedCurrent; -- 2.36.6