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

Jump to bottom

Add concurrent op check #11309

Merged
merged 19 commits into from Aug 31, 2022
Merged

Add concurrent op check #11309

merged 19 commits into from Aug 31, 2022

Conversation

WayneFerrao
Copy link
Contributor

@WayneFerrao WayneFerrao commented Jul 27, 2022 •
edited

Description

AB#1092
This PR adds assertions to DeltaManager and ContainerRuntime to ensure that ops are not being sent while currently being processsed. Currently this is happening unchecked, leading to non-deterministic outcomes.

Reviewer Guidance

  • I implemented this assertion as a "hidden" check that will be shipped dark. When toggled true, preventConcurrentOpSend will close the container if attempting to send an op while processing another. It is being added and tested(itExpects.skip()) in this manner because this change currently breaks many other tests.

@WayneFerrao WayneFerrao marked this pull request as ready for review July 27, 2022 18:35
@WayneFerrao WayneFerrao requested review from a team as code owners July 27, 2022 18:35
@WayneFerrao WayneFerrao marked this pull request as draft July 27, 2022 18:36
@github-actions github-actions bot added area: loader Loader related issues area: runtime Runtime related issues base: main PRs targeted against main branch labels Jul 27, 2022
vladsud
vladsud reviewed Jul 27, 2022
View reviewed changes
packages/loader/container-loader/src/deltaManager.ts Outdated
@@ -186,6 +190,9 @@ export class DeltaManager<TConnectionManager extends IConnectionManager>
public get clientDetails() { return this.connectionManager.clientDetails; }

public submit(type: MessageType, contents: any, batch = false, metadata?: any) {
if (this.opsCurrentlyProcessing > 0) {
this.close(new UsageError("Currently processing ops: Container closed"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add better message here. I'd say "Making changes to data model is disallowed while processing ops."

vladsud
vladsud reviewed Jul 27, 2022
View reviewed changes
packages/loader/container-loader/src/deltaManager.ts Outdated
@@ -762,6 +768,7 @@ export class DeltaManager<TConnectionManager extends IConnectionManager>

private processInboundMessage(message: ISequencedDocumentMessage): void {
const startTime = Date.now();
this.opsCurrentlyProcessing++;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather make it a boolean (as there is no reentrancy here). You could also add asserts that it is false (zero in current form) before we change it here, and true below we reset it back

vladsud
vladsud reviewed Jul 27, 2022
View reviewed changes
packages/runtime/container-runtime/src/containerRuntime.ts Outdated
@@ -1890,6 +1890,10 @@ export class ContainerRuntime extends TypedEventEmitter<IContainerRuntimeEvents>

public process(messageArg: ISequencedDocumentMessage, local: boolean) {
this.verifyNotClosed();
if (this.opsCurrentlyProcessing !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not remember why we decided to go with checking it in two places, but worth adding some comment on why we are doing

vladsud
vladsud reviewed Jul 27, 2022
View reviewed changes
packages/loader/container-loader/src/test/deltaManager.spec.ts Outdated
@@ -332,6 +332,12 @@ describe("Loader", () => {
deltaManager.connectionManager.forceReadonly(true);
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

You could test it at that level (at least the check in DeltaManager, so yes, adding some code to do so would be nice).

@vladsud
Copy link
Contributor

vladsud commented Aug 3, 2022

Please add breaking.md note about these changes, how users can temporarily disable it, and the fact that they will lose such capability in the future, thus need to fix any bugs it finds.

vladsud
vladsud reviewed Aug 3, 2022
View reviewed changes
packages/runtime/container-runtime/src/test/containerRuntime.spec.ts Outdated
pushOp(message);
pushOp(message);
pushOp(message);
await processOps();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this test actually testing. It does not result in ops being generated while processing an op.
I.e. it will either pass of fail in the same way, with or without your other changes.
Plus, if I got it right, this code uses MockDeltaManager, not real (production) code you are modifying, and there is no ContainerRuntime created in this test (as far as I can see).

I think the easiest way to validate new behavior is to use some existing DDS (in end-to-end tests) and change it from within event handler.

@github-actions github-actions bot added the area: tests Tests to add, test infrastructure improvements, etc label Aug 10, 2022
@github-actions github-actions bot removed the area: runtime Runtime related issues label Aug 10, 2022
@github-actions github-actions bot added the breaking change This PR or issue would introduce a breaking change label Aug 10, 2022
@WayneFerrao WayneFerrao marked this pull request as ready for review August 10, 2022 18:24
@WayneFerrao WayneFerrao requested a review from a team as a code owner August 10, 2022 18:24
@WayneFerrao WayneFerrao requested a review from a team as a code owner August 31, 2022 15:49
@github-actions github-actions bot added area: build Build related issues area: dds Issues related to distributed data structures area: dds: propertydds area: driver Driver related issues area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: runtime Runtime related issues area: server Server related issues (routerlicious) breaking change This PR or issue would introduce a breaking change dependencies Pull requests that update a dependency file public api change Changes to a public API labels Aug 31, 2022
@github-actions github-actions bot removed area: server Server related issues (routerlicious) area: dds: propertydds dependencies Pull requests that update a dependency file area: driver Driver related issues area: runtime Runtime related issues public api change Changes to a public API area: dds Issues related to distributed data structures area: build Build related issues area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct labels Aug 31, 2022
@WayneFerrao WayneFerrao force-pushed the add-concurrent-op-check branch 3 times, most recently from 9aaa435 to 2e4dccc Compare August 31, 2022 18:40
@WayneFerrao WayneFerrao merged commit 28620b2 into microsoft:main Aug 31, 2022
@github-actions
Copy link
Contributor

This commit is queued for merging with the next branch! Please ignore this PR for now. Contact @microsoft/fluid-cr-infra for help.

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

@andre4i andre4i andre4i left review comments

@markfields markfields markfields approved these changes

@vladsud vladsud vladsud approved these changes

@jatgarg jatgarg Awaiting requested review from jatgarg

@msfluid-bot msfluid-bot Awaiting requested review from msfluid-bot

Assignees
No one assigned
Labels
area: loader Loader related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch breaking change This PR or issue would introduce a breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@WayneFerrao @vladsud @msfluid-bot @markfields @andre4i