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.

Sign up for GitHub

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

Revert: Reset joinSession cache only on connect_document_error #20784

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Revert: Reset joinSession cache only on connect_document_error #20784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions packages/drivers/driver-base/src/documentDeltaConnection.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading