Move isFluidHandle to shared-object-base by CraigMacomber · Pull Request #17328 · microsoft/FluidFramework

CraigMacomber · 2023-09-15T20:18:05Z

Description

FluidSeralizer now uses a more robust check and more documented types for its IFluidHandle detection.

Once an assert (to detect data where the behavior changed, which we expect currently not to be present) is removed, this will allow FluidSeralizer to safely handle arbitrary object keys (for example user provided strings). Before this change, if such data happend to be an object key "IFluidHandle", it would treat it as an IFluidHandle.

Further adopting the new FluidSerializableReadOnly in the API will be a breaking change, and thus is not part of this PR.

Breaking Changes

FluidSeralizer will now treat objects with a "IFluidHandle" field that is not a IFluidHandle (or is one but doesn't refer to itself in its .IFluidHandle field) as not a fluid handle.

This is not expected to break any usages, but any data impacted by this change will now assert.

Reviewer Guidance

The review process is outlined on this wiki page.

msfluid-bot · 2023-09-15T21:28:20Z

⯅ @fluid-example/bundle-size-tests: +3.49 KB

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	450.63 KB	451.35 KB	⯅ +737 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	240.94 KB	240.94 KB	■ No change
loader.js	167.41 KB	167.41 KB	⯅ +2 Bytes
map.js	48.05 KB	48.77 KB	⯅ +737 Bytes
matrix.js	141.84 KB	142.56 KB	⯅ +737 Bytes
odspDriver.js	90.35 KB	90.36 KB	⯅ +2 Bytes
odspPrefetchSnapshot.js	41.82 KB	41.82 KB	⯅ +2 Bytes
sharedString.js	162.77 KB	163.49 KB	⯅ +735 Bytes
sharedTree2.js	287.81 KB	288.41 KB	⯅ +621 Bytes
Total Size	1.75 MB	1.75 MB	⯅ +3.49 KB

Baseline commit: 09c8d09

Generated by 🚫 dangerJS against 710b1e7

api-report/shared-object-base.api.md

experimental/dds/tree2/src/feature-libraries/contextuallyTyped.ts

packages/dds/shared-object-base/src/serializer.ts

Co-authored-by: Mark Fields <markfields@users.noreply.github.com>

…to isFluidHandle

CraigMacomber · 2023-10-02T20:49:05Z

packages/dds/shared-object-base/src/serializer.ts

+		// To help detect if the limitation of this old handle detection was impacting behavior, keep it around for now:
+		const isHandleLegacy = (value as Partial<IFluidHandle>)?.IFluidHandle !== undefined;
+		const isHandle = isFluidHandle(value);
+		assert(isHandleLegacy === isHandle, "new isFluidHandle should not change existing policy.");


this check is breaking some "ci:tst:realsvc:local" tests related to GC.

I don't know if thats finding a real issue (we have code which uses handles which do not have cyclic refs), or test issue (the test is doing something test specific that breaks this)

I talked to @DLehenbauer About this and got some of the history for this pattern. Turns out that I think the assumption that these refs are cyclic might not be required to be true (but we could possibly add this requirement).

Alternatively we could make handle use symbols to identify themselves.

I have updated this PR to permit noncyclic handles, changing how the detection works and rewriting most of the documentation.

This still fails tests. All the docs in this code claim its trying to detect IFluidHandle, but the actual implementation seems to detect IProvideFluidHandle. My changes make it actually detect IFluidHandle, and this breaks a 14 of GC related end-to-end tests.

It is unclear to me if the tests are wrong, and happened to work due to incorrect detecting of IFluidHandles in this code, or if this code is documented incorrectly and should be made to detect and special case IProvideFluidHandle instances instead.

From the CI run, its the same test failing 14 times because it runs in different compat mode. I believe the test is incorrect. Let me take a stab at fixing it.

Here's the fix - #18551

Since its possible others might have made the same or similar mistakes in their code, I have added a changeset to this PR detailing that this could be a problem and how to fix it.

packages/dds/shared-object-base/src/serializer.ts

packages/dds/shared-object-base/src/test/serializer.spec.ts

markfields

Left some small suggestions, but looks good

…to isFluidHandle

.changeset/cyan-buses-wave.md

experimental/dds/tree2/src/simple-tree/proxies.ts

markfields · 2023-12-03T03:10:59Z

experimental/dds/tree2/src/simple-tree/proxies.ts

 		return extractContentMap(content);
 	} else if (content !== null && typeof content === "object") {
+		const flexNode = tryGetEditNode(content);
+		// Only run isFluidHandle if content can't be an unhydrated node,


I read the function description and code and can't quite tie this comment to what's happening here. Must be some implicit connection between if it's a Map and being "unhydrated"? Is "flex" a well-defined term? How does it relate to hydration etc?

markfields · 2023-12-03T03:26:43Z

packages/dds/shared-object-base/src/test/serializer.spec.ts

+				attachGraph: () => assert.fail(),
+				bind: () => assert.fail(),
+				absolutePath: "",
+			} satisfies IFluidHandle),


Wasn't familiar with satisfies, looked it up, very cool! Always glad to learn to TS features from you!

markfields · 2023-12-03T03:32:08Z

packages/dds/shared-object-base/src/serializer.ts

+ * Data Store serializer implementation.
+ *
+ * @privateRemarks
+ * Since this type is package exported (not just the Interface above),


It's @internal now! I had wanted to stop it from being public anyway, and it just happened in bulk. Maybe it's kind to wait until after internal.8.0 "just in case", but we should be good to change stuff like this with more liberty now AFAIK.

markfields · 2023-12-03T03:39:05Z

packages/dds/shared-object-base/src/serializer.ts

 		// Detect if 'value' is an IFluidHandle.
-		const handle = value?.IFluidHandle;
+		// To help detect if the limitation of this old handle detection was impacting behavior, keep it around for now:
+		const isHandleLegacy = (value as Partial<IFluidHandle>)?.IFluidHandle !== undefined;


I'm always torn on asserts like the one below. If there's a case out there that violates it (like you found), this code will obviously throw. But is the calling code prepared to properly handle that throw? It's hard to anticipate exactly how that error will manifest. But I guess what else can you do. It's a great validation that our test (now) don't hit it.

No suggestion here, it's fine as-is, just thinking out loud.

markfields · 2023-12-03T03:40:20Z

packages/dds/shared-object-base/src/serializer.ts

+ * (due to decodeValue's use of isSerializedHandle causing them to be wrongly parsed as handles) and should be avoided.
+ * @public
+ */
+export type FluidSerializableReadOnly =


@jason-ha has a PR out about some similar recursive types, is this new type harmonious with whatever he's doing? I haven't looked as his PR yet.

This will not work with the current type test support.
TypeOnly utility will not be able to handle anything this such a type in it. (Although currently if this only appears two levels of properties down the error may not be observed.)
See #18523 that injects an interface where there is recursion to workaround the TypeOnly issue.

markfields · 2023-12-03T03:42:08Z

packages/dds/shared-object-base/src/serializer.ts

+	| null
+	| readonly FluidSerializableReadOnly[]
+	| { readonly [P in string]?: FluidSerializableReadOnly };
+


How would you contrast [P in string] v. something like [P in keyof object]? I guess Symbol keys would be excluded, not sure how it would play out. How did you land on P in string?

markfields · 2023-12-03T03:43:02Z

packages/dds/shared-object-base/src/serializer.ts

+/**
+ * Check if a value in {@link FluidSerializableReadOnly} data is an {@link @fluidframework/core-interfaces#IFluidHandle}.
+ *
+ * Warning: Non-IFluidHandle objects with a key "IFluidHandle" will cause an assert.


Why assert instead of merely returning false? Seems unnecessarily disruptive if a violation were to happen.

Ok I read the comment in the implementation. Similar to the assert in encodeValue - it's about failing fast during this transition to stricter runtime checks.

Do you expect to come back later and remove these asserts?

markfields · 2023-12-03T04:55:11Z

packages/dds/shared-object-base/src/serializer.ts

+		return false;
+	}
+	const partialHandle = value as Partial<IFluidHandle>;
+	const innerHandle = partialHandle.IFluidHandle;


Given the pattern where innerHandle === value, calling it innerHandle could be misleading to an outside. Maybe add a comment saying something like nnerHandle is expected to be a cyclical reference to value itself, not a secondary "inner" handle.

markfields · 2023-12-03T04:58:19Z

packages/dds/shared-object-base/src/serializer.ts

+		);
+
+		assert(
+			innerHandle.IFluidHandle?.IFluidHandle?.IFluidHandle?.IFluidHandle !== undefined,


markfields · 2023-12-03T04:59:37Z

packages/dds/shared-object-base/src/serializer.ts

+ * @privateRemarks
+ * Any of the following changes to IFluidHandle would solve the ambiguity problem:
+ *
+ * - The "IFluidHandle" property could be required to be a cyclic reference back to the parent.


It's pretty tempting to just enforce this. Maybe after the next release we enforce that further restriction.

markfields · 2023-12-03T05:02:48Z

packages/dds/shared-object-base/src/serializer.ts

+	// If the object fails this check, it will assert to detect problematic data that would have incorrectly been treated as a handle in previous versions of the serializer.
+
+	// Since json compatible data shouldn't have methods, and IFluidHandle requires one, use that as a redundant check:
+	const getMember = (value as Partial<IFluidHandle>).get;


Since isFluidHandle is called repeatedly/recursively on a critical path (op processing), I'm wondering if we need to deliberately optimize it. Putting this comment here because maybe we put this under the if (value !== innerHandle) check so it (likely) never actually has to run.

But the comment appllies in general, but the happy path of the function looks pretty lean. Do we have any perf benchmarks that cover this code that we could observe before/after? I think we do but not sure where or how to intepret them.

markfields

Left plenty of comments to consider, but nothing blocking. Nice incremental change to make!

jason-ha

FluidSerializableReadOnly needs altered to allow TypeOnly to wrap it.
Currently:

declare function use_current_TypeAlias_FluidSerializableReadOnly(
    use: TypeOnly<current.FluidSerializableReadOnly>): void;

will result in

@fluidframework/shared-object-base: [cjs]: src/test/types/validateSharedObjectBasePrevious.generated.ts:25:10 - error TS2589: Type instantiation is excessively deep and possibly infinite.
@fluidframework/shared-object-base: 
@fluidframework/shared-object-base: 25     use: TypeOnly<current.FluidSerializableReadOnly>): void;

(The type test generator should really create a declaration for all types in current, even if not present in the old to expose such an issue.)

…to isFluidHandle

jason-ha · 2023-12-04T19:14:47Z

packages/dds/shared-object-base/src/serializer.ts

+	const getMember = (value as Partial<IFluidHandle>).get;
+	assert(
+		typeof getMember === "function",
+		"Fluid handle detection found IFluidHandle field, but not have a get method",


...but +does+ not have or similar wording fix needed

CraigMacomber · 2024-01-17T01:01:41Z

I think the best thing to do here is fix the fact that its impossible to robustly detect handles, which can be done by using a class or symbol for the handles. I believe https://dev.azure.com/fluidframework/internal/_workitems/edit/1507 is tracking this internally. If we are unable to make handles more robust, then maybe landing something like this PR makes sense.

microsoft-github-policy-service · 2024-03-17T17:03:36Z

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

Move isFluidHandle to shared-object-base

cb33762

CraigMacomber marked this pull request as ready for review September 15, 2023 20:18

CraigMacomber requested review from a team as code owners September 15, 2023 20:18

github-actions bot added area: dds Issues related to distributed data structures area: dds: tree public api change Changes to a public API base: main PRs targeted against main branch labels Sep 15, 2023

Fix test

f0b77ff

markfields reviewed Sep 20, 2023

View reviewed changes

api-report/shared-object-base.api.md Show resolved Hide resolved

markfields reviewed Sep 20, 2023

View reviewed changes

experimental/dds/tree2/src/feature-libraries/contextuallyTyped.ts Outdated Show resolved Hide resolved

markfields reviewed Sep 20, 2023

View reviewed changes

packages/dds/shared-object-base/src/serializer.ts Show resolved Hide resolved

markfields reviewed Sep 20, 2023

View reviewed changes

packages/dds/shared-object-base/src/serializer.ts Outdated Show resolved Hide resolved

CraigMacomber and others added 2 commits September 19, 2023 22:53

Add type tests for FluidSerializableReadOnly

a2af89e

Update packages/dds/shared-object-base/src/serializer.ts

f36595d

Co-authored-by: Mark Fields <markfields@users.noreply.github.com>

markfields approved these changes Sep 21, 2023

View reviewed changes

CraigMacomber added 3 commits September 26, 2023 12:11

Add assert and round-trip warnings

6bca08e

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

5323709

…to isFluidHandle

Fix compile error from unused value in type tests

1eb1f80

CraigMacomber commented Oct 2, 2023

View reviewed changes

markfields reviewed Oct 2, 2023

View reviewed changes

packages/dds/shared-object-base/src/serializer.ts Outdated Show resolved Hide resolved

markfields reviewed Oct 2, 2023

View reviewed changes

packages/dds/shared-object-base/src/test/serializer.spec.ts Show resolved Hide resolved

markfields previously approved these changes Oct 2, 2023

View reviewed changes

CraigMacomber added 5 commits November 29, 2023 11:20

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

87ea4a3

…to isFluidHandle

Tolerate non-cyclic handles

a08aa2e

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

8c9d59e

…to isFluidHandle

Update for changes from main

ea88a1b

Fix tests

a960f65

markfields self-assigned this Dec 3, 2023

markfields reviewed Dec 3, 2023

View reviewed changes

.changeset/cyan-buses-wave.md Outdated Show resolved Hide resolved

markfields reviewed Dec 3, 2023

View reviewed changes

.changeset/cyan-buses-wave.md Outdated Show resolved Hide resolved

markfields reviewed Dec 3, 2023

View reviewed changes

experimental/dds/tree2/src/simple-tree/proxies.ts Show resolved Hide resolved

markfields reviewed Dec 3, 2023

View reviewed changes

markfields approved these changes Dec 3, 2023

View reviewed changes

CraigMacomber added 2 commits December 4, 2023 10:21

Update .changeset/cyan-buses-wave.md

6296422

Update .changeset/cyan-buses-wave.md

efa6981

jason-ha requested changes Dec 4, 2023

View reviewed changes

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

0efa3d5

…to isFluidHandle

jason-ha reviewed Dec 4, 2023

View reviewed changes

jason-ha mentioned this pull request Dec 4, 2023

improvement(client)!: enhance Jsonable types #18523

Merged

Fix merge

ffe6abd

github-actions bot added the community-contribution label Dec 4, 2023

CraigMacomber marked this pull request as draft January 17, 2024 01:02

microsoft-github-policy-service bot added the status: stale label Mar 17, 2024

microsoft-github-policy-service bot closed this Mar 26, 2024

Comments

Conversation

CraigMacomber commented Sep 15, 2023

Description

Breaking Changes

Reviewer Guidance

Uh oh!

msfluid-bot commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

markfields left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markfields left a comment

Choose a reason for hiding this comment

Uh oh!

jason-ha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CraigMacomber commented Jan 17, 2024

Uh oh!

microsoft-github-policy-service bot commented Mar 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

msfluid-bot commented Sep 15, 2023 •

edited

Loading