Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 30 commits into from
Feb 21, 2023

Conversation

andre4i
Copy link
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
@@ -184,6 +184,9 @@ export interface IContainerContext extends IDisposable {
getLoadedFromVersion(): IVersion | undefined;

updateDirtyContainerState(dirty: boolean): void;

readonly supportedFeatures?: ReadonlyMap<string, unknown>;
Copy link
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)

@msfluid-bot
Copy link
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

@andre4i andre4i merged commit 3abbc67 into microsoft:main Feb 21, 2023
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