Skip to content

Don't allow FlushModeExperimental.Async if the loader does not support reference sequence numbers#14239

Merged
andre4i merged 30 commits intomicrosoft:mainfrom
andre4i:disallow-async-with-old-loader
Feb 21, 2023
Merged

Don't allow FlushModeExperimental.Async if the loader does not support reference sequence numbers#14239
andre4i merged 30 commits intomicrosoft:mainfrom
andre4i:disallow-async-with-old-loader

Conversation

@andre4i
Copy link
Copy Markdown
Contributor

@andre4i andre4i commented Feb 17, 2023

Description

With FlushMode==FlushModeExperimental.Async, batches will be flushed across JS turns, drastically increasing the chances of ending up in states with mismatched reference sequence numbers in the same batch. This change #14150 alleviates the risk, by flushing batches earlier. However, as old loader do not receive the reference sequence numbers from the runtime, there is considerable risk of mixed batches in that case (new runtime + FlushModeExperimental.Async - old loader). Therefore, we need to explicitly block this combination.

Loaders from versions older than the present will not work with FlushModeExperimental.Async as they don't specify it's supported features in the ContainerContext implementation at all.

@andre4i andre4i requested review from a team as code owners February 17, 2023 22:46
@andre4i andre4i requested review from a team as code owners February 17, 2023 22:46
@github-actions github-actions Bot added area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API base: main PRs targeted against main branch labels Feb 17, 2023

updateDirtyContainerState(dirty: boolean): void;

readonly supportedFeatures?: ReadonlyMap<string, unknown>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Mostly talking aloud)
I believe it's right approach, but I do not know how to think about documentation here.
I.e. we should fully support parallel implementations of IRuntime, or vice versa - IContainerContext, including by 3rd parties. It's not very clear what needs to be done here RE properly documenting all the properties we are exposing here, as their timeframe (i.e. I totally expect exposure of features to be "sunset" once we have 100% saturation of new bits, i.e. the need to check for feature presents to go away)

Comment thread packages/loader/container-loader/src/containerContext.ts Outdated
Comment thread packages/runtime/container-runtime/src/containerRuntime.ts Outdated
@msfluid-bot
Copy link
Copy Markdown
Collaborator

msfluid-bot commented Feb 17, 2023

@fluid-example/bundle-size-tests: +498 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 432.62 KB 432.83 KB +215 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 227.53 KB 227.74 KB +217 Bytes
loader.js 158.61 KB 158.67 KB +66 Bytes
map.js 43.88 KB 43.88 KB No change
matrix.js 136.06 KB 136.06 KB No change
odspDriver.js 91.79 KB 91.79 KB No change
odspPrefetchSnapshot.js 43.53 KB 43.53 KB No change
sharedString.js 156.6 KB 156.6 KB No change
Total Size 1.37 MB 1.37 MB +498 Bytes

Baseline commit: 3132fbd

Generated by 🚫 dangerJS against 4b5cb94

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch public api change Changes to a public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants