Skip to content

Commit c9d1562

Browse files
vladsudvladsud
authored
Remove ability to reject reentrant ops (#20621)
Remove failed experiment from repo. It has been proven that pursuing this direction is not feasible / too expensive and does not provide good programming model. While it does not close venue for some misuse, we have covered some aspects of it (properly tracking referenceSequence number / rebasing for ops sent via re-entrancy). See https://dev.azure.com/fluidframework/internal/_workitems/edit/2322#12286846 for some details. Key changes: 1. Remove IContainerRuntimeOptions.enableOpReentryCheck and code associated with this option 2. Remove ensureNoDataModelChanges() from API surfaces where it's safe. Some interfaces can be cleaned only in the future (due to N/N-1 compat promises) 3. Instead of relying on DDS to use ensureNoDataModelChanges to catch re-entrnacy, runtime will call it from process(), thus casting wider net and also not requiring each DDS to do so. - the side-effect of it - it will not catch re-entrant local changes, but such changes should not be rebased, so that's Ok. Please note that opReentrancy.spec.ts has plenty of test coverage here, including UTs like "Eventual consistency with op reentry..." that uses SharedString to validate proper rebasing happens when dealing with reentrancy.
1 parent 55b0f70 commit c9d1562

20 files changed

Lines changed: 60 additions & 367 deletions

File tree

‎packages/dds/shared-object-base/src/sharedObject.ts‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -585,19 +585,7 @@ export abstract class SharedObjectCore<TEvent extends ISharedObjectEvents = ISha
585585
*/
586586
public emit(event: EventEmitterEventType, ...args: any[]): boolean {
587587
return this.callbacksHelper.measure(() => {
588-
// Creating ops while handling a DDS event can lead
589-
// to undefined behavior and events observed in the wrong order.
590-
// For example, we have two callbacks registered for a DDS, A and B.
591-
// Then if on change #1 callback A creates change #2, the invocation flow will be:
592-
//
593-
// A because of #1
594-
// A because of #2
595-
// B because of #2
596-
// B because of #1
597-
//
598-
// The runtime must enforce op coherence by not allowing any ops to be created during
599-
// the event handler
600-
return this.runtime.ensureNoDataModelChanges(() => super.emit(event, ...args));
588+
return super.emit(event, ...args);
601589
});
602590
}
603591

‎packages/runtime/container-runtime/api-report/container-runtime.api.md‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -723,7 +723,6 @@ export interface IContainerRuntimeOptions {
723723
readonly chunkSizeInBytes?: number;
724724
readonly compressionOptions?: ICompressionRuntimeOptions;
725725
readonly enableGroupedBatching?: boolean;
726-
readonly enableOpReentryCheck?: boolean;
727726
readonly enableRuntimeIdCompressor?: IdCompressorMode;
728727
readonly explicitSchemaControl?: boolean;
729728
readonly flushMode?: FlushMode;

‎packages/runtime/container-runtime/src/channelCollection.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,7 @@ export function wrapContext(context: IFluidParentContext): IFluidParentContext {
168168
getAudience: (...args) => {
169169
return context.getAudience(...args);
170170
},
171+
// back-compat, to be removed in 2.0
171172
ensureNoDataModelChanges: (...args) => {
172173
return context.ensureNoDataModelChanges(...args);
173174
},

‎packages/runtime/container-runtime/src/containerRuntime.ts‎

Lines changed: 19 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,6 @@ import {
159159
OpSplitter,
160160
Outbox,
161161
RemoteMessageProcessor,
162-
getLongStack,
163162
} from "./opLifecycle/index.js";
164163
import { pkgVersion } from "./packageVersion.js";
165164
import {
@@ -459,16 +458,6 @@ export interface IContainerRuntimeOptions {
459458
*/
460459
readonly enableRuntimeIdCompressor?: IdCompressorMode;
461460

462-
/**
463-
* If enabled, the runtime will block all attempts to send an op inside the
464-
* {@link ContainerRuntime#ensureNoDataModelChanges} callback. The callback is used by
465-
* {@link @fluidframework/shared-object-base#SharedObjectCore} for event handlers so enabling this
466-
* will disallow modifying DDSes while handling DDS events.
467-
*
468-
* By default, the feature is disabled. If enabled from options, the `Fluid.ContainerRuntime.DisableOpReentryCheck`
469-
* can be used to disable it at runtime.
470-
*/
471-
readonly enableOpReentryCheck?: boolean;
472461
/**
473462
* If enabled, the runtime will group messages within a batch into a single
474463
* message to be sent to the service.
@@ -808,7 +797,6 @@ export class ContainerRuntime
808797
maxBatchSizeInBytes = defaultMaxBatchSizeInBytes,
809798
enableRuntimeIdCompressor,
810799
chunkSizeInBytes = defaultChunkSizeInBytes,
811-
enableOpReentryCheck = false,
812800
enableGroupedBatching = false,
813801
explicitSchemaControl = false,
814802
} = runtimeOptions;
@@ -1021,7 +1009,6 @@ export class ContainerRuntime
10211009
chunkSizeInBytes,
10221010
// Requires<> drops undefined from IdCompressorType
10231011
enableRuntimeIdCompressor: enableRuntimeIdCompressor as "on" | "delayed",
1024-
enableOpReentryCheck,
10251012
enableGroupedBatching,
10261013
explicitSchemaControl,
10271014
},
@@ -1224,14 +1211,6 @@ export class ContainerRuntime
12241211

12251212
private ensureNoDataModelChangesCalls = 0;
12261213

1227-
/**
1228-
* Tracks the number of detected reentrant ops to report,
1229-
* in order to self-throttle the telemetry events.
1230-
*
1231-
* This should be removed as part of ADO:2322
1232-
*/
1233-
private opReentryCallsToReport = 5;
1234-
12351214
/**
12361215
* Invokes the given callback and expects that no ops are submitted
12371216
* until execution finishes. If an op is submitted, an error will be raised.
@@ -1265,7 +1244,6 @@ export class ContainerRuntime
12651244

12661245
private dirtyContainer: boolean;
12671246
private emitDirtyDocumentEvent = true;
1268-
private readonly enableOpReentryCheck: boolean;
12691247
private readonly disableAttachReorder: boolean | undefined;
12701248
private readonly closeSummarizerDelayMs: number;
12711249
/**
@@ -1549,14 +1527,6 @@ export class ContainerRuntime
15491527
this.validateSummaryHeuristicConfiguration(this.summaryConfiguration);
15501528
}
15511529

1552-
const disableOpReentryCheck = this.mc.config.getBoolean(
1553-
"Fluid.ContainerRuntime.DisableOpReentryCheck",
1554-
);
1555-
this.enableOpReentryCheck =
1556-
runtimeOptions.enableOpReentryCheck === true &&
1557-
// Allow for a break-glass config to override the options
1558-
disableOpReentryCheck !== true;
1559-
15601530
this.summariesDisabled = this.isSummariesDisabled();
15611531
this.maxOpsSinceLastSummary = this.getMaxOpsSinceLastSummary();
15621532
this.initialSummarizerDelayMs = this.getInitialSummarizerDelayMs();
@@ -1868,7 +1838,6 @@ export class ContainerRuntime
18681838
idCompressorMode: this.idCompressorMode,
18691839
featureGates: JSON.stringify({
18701840
...featureGatesForTelemetry,
1871-
disableOpReentryCheck,
18721841
disableChunking,
18731842
disableAttachReorder: this.disableAttachReorder,
18741843
disablePartialFlush,
@@ -2550,20 +2519,25 @@ export class ContainerRuntime
25502519
// but will not modify the contents object (likely it will replace it on the message).
25512520
const messageCopy = { ...messageArg };
25522521
for (const message of this.remoteMessageProcessor.process(messageCopy)) {
2553-
if (modernRuntimeMessage) {
2554-
this.processCore({
2555-
// Cast it since we expect it to be this based on modernRuntimeMessage computation above.
2556-
// There is nothing really ensuring that anytime original message.type is Operation that
2557-
// the result messages will be so. In the end modern bool being true only directs to
2558-
// throw error if ultimately unrecognized without compat details saying otherwise.
2559-
message: message as InboundSequencedContainerRuntimeMessage,
2560-
local,
2561-
modernRuntimeMessage,
2562-
});
2563-
} else {
2564-
// Unrecognized message will be ignored.
2565-
this.processCore({ message, local, modernRuntimeMessage });
2566-
}
2522+
const msg: MessageWithContext = modernRuntimeMessage
2523+
? {
2524+
// Cast it since we expect it to be this based on modernRuntimeMessage computation above.
2525+
// There is nothing really ensuring that anytime original message.type is Operation that
2526+
// the result messages will be so. In the end modern bool being true only directs to
2527+
// throw error if ultimately unrecognized without compat details saying otherwise.
2528+
message: message as InboundSequencedContainerRuntimeMessage,
2529+
local,
2530+
modernRuntimeMessage,
2531+
}
2532+
: // Unrecognized message will be ignored.
2533+
{
2534+
message,
2535+
local,
2536+
modernRuntimeMessage,
2537+
};
2538+
2539+
// ensure that we observe any re-entrancy, and if needed, rebase ops
2540+
this.ensureNoDataModelChanges(() => this.processCore(msg));
25672541
}
25682542
}
25692543

@@ -3884,7 +3858,6 @@ export class ContainerRuntime
38843858
metadata?: { localId: string; blobId?: string },
38853859
): void {
38863860
this.verifyNotClosed();
3887-
this.verifyCanSubmitOps();
38883861

38893862
// There should be no ops in detached container state!
38903863
assert(
@@ -4039,37 +4012,6 @@ export class ContainerRuntime
40394012
}
40404013
}
40414014

4042-
private verifyCanSubmitOps() {
4043-
if (this.ensureNoDataModelChangesCalls > 0) {
4044-
const errorMessage =
4045-
"Op was submitted from within a `ensureNoDataModelChanges` callback";
4046-
if (this.opReentryCallsToReport > 0) {
4047-
this.mc.logger.sendTelemetryEvent(
4048-
{ eventName: "OpReentry" },
4049-
// We need to capture the call stack in order to inspect the source of this usage pattern
4050-
getLongStack(() => new UsageError(errorMessage)),
4051-
);
4052-
this.opReentryCallsToReport--;
4053-
}
4054-
4055-
// Creating ops while processing ops can lead
4056-
// to undefined behavior and events observed in the wrong order.
4057-
// For example, we have two callbacks registered for a DDS, A and B.
4058-
// Then if on change #1 callback A creates change #2, the invocation flow will be:
4059-
//
4060-
// A because of #1
4061-
// A because of #2
4062-
// B because of #2
4063-
// B because of #1
4064-
//
4065-
// The runtime must enforce op coherence by not allowing ops to be submitted
4066-
// while ops are being processed.
4067-
if (this.enableOpReentryCheck) {
4068-
throw new UsageError(errorMessage);
4069-
}
4070-
}
4071-
}
4072-
40734015
private reSubmitBatch(batch: IPendingBatchMessage[]) {
40744016
this.orderSequentially(() => {
40754017
for (const message of batch) {

‎packages/runtime/container-runtime/src/dataStoreContext.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,7 @@ export abstract class FluidDataStoreContext
219219
return this._containerRuntime;
220220
}
221221

222+
// back-compat, to be removed in 2.0
222223
public ensureNoDataModelChanges<T>(callback: () => T): T {
223224
return this.parentContext.ensureNoDataModelChanges(callback);
224225
}

‎packages/runtime/container-runtime/src/test/containerRuntime.spec.ts‎

Lines changed: 0 additions & 148 deletions
Original file line numberDiff line numberDiff line change
@@ -542,151 +542,6 @@ describe("Runtime", () => {
542542
});
543543
}));
544544

545-
describe("Op reentry enforcement", () => {
546-
let containerRuntime: ContainerRuntime;
547-
548-
it("By default, don't enforce the op reentry check", async () => {
549-
containerRuntime = await ContainerRuntime.loadRuntime({
550-
context: getMockContext() as IContainerContext,
551-
registryEntries: [],
552-
provideEntryPoint: mockProvideEntryPoint,
553-
existing: false,
554-
});
555-
556-
assert.ok(
557-
containerRuntime.ensureNoDataModelChanges(() => {
558-
submitDataStoreOp(containerRuntime, "id", "test");
559-
return true;
560-
}),
561-
);
562-
563-
assert.ok(
564-
containerRuntime.ensureNoDataModelChanges(() =>
565-
containerRuntime.ensureNoDataModelChanges(() =>
566-
containerRuntime.ensureNoDataModelChanges(() => {
567-
submitDataStoreOp(containerRuntime, "id", "test");
568-
return true;
569-
}),
570-
),
571-
),
572-
);
573-
});
574-
575-
it("If option enabled, enforce the op reentry check", async () => {
576-
containerRuntime = await ContainerRuntime.loadRuntime({
577-
context: getMockContext() as IContainerContext,
578-
registryEntries: [],
579-
runtimeOptions: {
580-
enableOpReentryCheck: true,
581-
},
582-
provideEntryPoint: mockProvideEntryPoint,
583-
existing: false,
584-
});
585-
586-
assert.throws(() =>
587-
containerRuntime.ensureNoDataModelChanges(() =>
588-
submitDataStoreOp(containerRuntime, "id", "test"),
589-
),
590-
);
591-
592-
assert.throws(() =>
593-
containerRuntime.ensureNoDataModelChanges(() =>
594-
containerRuntime.ensureNoDataModelChanges(() =>
595-
containerRuntime.ensureNoDataModelChanges(() =>
596-
submitDataStoreOp(containerRuntime, "id", "test"),
597-
),
598-
),
599-
),
600-
);
601-
});
602-
603-
it("If option enabled but disabled via feature gate, don't enforce the op reentry check", async () => {
604-
containerRuntime = await ContainerRuntime.loadRuntime({
605-
context: getMockContext({
606-
"Fluid.ContainerRuntime.DisableOpReentryCheck": true,
607-
}) as IContainerContext,
608-
registryEntries: [],
609-
runtimeOptions: {
610-
enableOpReentryCheck: true,
611-
},
612-
provideEntryPoint: mockProvideEntryPoint,
613-
existing: false,
614-
});
615-
616-
containerRuntime.ensureNoDataModelChanges(() =>
617-
submitDataStoreOp(containerRuntime, "id", "test"),
618-
);
619-
620-
containerRuntime.ensureNoDataModelChanges(() =>
621-
containerRuntime.ensureNoDataModelChanges(() =>
622-
containerRuntime.ensureNoDataModelChanges(() =>
623-
submitDataStoreOp(containerRuntime, "id", "test"),
624-
),
625-
),
626-
);
627-
});
628-
629-
it("Report at most 5 reentrant ops", async () => {
630-
const mockLogger = new MockLogger();
631-
containerRuntime = await ContainerRuntime.loadRuntime({
632-
context: getMockContext({}, mockLogger) as IContainerContext,
633-
registryEntries: [],
634-
provideEntryPoint: mockProvideEntryPoint,
635-
existing: false,
636-
});
637-
638-
mockLogger.clear();
639-
containerRuntime.ensureNoDataModelChanges(() => {
640-
for (let i = 0; i < 10; i++) {
641-
submitDataStoreOp(containerRuntime, "id", "test");
642-
}
643-
});
644-
645-
// We expect only 5 events
646-
mockLogger.assertMatchStrict(
647-
Array.from(Array(5).keys()).map(() => ({
648-
eventName: "ContainerRuntime:OpReentry",
649-
error: "Op was submitted from within a `ensureNoDataModelChanges` callback",
650-
})),
651-
);
652-
});
653-
654-
it("Can't call flush() inside ensureNoDataModelChanges's callback", async () => {
655-
containerRuntime = await ContainerRuntime.loadRuntime({
656-
context: getMockContext() as IContainerContext,
657-
registryEntries: [],
658-
runtimeOptions: {
659-
flushMode: FlushMode.Immediate,
660-
},
661-
provideEntryPoint: mockProvideEntryPoint,
662-
existing: false,
663-
});
664-
665-
assert.throws(() =>
666-
containerRuntime.ensureNoDataModelChanges(() => {
667-
containerRuntime.orderSequentially(() => {});
668-
}),
669-
);
670-
});
671-
672-
it("Can't create an infinite ensureNoDataModelChanges recursive call ", async () => {
673-
containerRuntime = await ContainerRuntime.loadRuntime({
674-
context: getMockContext() as IContainerContext,
675-
registryEntries: [],
676-
provideEntryPoint: mockProvideEntryPoint,
677-
existing: false,
678-
});
679-
680-
const callback = () => {
681-
containerRuntime.ensureNoDataModelChanges(() => {
682-
submitDataStoreOp(containerRuntime, "id", "test");
683-
callback();
684-
});
685-
};
686-
assert.throws(() => callback());
687-
});
688-
});
689-
690545
describe("orderSequentially with rollback", () =>
691546
[
692547
FlushMode.TurnBased,
@@ -1569,7 +1424,6 @@ describe("Runtime", () => {
15691424
maxBatchSizeInBytes: 700 * 1024,
15701425
chunkSizeInBytes: 204800,
15711426
enableRuntimeIdCompressor: undefined,
1572-
enableOpReentryCheck: false,
15731427
enableGroupedBatching: false,
15741428
explicitSchemaControl: false,
15751429
} satisfies IContainerRuntimeOptions;
@@ -1598,7 +1452,6 @@ describe("Runtime", () => {
15981452
const featureGates = {
15991453
"Fluid.ContainerRuntime.CompressionDisabled": true,
16001454
"Fluid.ContainerRuntime.CompressionChunkingDisabled": true,
1601-
"Fluid.ContainerRuntime.DisableOpReentryCheck": false,
16021455
"Fluid.ContainerRuntime.IdCompressorEnabled": true,
16031456
"Fluid.ContainerRuntime.DisableGroupedBatching": true,
16041457
};
@@ -1619,7 +1472,6 @@ describe("Runtime", () => {
16191472
featureGates: JSON.stringify({
16201473
disableGroupedBatching: true,
16211474
disableCompression: true,
1622-
disableOpReentryCheck: false,
16231475
disableChunking: true,
16241476
}),
16251477
groupedBatchingEnabled: false,

0 commit comments

Comments
 (0)