Skip to content

Commit

Permalink
Revert: Reset joinSession cache only on connect_document_error (#20784)
Browse files Browse the repository at this point in the history 
Reverts #19974 PR which
was applied as a performance improvement. In the original PR, we checked
for the kind of error received on the socket to decide whether to clear
joinSession cache or not and reuse the existing session whenever
possible. However a bug was identified where one section of the code was
not update to stamp the errorFrom property which is used to perform such
check which resulted in the cache never clearing for and the code
getting stuck in a loop to continue connection retries.

Impact of revert: There is no compatibility-issue/regression that this
revert would introduce, as it was only a perf improvement.

Follow up: Captured in
[AB#7833](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/7833)
  • Loading branch information
@pragya91
pragya91 committed Apr 22, 2024
1 parent 6967b02 commit 3342aff
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 183 deletions.
2 changes: 0 additions & 2 deletions packages/drivers/driver-base/src/documentDeltaConnection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -766,8 +766,6 @@ export class DocumentDeltaConnection
details: JSON.stringify({
...this.getConnectionDetailsProps(),
}),
// We use this param to clear the joinSession cache if the error happens in connect_document flow.
errorFrom: handler,
},
);
}
Expand Down
10 changes: 3 additions & 7 deletions packages/drivers/odsp-driver/src/odspDelayLoadedDeltaStream.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,9 @@ export class OdspDelayLoadedDeltaStream {
this.currentConnection = connection;
return connection;
} catch (error) {
// Remove join session information from cache only if it is an error related to connect_document and not a socket related error.
// Otherwise keep it in cache so that this session can be re-used after disconnection.
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
if ((error as any).errorFrom === "connect_document_error") {
this.clearJoinSessionTimer();
this.cache.sessionJoinCache.remove(this.joinSessionKey);
}
this.clearJoinSessionTimer();
this.cache.sessionJoinCache.remove(this.joinSessionKey);

const normalizedError = this.annotateConnectionError(
error,
"createDeltaConnection",
Expand Down
163 changes: 2 additions & 161 deletions packages/drivers/odsp-driver/src/test/joinSessionCacheTests.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,18 @@
*/

import { strict as assert } from "node:assert";

import type { IResolvedUrl } from "@fluidframework/driver-definitions/internal";
import { createOdspNetworkError } from "@fluidframework/odsp-doclib-utils/internal";
import { IResolvedUrl } from "@fluidframework/driver-definitions/internal";
import {
IOdspResolvedUrl,
ISocketStorageDiscovery,
} from "@fluidframework/odsp-driver-definitions/internal";
import type { IClient } from "@fluidframework/protocol-definitions";
import { MockLogger } from "@fluidframework/telemetry-utils/internal";
import { type SinonStub, stub } from "sinon";
import { Socket } from "socket.io-client";

import { createOdspUrl } from "../createOdspUrl.js";
import { OdspDocumentServiceFactory } from "../odspDocumentServiceFactory.js";
import { OdspDriverUrlResolver } from "../odspDriverUrlResolver.js";
import { getJoinSessionCacheKey } from "../odspUtils.js";
import * as socketModule from "../socketModule.js";
import * as joinSession from "../vroom.js";

// eslint-disable-next-line import/no-internal-modules
import { ClientSocketMock } from "./socketTests/socketMock.js";

describe("expose joinSessionInfo Tests", () => {
const siteUrl = "https://www.localhost.xxx";
const driveId = "driveId";
const itemId = "itemId";
let socket: ClientSocketMock | undefined;

const resolvedUrl = {
siteUrl,
Expand All @@ -45,35 +30,13 @@ describe("expose joinSessionInfo Tests", () => {
id: "id",
tenantId: "tenantId",
snapshotStorageUrl: "https://fake/snapshotStorageUrl",
socketToken: "token", // providing socket token here so that the tests can bypass the need for token fetcher callback
refreshSessionDurationSeconds: 5,
refreshSessionDurationSeconds: 100,
};
const odspDocumentServiceFactory = new OdspDocumentServiceFactory(
async (_options) => "token",
async (_options) => "token",
);

function addJoinSessionStub(): SinonStub {
const joinSessionStub = stub(joinSession, "fetchJoinSession").callsFake(
async () => joinSessionResponse,
);
return joinSessionStub;
}

async function mockSocket<T>(_response: Socket, callback: () => Promise<T>): Promise<T> {
const getSocketCreationStub = stub(socketModule, "SocketIOClientStatic");
getSocketCreationStub.returns(_response);
try {
return await callback();
} finally {
getSocketCreationStub.restore();
}
}

afterEach(() => {
socket?.close();
});

it("Response missing in join session cache", async () => {
const info = await odspDocumentServiceFactory.getRelayServiceSessionInfo(resolvedUrl);
assert(info === undefined, "no cached response");
Expand Down Expand Up @@ -103,126 +66,4 @@ describe("expose joinSessionInfo Tests", () => {
}
assert(failed, "resolved url not correct");
});

it("Error of type connect_document_error should clear joinSession info from cache", async () => {
// joinSession stub will be internally invoked by connectToDeltaStream below so mocking it here.
const joinSessionStub = addJoinSessionStub();

// Setup for mocking socket a error when connectToDeltaStream gets executed below
const resolver = new OdspDriverUrlResolver();
const odspResolvedUrl = await resolver.resolve({
url: createOdspUrl({ driveId, itemId, siteUrl, dataStorePath: "/" }),
});
const service = await odspDocumentServiceFactory.createDocumentService(
odspResolvedUrl,
new MockLogger().toTelemetryLogger(),
);
const errorToThrow = createOdspNetworkError("TestError", 429);
const errorFromEvent = "connect_document_error";
socket = new ClientSocketMock({
connect_document: { eventToEmit: errorFromEvent, errorToThrow },
});
const client: IClient = {
mode: "read",
details: { capabilities: { interactive: true } },
permission: [],
user: { id: "id" },
scopes: [],
};

// Save a mock joinSession response in nonPersistenCache to test with later.
const cacheKey = getJoinSessionCacheKey(odspResolvedUrl);
// eslint-disable-next-line @typescript-eslint/dot-notation, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
odspDocumentServiceFactory["nonPersistentCache"].sessionJoinCache.add(
getJoinSessionCacheKey(odspResolvedUrl),
async () => {
return { entryTime: Date.now(), joinSessionResponse };
},
);

try {
await mockSocket(socket as unknown as Socket, async () =>
service.connectToDeltaStream(client),
);
} catch (error) {
assert(
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
(error as any).errorFrom === errorFromEvent,
`errorFrom param with value as '${errorFromEvent}' should be available`,
);

const info =
await odspDocumentServiceFactory.getRelayServiceSessionInfo(odspResolvedUrl);
assert(
info === undefined,
`joinSession cache should get cleared when '${errorFromEvent}' occurs`,
);
} finally {
// reset nonPersistenCache changes from the test
// eslint-disable-next-line @typescript-eslint/dot-notation, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
odspDocumentServiceFactory["nonPersistentCache"].sessionJoinCache.remove(cacheKey);
joinSessionStub.restore();
}
});

it("Socket errors should not result in clearing of joinSession info from cache", async () => {
// joinSession stub will be internally invoked by connectToDeltaStream below so mocking it here.
const joinSessionStub = addJoinSessionStub();

// Setup for mocking socket a error when connectToDeltaStream gets executed below
const resolver = new OdspDriverUrlResolver();
const odspResolvedUrl = await resolver.resolve({
url: createOdspUrl({ driveId, itemId, siteUrl, dataStorePath: "/" }),
});
const service = await odspDocumentServiceFactory.createDocumentService(
odspResolvedUrl,
new MockLogger().toTelemetryLogger(),
);
const errorToThrow = createOdspNetworkError("TestSocketError", 401);
const errorFromEvent = "connect_error";
socket = new ClientSocketMock({
connect_document: { eventToEmit: errorFromEvent, errorToThrow },
});
const client: IClient = {
mode: "read",
details: { capabilities: { interactive: true } },
permission: [],
user: { id: "id" },
scopes: [],
};

// Save a mock joinSession response in nonPersistenCache to test with later.
const cacheKey = getJoinSessionCacheKey(odspResolvedUrl);
// eslint-disable-next-line @typescript-eslint/dot-notation, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
odspDocumentServiceFactory["nonPersistentCache"].sessionJoinCache.add(
cacheKey,
async () => {
return { entryTime: Date.now(), joinSessionResponse };
},
);

try {
await mockSocket(socket as unknown as Socket, async () =>
service.connectToDeltaStream(client),
);
} catch (error) {
assert(
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
(error as any).errorFrom === errorFromEvent,
`errorFrom param with value as ${errorFromEvent} should be present`,
);

const info =
await odspDocumentServiceFactory.getRelayServiceSessionInfo(odspResolvedUrl);
assert(
info === joinSessionResponse,
`joinSession cache should not get cleared when '${errorFromEvent}' occurs`,
);
} finally {
// reset nonPersistenCache changes from the test
// eslint-disable-next-line @typescript-eslint/dot-notation, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call
odspDocumentServiceFactory["nonPersistentCache"].sessionJoinCache.remove(cacheKey);
joinSessionStub.restore();
}
});
});
14 changes: 1 addition & 13 deletions packages/drivers/odsp-driver/src/test/socketTests/socketMock.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,19 +106,7 @@ export class ClientSocketMock extends TypedEventEmitter<SocketMockEvents> {
case "connect_document": {
const connectMessage = args[0] as IConnect;
switch (this.mockSocketConnectResponse.connect_document.eventToEmit) {
case "connect_document_error": {
const errorToThrow =
this.mockSocketConnectResponse.connect_document.errorToThrow ??
createGenericNetworkError(
"TestError",
{ canRetry: false },
{ driverVersion, isSocketError: false },
);
this.emit(
this.mockSocketConnectResponse.connect_document.eventToEmit,
errorToThrow,
);
}
case "connect_document_error":
case "connect_error": {
const errorToThrow =
this.mockSocketConnectResponse.connect_document.errorToThrow ??
Expand Down

0 comments on commit 3342aff

Please sign in to comment.