From 969c359ca1b5167c7caeb3bb39cf7e18ba3ec10a Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Fri, 11 Apr 2025 16:14:42 -0700 Subject: [PATCH 1/5] Fix cache lifetime and deleted node errors for nodes after view disposal. --- .../feature-libraries/flex-tree/context.ts | 12 ++- .../flex-tree/flexTreeTypes.ts | 8 ++ .../src/shared-tree/checkoutFlexTreeView.ts | 6 ++ .../src/simple-tree/core/treeNodeKernel.ts | 74 ++++++++++++------- .../simple-tree/core/unhydratedFlexTree.ts | 6 +- .../dds/tree/src/simple-tree/objectNode.ts | 6 +- .../src/test/shared-tree/sharedTree.spec.ts | 55 +++++++++++++- 7 files changed, 132 insertions(+), 35 deletions(-) diff --git a/packages/dds/tree/src/feature-libraries/flex-tree/context.ts b/packages/dds/tree/src/feature-libraries/flex-tree/context.ts index 7cb071acfa2a..d04faa0e212c 100644 --- a/packages/dds/tree/src/feature-libraries/flex-tree/context.ts +++ b/packages/dds/tree/src/feature-libraries/flex-tree/context.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. */ -import { assert } from "@fluidframework/core-utils/internal"; +import { assert, debugAssert } from "@fluidframework/core-utils/internal"; import { type ForestEvents, @@ -43,6 +43,11 @@ export interface FlexTreeContext { * If false, this context was created for use in a unhydrated tree, and the full document schema is unknown. */ isHydrated(): this is FlexTreeHydratedContext; + + /** + * If true, none of the nodes in this context can be used. + */ + isDisposed(): boolean; } /** @@ -106,9 +111,14 @@ export class Context implements FlexTreeHydratedContext, IDisposable { } public isHydrated(): this is FlexTreeHydratedContext { + debugAssert(() => !this.disposed || "Disposed"); return true; } + public isDisposed(): boolean { + return this.disposed; + } + public get schema(): TreeStoredSchema { return this.checkout.storedSchema; } diff --git a/packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts b/packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts index b369c156eabf..d37172fd3ac8 100644 --- a/packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts +++ b/packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts @@ -94,6 +94,14 @@ export enum TreeStatus { /** * Is removed and cannot be added back to the original document tree. + * @remarks + * Nodes can enter this state for multiple reasons: + * - The node was removed and nothing (ex: undo/redo history) kept it from being cleaned up. + * - The {@link TreeView} was disposed or had a schema change which made the tree incompatible. + * @privateRemarks + * There was planned work (AB#17948) to make the first reason a node could become "Deleted" impossible, + * at least as an opt in feature, + * by lifetime extending all nodes which are still possible to reach automatically. */ Deleted = 2, diff --git a/packages/dds/tree/src/shared-tree/checkoutFlexTreeView.ts b/packages/dds/tree/src/shared-tree/checkoutFlexTreeView.ts index 06bc0057203e..67113ad330fd 100644 --- a/packages/dds/tree/src/shared-tree/checkoutFlexTreeView.ts +++ b/packages/dds/tree/src/shared-tree/checkoutFlexTreeView.ts @@ -34,6 +34,8 @@ export class CheckoutFlexTreeView { + assert(!this.disposed, "disposed"); const branch = this.checkout.branch(); return new CheckoutFlexTreeView(branch, this.schema, this.nodeKeyManager); } diff --git a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts index a19658ea6b2e..fdbf6b0128ca 100644 --- a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts @@ -3,7 +3,8 @@ * Licensed under the MIT License. */ -import { assert, Lazy, fail } from "@fluidframework/core-utils/internal"; +import { assert, Lazy, fail, debugAssert } from "@fluidframework/core-utils/internal"; +import { UsageError } from "@fluidframework/telemetry-utils/internal"; import { createEmitter } from "@fluid-internal/client-utils"; import type { Listenable, Off } from "@fluidframework/core-interfaces"; import type { InternalTreeNode, TreeNode, Unhydrated } from "./types.js"; @@ -72,7 +73,7 @@ export function tryGetTreeNodeSchema(value: unknown): undefined | TreeNodeSchema /** The {@link HydrationState} of a {@link TreeNodeKernel} before the kernel is hydrated */ interface UnhydratedState { off: Off; - innerNode: UnhydratedFlexTreeNode; + readonly innerNode: UnhydratedFlexTreeNode; } /** The {@link HydrationState} of a {@link TreeNodeKernel} after the kernel is hydrated */ @@ -80,9 +81,9 @@ interface HydratedState { /** The flex node for this kernel (lazy - undefined if it has not yet been demanded) */ innerNode?: FlexTreeNode; /** The {@link AnchorNode} that this node is associated with. */ - anchorNode: AnchorNode; + readonly anchorNode: AnchorNode; /** All {@link Off | event deregistration functions} that should be run when the kernel is disposed. */ - offAnchorNode: Set; + readonly offAnchorNode: Set; } /** State within a {@link TreeNodeKernel} that is related to the hydration process */ @@ -287,35 +288,46 @@ export class TreeNodeKernel { * * For hydrated nodes it returns a FlexTreeNode backed by the forest. * Note that for "marinated" nodes, this FlexTreeNode exists and returns it: it does not return the MapTreeNode which is the current InnerNode. + * + * If `allowDeleted` is false, this will throw a UsageError if the node is deleted. */ - public getOrCreateInnerNode(allowFreed = false): InnerNode { + public getOrCreateInnerNode(allowDeleted = false): InnerNode { if (!isHydrated(this.#hydrationState)) { + debugAssert( + () => + this.#hydrationState.innerNode?.context.isDisposed() === false || + "Unhydrated node should never be disposed", + ); return this.#hydrationState.innerNode; // Unhydrated case } - if (this.#hydrationState.innerNode !== undefined) { - return this.#hydrationState.innerNode; // Cooked case + if (this.#hydrationState.innerNode === undefined) { + // Marinated case -> cooked + const anchorNode = this.#hydrationState.anchorNode; + // The proxy is bound to an anchor node, but it may or may not have an actual flex node yet + const flexNode = anchorNode.slots.get(flexTreeSlot); + if (flexNode !== undefined) { + // If the flex node already exists, use it... + this.#hydrationState.innerNode = flexNode; + } else { + // ...otherwise, the flex node must be created + const context = + anchorNode.anchorSet.slots.get(ContextSlot) ?? fail(0xb41 /* missing context */); + const cursor = context.checkout.forest.allocateCursor("getFlexNode"); + context.checkout.forest.moveCursorToPath(anchorNode, cursor); + this.#hydrationState.innerNode = makeTree(context, cursor); + cursor.free(); + // Calling this is a performance improvement, however, do this only after demand to avoid momentarily having no anchors to anchorNode + anchorForgetters?.get(this.node)?.(); + if (!allowDeleted) { + assertFlexTreeEntityNotFreed(this.#hydrationState.innerNode); + } + } } - // Marinated case -> cooked - const anchorNode = this.#hydrationState.anchorNode; - // The proxy is bound to an anchor node, but it may or may not have an actual flex node yet - const flexNode = anchorNode.slots.get(flexTreeSlot); - if (flexNode !== undefined) { - // If the flex node already exists, use it... - this.#hydrationState.innerNode = flexNode; - } else { - // ...otherwise, the flex node must be created - const context = - anchorNode.anchorSet.slots.get(ContextSlot) ?? fail(0xb41 /* missing context */); - const cursor = context.checkout.forest.allocateCursor("getFlexNode"); - context.checkout.forest.moveCursorToPath(anchorNode, cursor); - this.#hydrationState.innerNode = makeTree(context, cursor); - cursor.free(); - // Calling this is a performance improvement, however, do this only after demand to avoid momentarily having no anchors to anchorNode - anchorForgetters?.get(this.node)?.(); - if (!allowFreed) { - assertFlexTreeEntityNotFreed(this.#hydrationState.innerNode); + if (!allowDeleted) { + if (this.#hydrationState.innerNode.context.isDisposed()) { + throw new UsageError("Cannot access a Deleted node."); } } @@ -417,11 +429,15 @@ export const unhydratedFlexTreeNodeToTreeNode = */ export const proxySlot = anchorSlot(); +/** + * Dispose a TreeNode (if any) for an existing anchor without disposing the anchor. + */ export function tryDisposeTreeNode(anchorNode: AnchorNode): void { const treeNode = anchorNode.slots.get(proxySlot); if (treeNode !== undefined) { const kernel = getKernel(treeNode); kernel.dispose(); + anchorNode.slots.delete(proxySlot); } } @@ -454,10 +470,12 @@ export function getSimpleContextFromInnerNode(innerNode: InnerNode): Context { * * For hydrated nodes it returns a FlexTreeNode backed by the forest. * Note that for "marinated" nodes, this FlexTreeNode exists and returns it: it does not return the MapTreeNode which is the current InnerNode. + * + * If `allowDeleted` is false, this will throw a UsageError if the node is deleted. */ -export function getOrCreateInnerNode(treeNode: TreeNode, allowFreed = false): InnerNode { +export function getOrCreateInnerNode(treeNode: TreeNode, allowDeleted = false): InnerNode { const kernel = getKernel(treeNode); - return kernel.getOrCreateInnerNode(allowFreed); + return kernel.getOrCreateInnerNode(allowDeleted); } /** diff --git a/packages/dds/tree/src/simple-tree/core/unhydratedFlexTree.ts b/packages/dds/tree/src/simple-tree/core/unhydratedFlexTree.ts index 9a4af008341e..df4141ac8d36 100644 --- a/packages/dds/tree/src/simple-tree/core/unhydratedFlexTree.ts +++ b/packages/dds/tree/src/simple-tree/core/unhydratedFlexTree.ts @@ -73,7 +73,7 @@ interface LocationInField { * * Create a `UnhydratedFlexTreeNode` by calling {@link getOrCreate}. */ -export class UnhydratedFlexTreeNode implements UnhydratedFlexTreeNode { +export class UnhydratedFlexTreeNode implements FlexTreeNode { public get schema(): TreeNodeSchemaIdentifier { return this.mapTree.type; } @@ -277,6 +277,10 @@ export class UnhydratedContext implements FlexTreeContext { public readonly schema: TreeStoredSchema, ) {} + public isDisposed(): boolean { + return false; + } + public isHydrated(): this is FlexTreeHydratedContext { return false; } diff --git a/packages/dds/tree/src/simple-tree/objectNode.ts b/packages/dds/tree/src/simple-tree/objectNode.ts index e8b8aac0be78..091d83507a6d 100644 --- a/packages/dds/tree/src/simple-tree/objectNode.ts +++ b/packages/dds/tree/src/simple-tree/objectNode.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. */ -import { assert, Lazy, fail } from "@fluidframework/core-utils/internal"; +import { assert, Lazy, fail, debugAssert } from "@fluidframework/core-utils/internal"; import { UsageError } from "@fluidframework/telemetry-utils/internal"; import type { FieldKey, SchemaPolicy } from "../core/index.js"; @@ -174,6 +174,8 @@ function createFlexKeyMapping(fields: Record): Simp } /** + * Creates a proxy handler for the given schema. + * * @param allowAdditionalProperties - If true, setting of unexpected properties will be forwarded to the target object. * Otherwise setting of unexpected properties will error. * TODO: consider implementing this using `Object.preventExtension` instead. @@ -196,9 +198,9 @@ function createProxyHandler( const handler: ProxyHandler = { get(target, propertyKey, proxy): unknown { const fieldInfo = schema.flexKeyMap.get(propertyKey); - if (fieldInfo !== undefined) { const flexNode = getOrCreateInnerNode(proxy); + debugAssert(() => !flexNode.context.isDisposed() || "FlexTreeNode is disposed"); const field = flexNode.tryGetField(fieldInfo.storedKey); if (field !== undefined) { return getTreeNodeForField(field); diff --git a/packages/dds/tree/src/test/shared-tree/sharedTree.spec.ts b/packages/dds/tree/src/test/shared-tree/sharedTree.spec.ts index b44df3160ad0..a80388a35a22 100644 --- a/packages/dds/tree/src/test/shared-tree/sharedTree.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/sharedTree.spec.ts @@ -35,6 +35,7 @@ import { // eslint-disable-next-line import/no-internal-modules } from "../../feature-libraries/chunked-forest/chunkedForest.js"; import { + flexTreeSlot, MockNodeIdentifierManager, TreeCompressionStrategy, TreeStatus, @@ -109,6 +110,9 @@ import { // eslint-disable-next-line import/no-internal-modules } from "../../simple-tree/api/index.js"; import type { IChannel } from "@fluidframework/datastore-definitions/internal"; +import { configureDebugAsserts } from "@fluidframework/core-utils/internal"; +// eslint-disable-next-line import/no-internal-modules +import { proxySlot } from "../../simple-tree/core/treeNodeKernel.js"; const enableSchemaValidation = true; @@ -127,6 +131,15 @@ class MockSharedTreeRuntime extends MockFluidDataStoreRuntime { } describe("SharedTree", () => { + let debugAssertsDefault: boolean; + beforeEach(() => { + debugAssertsDefault = configureDebugAsserts(true); + }); + + afterEach(() => { + configureDebugAsserts(debugAssertsDefault); + }); + describe("viewWith", () => { it("initialize tree", () => { const tree = treeTestFactory(); @@ -158,9 +171,28 @@ describe("SharedTree", () => { assert.deepEqual(view2.root, 10); }); - // TODO (AB#31456): Enable this test once the bug is fixed. - it.skip("initialize-dispose-view with object schema", () => { + it("re-view after view disposal with TreeNodes", () => { const tree = treeTestFactory(); + + // Scan AnchorSet and check its slots for cached invalid data. + function checkAnchors(allowNodes: boolean) { + const anchors = tree.kernel.checkout.forest.anchors; + for (const anchor of anchors) { + const node = anchor.slots.get(flexTreeSlot); + if (node !== undefined) { + assert(node.context.isDisposed() === false); + assert(allowNodes); + } + const proxy = anchor.slots.get(proxySlot); + if (proxy !== undefined) { + assert.equal(Tree.status(proxy), TreeStatus.InDocument); + assert(allowNodes); + } + } + } + + checkAnchors(false); + assert.deepEqual(tree.contentSnapshot().schema.rootFieldSchema, storedEmptyFieldSchema); const factory = new SchemaFactory("my-factory"); @@ -180,10 +212,27 @@ describe("SharedTree", () => { view1.initialize(new MySchema({ number: 10 })); assert.deepEqual(view1.root, expectedContents); + const root1 = view1.root; + + assert(Tree.status(root1) === TreeStatus.InDocument); + + checkAnchors(true); + view1.dispose(); + assert(Tree.status(root1) === TreeStatus.Deleted); + assert.throws(() => root1.number, validateUsageError(/Deleted/)); + + checkAnchors(false); + const view2 = tree.viewWith(config); - assert.deepEqual(view2.root, expectedContents); // <-- This throws with assert 0x778 + + checkAnchors(false); + + const root2 = view2.root; + assert.notEqual(root1, root2); + assert.equal(view2.root.number, 10); + assert.deepEqual(view2.root, expectedContents); }); it("concurrent initialize", () => { From ad06564c8e8b3a19cb2b73e9152664afd57fdc71 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 14 Apr 2025 11:10:45 -0700 Subject: [PATCH 2/5] Update packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com> --- .../dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts b/packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts index d37172fd3ac8..93032980befc 100644 --- a/packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts +++ b/packages/dds/tree/src/feature-libraries/flex-tree/flexTreeTypes.ts @@ -96,7 +96,7 @@ export enum TreeStatus { * Is removed and cannot be added back to the original document tree. * @remarks * Nodes can enter this state for multiple reasons: - * - The node was removed and nothing (ex: undo/redo history) kept it from being cleaned up. + * - The node was removed and nothing (e.g. undo/redo history) kept it from being cleaned up. * - The {@link TreeView} was disposed or had a schema change which made the tree incompatible. * @privateRemarks * There was planned work (AB#17948) to make the first reason a node could become "Deleted" impossible, From 7d6b81d3a9268eb8e51523fdd158ff44b8329fe5 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 14 Apr 2025 11:23:01 -0700 Subject: [PATCH 3/5] Add changeset --- .changeset/true-guests-learn.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 .changeset/true-guests-learn.md diff --git a/.changeset/true-guests-learn.md b/.changeset/true-guests-learn.md new file mode 100644 index 000000000000..edb5cfaf34da --- /dev/null +++ b/.changeset/true-guests-learn.md @@ -0,0 +1,14 @@ +--- +"@fluidframework/tree": minor +"fluid-framework": minor +"__section": tree +--- +Improve handling of of deleted nodes + +[TreeNodes](https://fluidframework.com/docs/api/fluid-framework/treenode-class) which are [deleted](https://fluidframework.com/docs/api/fluid-framework/treestatus-enum#deleted-enummember) were not handled properly. +This has been improved in two ways: + +1. Accessing fields of deleted nodes now consistently gives a usage error indicating that this is invalid. +Previously this would assert indicating a bug in the implementation. +2. When a `TreeNode` is deleted, but the node still exists within the [ITree](https://fluidframework.com/docs/api/driver-definitions/itree-interface), then becomes accessible again later, an a new TreeNode now is allocated instead of reusing the deleted one. +Note that this can only happen when the entire view of the tree is disposed then recreated, for example when disposing and recreating a [TreeView](https://fluidframework.com/docs/api/fluid-framework/treeview-interface) or when the contents of the view are disposed due to being out of schema (another client did a schema upgrade), then brought back into schema (the schema upgrade was undone). From 0a43e317e9a53779a03aa3028bc5fa35bb66885f Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 14 Apr 2025 11:28:05 -0700 Subject: [PATCH 4/5] Edit changeset --- .changeset/true-guests-learn.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.changeset/true-guests-learn.md b/.changeset/true-guests-learn.md index edb5cfaf34da..0e1756f324ce 100644 --- a/.changeset/true-guests-learn.md +++ b/.changeset/true-guests-learn.md @@ -3,12 +3,13 @@ "fluid-framework": minor "__section": tree --- -Improve handling of of deleted nodes +Improve handling of deleted nodes -[TreeNodes](https://fluidframework.com/docs/api/fluid-framework/treenode-class) which are [deleted](https://fluidframework.com/docs/api/fluid-framework/treestatus-enum#deleted-enummember) were not handled properly. +[TreeNodes](https://fluidframework.com/docs/api/fluid-framework/treenode-class) which are [deleted](https://fluidframework.com/docs/api/fluid-framework/treestatus-enum#deleted-enummember) were not handled correctly. This has been improved in two ways: 1. Accessing fields of deleted nodes now consistently gives a usage error indicating that this is invalid. Previously this would assert indicating a bug in the implementation. 2. When a `TreeNode` is deleted, but the node still exists within the [ITree](https://fluidframework.com/docs/api/driver-definitions/itree-interface), then becomes accessible again later, an a new TreeNode now is allocated instead of reusing the deleted one. -Note that this can only happen when the entire view of the tree is disposed then recreated, for example when disposing and recreating a [TreeView](https://fluidframework.com/docs/api/fluid-framework/treeview-interface) or when the contents of the view are disposed due to being out of schema (another client did a schema upgrade), then brought back into schema (the schema upgrade was undone). +Note that this can only happen when the entire view of the `ITree` is disposed then recreated. +This happens when disposing and recreating a [TreeView](https://fluidframework.com/docs/api/fluid-framework/treeview-interface) or when the contents of the view are disposed due to being out of schema (another client did a schema upgrade), then brought back into schema (the schema upgrade was undone). From 850d79bd3ac9f00e1c40fb706079aaa6cb9a3202 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 15 Apr 2025 13:49:26 -0700 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com> --- .changeset/true-guests-learn.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.changeset/true-guests-learn.md b/.changeset/true-guests-learn.md index 0e1756f324ce..8ce831d477fd 100644 --- a/.changeset/true-guests-learn.md +++ b/.changeset/true-guests-learn.md @@ -8,8 +8,8 @@ Improve handling of deleted nodes [TreeNodes](https://fluidframework.com/docs/api/fluid-framework/treenode-class) which are [deleted](https://fluidframework.com/docs/api/fluid-framework/treestatus-enum#deleted-enummember) were not handled correctly. This has been improved in two ways: -1. Accessing fields of deleted nodes now consistently gives a usage error indicating that this is invalid. -Previously this would assert indicating a bug in the implementation. -2. When a `TreeNode` is deleted, but the node still exists within the [ITree](https://fluidframework.com/docs/api/driver-definitions/itree-interface), then becomes accessible again later, an a new TreeNode now is allocated instead of reusing the deleted one. +1. Accessing fields of deleted nodes now consistently throws a usage error indicating that this is invalid. +Previously this would throw an assertion error, which was a bug. +2. When a `TreeNode` is deleted, but the node still exists within the [`ITree`](https://fluidframework.com/docs/api/driver-definitions/itree-interface), then becomes accessible again later, now a new `TreeNode` is allocated instead of trying to reuse the deleted one. Note that this can only happen when the entire view of the `ITree` is disposed then recreated. This happens when disposing and recreating a [TreeView](https://fluidframework.com/docs/api/fluid-framework/treeview-interface) or when the contents of the view are disposed due to being out of schema (another client did a schema upgrade), then brought back into schema (the schema upgrade was undone).