Skip to content

Commit

Permalink
Revert "feat(Container): Add IContainer.closedWithError to access the…
Browse files Browse the repository at this point in the history
… error that closed or disposed the container" (#23399)

#### Description
 
Reverts #23364


This change appears to cause errors in our end to end stress tests.
  • Loading branch information
jikim-msft authored Dec 23, 2024
1 parent 58619c3 commit 6780fdb
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 95 deletions.
18 changes: 0 additions & 18 deletions .changeset/busy-mangos-ask.md

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ export interface IContainer extends IEventProvider<IContainerEvents> {
readonly clientId?: string | undefined;
close(error?: ICriticalContainerError): void;
readonly closed: boolean;
readonly closedWithError?: ICriticalContainerError | undefined;
connect(): void;
readonly connectionState: ConnectionState;
containerMetadata: Record<string, string>;
Expand Down
8 changes: 0 additions & 8 deletions packages/common/container-definitions/src/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -374,14 +374,6 @@ export interface IContainer extends IEventProvider<IContainerEvents> {
*/
readonly disposed?: boolean;

/**
* The error that caused the container to close or dispose.
*
* @remarks If the container is closed first and then later disposed,
* this will be the error from the close (even if undefined), not the dispose.
*/
readonly closedWithError?: ICriticalContainerError | undefined;

/**
* Whether or not there are any local changes that have not been saved.
*/
Expand Down
19 changes: 2 additions & 17 deletions packages/loader/container-loader/src/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -579,11 +579,6 @@ export class Container
return this._lifecycleState === "disposing" || this._lifecycleState === "disposed";
}

public get closedWithError(): ICriticalContainerError | undefined {
return this._closedWithError;
}
private _closedWithError?: ICriticalContainerError;

private readonly storageAdapter: ContainerStorageAdapter;

private readonly _deltaManager: DeltaManager<ConnectionManager>;
Expand Down Expand Up @@ -1113,7 +1108,6 @@ export class Container
);

this._lifecycleState = "closing";
this._closedWithError = error;

// Back-compat for Old driver
if (this.service?.off !== undefined) {
Expand Down Expand Up @@ -1155,24 +1149,15 @@ export class Container
this.mc.logger.sendTelemetryEvent(
{
eventName: "ContainerDispose",
// Use error category if there's an error, unless we already closed with an error (i.e. _closedWithError is set)
category:
error !== undefined && this._closedWithError === undefined ? "error" : "generic",
details: {
closedWithErrorType: this._closedWithError?.errorType,
closedWithErrorMessage: this._closedWithError?.message,
},
// Only log error if container isn't closed
category: !this.closed && error !== undefined ? "error" : "generic",
},
error,
);

// ! Progressing from "closed" to "disposing" is not allowed
if (this._lifecycleState !== "closed") {
this._lifecycleState = "disposing";

// Only set _closedWithError if we're "disposing and closing" all at once
// (disposing from a non-closed state)
this._closedWithError = error;
}

this._protocolHandler?.close();
Expand Down
54 changes: 3 additions & 51 deletions packages/test/test-end-to-end-tests/src/test/container.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ describeCompat("Container", "NoCompat", (getTestObjectProvider) => {
() => runtimeDispose++,
);

container.dispose(new DataCorruptionError("dispose error", {}));
container.dispose(new DataCorruptionError("expected", {}));
assert.strictEqual(
containerDisposed,
1,
Expand All @@ -717,11 +717,6 @@ describeCompat("Container", "NoCompat", (getTestObjectProvider) => {
1,
"IContainerRuntime should send dispose event on container dispose",
);
assert.strictEqual(
container.closedWithError?.message,
"dispose error",
"Expected the error that disposed the container",
);
},
);

Expand Down Expand Up @@ -749,56 +744,13 @@ describeCompat("Container", "NoCompat", (getTestObjectProvider) => {
() => runtimeDispose++,
);

container.close(new DataCorruptionError("close error", {}));
container.dispose(new DataCorruptionError("dispose error", {}));
container.close(new DataCorruptionError("expected", {}));
container.dispose(new DataCorruptionError("expected", {}));
assert.strictEqual(containerDisposed, 1, "Container should send disposed event");
assert.strictEqual(containerClosed, 1, "Container should send closed event");
assert.strictEqual(deltaManagerDisposed, 1, "DeltaManager should send disposed event");
assert.strictEqual(deltaManagerClosed, 1, "DeltaManager should send closed event");
assert.strictEqual(runtimeDispose, 1, "IContainerRuntime should send dispose event");
assert.strictEqual(
container.closedWithError?.message,
"close error",
"Expected the error that closed the container (not the dispose one)",
);
},
);

itExpects(
"Closing (no error) then disposing (with error) container leaves closedWithError unset",
[
{ eventName: "fluid:telemetry:Container:ContainerClose", category: "generic" },
{ eventName: "fluid:telemetry:Container:ContainerDispose", category: "error" },
],
async () => {
const container = await createConnectedContainer();
container.close();
container.dispose(new DataCorruptionError("dispose error", {}));

assert.strictEqual(
container.closedWithError,
undefined,
"Expected undefined since the container was closed without error",
);
},
);

itExpects(
"Closing (with error) then disposing (no error) container should set closedWithError properly",
[
{ eventName: "fluid:telemetry:Container:ContainerClose", category: "error" },
{ eventName: "fluid:telemetry:Container:ContainerDispose", category: "generic" },
],
async () => {
const container = await createConnectedContainer();
container.close(new DataCorruptionError("close error", {}));
container.dispose();

assert.strictEqual(
container.closedWithError?.message,
"close error",
"Expected the error that closed the container",
);
},
);

Expand Down

0 comments on commit 6780fdb

Please sign in to comment.