Store handles in detached DDS before attaching #21132

jzaffiro · 2024-05-16T22:45:30Z

Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list.

AB#8009

…ds to attach

packages/test/test-end-to-end-tests/src/test/handleValidation.spec.ts

anthony-murphy · 2024-05-20T17:46:54Z

packages/test/test-end-to-end-tests/src/test/handleValidation.spec.ts

+ const attachedDataStore =
+ (await container1.getEntryPoint()) as ITestFluidObject;
+
+ const dataStoreB =


i don't think we want a new data store here and we should just use the attachedDataStore's runtime. The case with a new datastore is interesting, but i think that is covered in the pervious, at least to some degree. The differenence is if the to whole data store is detached, it will get one big summary of the whole datastore and send an attach op for that, but if the datastore is already attached, then each dds will get its own attach op.

Something interesting about this - with the new data store, the majority of the test cases fail, except for the ones that use map and directory (the ones that have the bindHandles workaround for AB#7452). Without this data store, they all pass, but I'm not sure if we still need coverage for not binding handles from all of the other DDSs.

interesting. it might be useful to have both tests then, those that use a new datastore, and those that do not.

…use a new data store

jzaffiro · 2024-05-20T22:01:48Z

packages/test/test-end-to-end-tests/src/test/handleValidation.spec.ts

+ await dataStore.getSharedObject<IConsensusRegisterCollection<FluidObject>>(
+ registerId,
+ );
+ return this.downCast(register);
 },
 },
 // {


ConsensusQueue has an issue that causes the tests to hang. I'm looking into it, but commented it out for now.

… moreAttachTests

jzaffiro · 2024-05-21T19:13:12Z

packages/runtime/datastore/src/dataStoreRuntime.ts

- if (!(context instanceof LocalChannelContextBase)) {
- throw new LoggingError("Should only be called with local channel handles");
- }
+ const visitedContexts = new Set<string>();


@agarwal-navin: @anthony-murphy and I were wondering if this is an ok way to solve the problem of contexts for handles stored in other detached DDSs not being seen. The goal is to ensure that all contexts, even those for handles stored in a detached DDS, are able to be resolved and the handles will still get bound.

this solve the problem where binding a context results in another context binding, which the current code can miss, if the content that gets bound is before the context that binds it in the contexts list

it would be good to get this in and start back porting to rc3 and rc4. we may find a better way to do this, but i think we can forward fix in main if we need to

@jzaffiro Can you explain the bug to me that you are trying to fix? The description says this is adding tests but it has code changes as well so I am not clear what is being solved here :-).

Is the problem only what Tony pointed out - "if the content that gets bound is before the context that binds it in the contexts list"? Basically, due to ordering we can miss some contexts?

The tests are to make sure we don't hit a bug where handles that are stored in detached DDSs are not bound when we attach. The tests starting on line 501 of handleValidation.spec.ts were not able to find the DDS referenced to in the other detached DDS, which is what this code here solves. I'll update the description as well.

Before #20998, we were also seeing a bug where going from detached --> attached would cause duplicate attach ops to be sent, and the tests starting on line 405 make sure we have coverage for all DDSs for that bug

msfluid-bot · 2024-05-21T19:56:32Z

⯅ @fluid-example/bundle-size-tests: +222 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	453.96 KB	454.04 KB	⯅ +74 Bytes
azureClient.js	551.21 KB	551.29 KB	⯅ +74 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	257.82 KB	257.82 KB	■ No change
fluidFramework.js	357 KB	357 KB	■ No change
loader.js	132.91 KB	132.91 KB	■ No change
map.js	41.43 KB	41.43 KB	■ No change
matrix.js	143.66 KB	143.66 KB	■ No change
odspClient.js	519.75 KB	519.83 KB	⯅ +74 Bytes
odspDriver.js	97.3 KB	97.3 KB	■ No change
odspPrefetchSnapshot.js	42.16 KB	42.16 KB	■ No change
sharedString.js	160.18 KB	160.18 KB	■ No change
sharedTree.js	356.98 KB	356.98 KB	■ No change
Total Size	3.19 MB	3.19 MB	⯅ +222 Bytes

Baseline commit: 1a92d06

Generated by 🚫 dangerJS against b06a239

Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list. [AB#8009](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8009) --------- Co-authored-by: Tony Murphy <[email protected]>

…1188) Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list. Port of #21132 [AB#8009](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8009) Co-authored-by: Tony Murphy <[email protected]>

…1187) Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list. Port of #21132 [AB#8009](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8009) --------- Co-authored-by: Tony Murphy <[email protected]>

With the change to the ordering of getting the attachment summary in #20998 and the testing in #20995 and #21132, we can now remove the workarounds that call bindHandles or makeSerializable when not in an attached state.

Add tests for storing handles detached, then storing in an attached d…

22aaeb9

…ds to attach

github-actions bot added area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch labels May 16, 2024

jzaffiro changed the title ~~Add tests for storing handles detached, then storing in an attached d…~~ Store handles in detached DDS before attaching May 16, 2024

anthony-murphy reviewed May 16, 2024

View reviewed changes

packages/test/test-end-to-end-tests/src/test/handleValidation.spec.ts Outdated Show resolved Hide resolved

anthony-murphy reviewed May 16, 2024

View reviewed changes

packages/test/test-end-to-end-tests/src/test/handleValidation.spec.ts Outdated Show resolved Hide resolved

jzaffiro added 2 commits May 17, 2024 00:35

abstraction for map

9cdec05

WIP: test clean up

babed31

anthony-murphy reviewed May 20, 2024

View reviewed changes

jzaffiro added 3 commits May 20, 2024 10:49

Merge branch 'main' into moreAttachTests

7330bdb

fix import

66a6c13

Add set of tests that use a new data store and keep those that don't …

0455b0d

…use a new data store

jzaffiro commented May 20, 2024

View reviewed changes

Tony Murphy and others added 3 commits May 21, 2024 10:05

Ensure all handles are bound on dds attach

71a4dcd

Merge remote-tracking branch 'anthony-murphy/fix-handle-binding' into…

010315f

… moreAttachTests

Remove consensus queue tests and make storeHandle async

b06a239

github-actions bot added the area: runtime Runtime related issues label May 21, 2024

jzaffiro marked this pull request as ready for review May 21, 2024 19:09

jzaffiro requested review from markfields and agarwal-navin May 21, 2024 19:09

jzaffiro commented May 21, 2024

View reviewed changes

anthony-murphy approved these changes May 21, 2024

View reviewed changes

jzaffiro merged commit 6ff95cd into microsoft:main May 21, 2024
32 checks passed

jzaffiro deleted the moreAttachTests branch May 21, 2024 21:01

jzaffiro mentioned this pull request May 21, 2024

Port RC3: Store handles in detached DDS before attaching (#21132) #21187

Merged

jzaffiro mentioned this pull request May 21, 2024

Port RC4: Store handles in detached DDS before attaching (#21132) #21188

Merged

jzaffiro mentioned this pull request May 23, 2024

Remove extra calls to bind when not attached #21213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store handles in detached DDS before attaching #21132

Store handles in detached DDS before attaching #21132

jzaffiro commented May 16, 2024 •

edited

anthony-murphy May 20, 2024

jzaffiro May 20, 2024 •

edited

anthony-murphy May 20, 2024

jzaffiro May 20, 2024

jzaffiro May 21, 2024

anthony-murphy May 21, 2024

anthony-murphy May 21, 2024

agarwal-navin May 21, 2024

jzaffiro May 21, 2024

jzaffiro May 21, 2024

msfluid-bot commented May 21, 2024

Store handles in detached DDS before attaching #21132

Store handles in detached DDS before attaching #21132

Conversation

jzaffiro commented May 16, 2024 • edited

Choose a reason for hiding this comment

jzaffiro May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfluid-bot commented May 21, 2024

jzaffiro commented May 16, 2024 •

edited

jzaffiro May 20, 2024 •

edited