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

Revert "feat(Container): Add IContainer.closedWithError to access the error that closed or disposed the container" #23398

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading