Skip to content

Commit

Permalink
Presentation: Fix presentation rpc requests caching (#3275)
Browse files Browse the repository at this point in the history
* Fix requests memoization

* Fix a false log message

(cherry picked from commit e39af7a)
  • Loading branch information
grigasp authored and mergify-bot committed Feb 28, 2022
1 parent f79ea0c commit da5e7e3
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 16 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@itwin/presentation-backend",
"comment": "Fix RPC requests' memoization causing similar requests to different iModels to be memoized as a single request.",
"type": "none"
}
],
"packageName": "@itwin/presentation-backend"
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ export class PresentationRpcImpl extends PresentationRpcInterface implements IDi
// attempt to clean up every second
cleanupInterval: 1000,

cleanupHandler: (id) => {
Logger.logTrace(PresentationBackendLoggerCategory.Rpc, `Cleaning up request without frontend retrieving it: ${id}.`);
cleanupHandler: (id, _, reason) => {
if (reason !== "request")
Logger.logTrace(PresentationBackendLoggerCategory.Rpc, `Cleaning up request without frontend retrieving it: ${id}.`);
},
});
}
Expand Down Expand Up @@ -107,7 +108,7 @@ export class PresentationRpcImpl extends PresentationRpcInterface implements IDi
}

private async makeRequest<TRpcOptions extends { rulesetOrId?: Ruleset | string, clientId?: string, diagnostics?: DiagnosticsOptions, rulesetVariables?: RulesetVariableJSON[] }, TResult>(token: IModelRpcProps, requestId: string, requestOptions: TRpcOptions, request: ContentGetter<Promise<TResult>>): PresentationRpcResponse<TResult> {
const requestKey = JSON.stringify(requestOptions);
const requestKey = JSON.stringify({ iModelKey: token.key, requestId, requestOptions });

Logger.logInfo(PresentationBackendLoggerCategory.Rpc, `Received '${requestId}' request. Params: ${requestKey}`);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { PresentationError, PresentationStatus } from "@itwin/presentation-commo
*/
export interface TemporaryStorageProps<T> {
/** A method that's called for every value before it's removed from storage */
cleanupHandler?: (id: string, value: T) => void;
cleanupHandler?: (id: string, value: T, reason: "timeout" | "dispose" | "request") => void;

onDisposedSingle?: (id: string) => void;
onDisposedAll?: () => void;
Expand Down Expand Up @@ -90,7 +90,7 @@ export class TemporaryStorage<T> implements IDisposable {

if (this.props.cleanupHandler) {
this._values.forEach((v, id) => {
this.props.cleanupHandler!(id, v.value);
this.props.cleanupHandler!(id, v.value, "dispose");
});
}
this._values.clear();
Expand Down Expand Up @@ -119,12 +119,12 @@ export class TemporaryStorage<T> implements IDisposable {
}
}
for (const id of valuesToDispose)
this.deleteExistingEntry(id);
this.deleteExistingEntry(id, true);
};

private deleteExistingEntry(id: string) {
private deleteExistingEntry(id: string, isTimeout: boolean) {
assert(this._values.has(id));
this.props.cleanupHandler && this.props.cleanupHandler(id, this._values.get(id)!.value);
this.props.cleanupHandler && this.props.cleanupHandler(id, this._values.get(id)!.value, isTimeout ? "timeout" : "request");
this._values.delete(id);
this.props.onDisposedSingle && this.props.onDisposedSingle(id);
}
Expand Down Expand Up @@ -164,7 +164,7 @@ export class TemporaryStorage<T> implements IDisposable {
public deleteValue(id: string) {
// istanbul ignore else
if (this._values.has(id))
this.deleteExistingEntry(id);
this.deleteExistingEntry(id, false);
}

/**
Expand Down
54 changes: 51 additions & 3 deletions presentation/backend/src/test/PresentationRpcImpl.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as faker from "faker";
import * as sinon from "sinon";
import * as moq from "typemoq";
import { IModelDb } from "@itwin/core-backend";
import { Id64String, using } from "@itwin/core-bentley";
import { Guid, Id64String, using } from "@itwin/core-bentley";
import { IModelNotFoundResponse, IModelRpcProps } from "@itwin/core-common";
import {
Content, ContentDescriptorRequestOptions, ContentDescriptorRpcRequestOptions, ContentFlags, ContentInstanceKeysRpcRequestOptions,
Expand Down Expand Up @@ -139,14 +139,14 @@ describe("PresentationRpcImpl", () => {
clientManagerFactory: () => presentationManagerMock.object,
});
testData = {
imodelToken: moq.Mock.ofType<IModelRpcProps>().object,
imodelToken: createIModelRpcProps(),
imodelMock: moq.Mock.ofType<IModelDb>(),
rulesetOrId: faker.random.word(),
pageOptions: { start: 123, size: 45 } as PageOptions,
displayType: "sample display type",
};
defaultRpcParams = { clientId: faker.random.uuid() };
stub_IModelDb_findByKey = sinon.stub(IModelDb, "findByKey").returns(testData.imodelMock.object);
stub_IModelDb_findByKey = sinon.stub(IModelDb, "findByKey").withArgs(testData.imodelToken.key).returns(testData.imodelMock.object);
impl = new PresentationRpcImpl({ requestTimeout: 10 });
});

Expand Down Expand Up @@ -217,6 +217,48 @@ describe("PresentationRpcImpl", () => {
expect(actualResult.result).to.eq(999);
});

it("should handle different iModel requests", async () => {
impl.dispose();
impl = new PresentationRpcImpl({ requestTimeout: 0 });
const rpcOptions: HierarchyRpcRequestOptions = {
...defaultRpcParams,
rulesetOrId: testData.rulesetOrId,
};

const managerOptions1: HierarchyRequestOptions<IModelDb, NodeKey> = {
imodel: testData.imodelMock.object,
rulesetOrId: testData.rulesetOrId,
parentKey: undefined,
};
const result1 = new ResolvablePromise<number>();
presentationManagerMock.setup(async (x) => x.getNodesCount(managerOptions1))
.returns(async () => result1)
.verifiable();

const iModelRpcProps2 = createIModelRpcProps();
const iModelMock2 = moq.Mock.ofType<IModelDb>();
stub_IModelDb_findByKey.withArgs(iModelRpcProps2.key).returns(iModelMock2.object);
const managerOptions2: HierarchyRequestOptions<IModelDb, NodeKey> = {
imodel: iModelMock2.object,
rulesetOrId: testData.rulesetOrId,
parentKey: undefined,
};
const result2 = new ResolvablePromise<number>();
presentationManagerMock.setup(async (x) => x.getNodesCount(managerOptions2))
.returns(async () => result2)
.verifiable();

const actualResultPromise1 = impl.getNodesCount(testData.imodelToken, rpcOptions);
const actualResultPromise2 = impl.getNodesCount(iModelRpcProps2, rpcOptions);
presentationManagerMock.verifyAll();

await result1.resolve(111);
await result2.resolve(222);

expect((await actualResultPromise1).result).to.eq(111);
expect((await actualResultPromise2).result).to.eq(222);
});

it("should forward ruleset variables to manager", async () => {
const rpcOptions: HierarchyRpcRequestOptions = {
...defaultRpcParams,
Expand Down Expand Up @@ -1473,3 +1515,9 @@ describe("PresentationRpcImpl", () => {
});

});

function createIModelRpcProps(): IModelRpcProps {
return {
key: Guid.createValue(),
};
}
22 changes: 18 additions & 4 deletions presentation/backend/src/test/TemporaryStorage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ describe("TemporaryStorage", () => {
storage.dispose();

expect(cleanupHandler.callCount).to.eq(values.length);
values.forEach((v, i) => {
expect(cleanupHandler.args[i][0]).to.eq(v);
});
expect(cleanupHandler.getCall(0)).to.be.calledWithExactly("a", "a", "dispose");
expect(cleanupHandler.getCall(1)).to.be.calledWithExactly("b", "b", "dispose");
expect(cleanupHandler.getCall(2)).to.be.calledWithExactly("c", "c", "dispose");
});

it("calls `onDisposedAll` callback", () => {
Expand Down Expand Up @@ -126,6 +126,20 @@ describe("TemporaryStorage", () => {

});

describe("deleteValue", () => {

it("calls cleanup handler", () => {
const cleanupHandler = sinon.spy();
const storage = new TemporaryStorage<string>({
cleanupHandler,
});
storage.addValue("a", "A");
storage.deleteValue("a");
expect(cleanupHandler).to.be.calledOnceWithExactly("a", "A", "request");
});

});

describe("disposeOutdatedValues", () => {

it("doesn't dispose value if neither `unusedValueLifetime` nor `maxValueLifetime` are specified", () => {
Expand Down Expand Up @@ -254,7 +268,7 @@ describe("TemporaryStorage", () => {
expect(storage.values.length).to.eq(1);
storage.disposeOutdatedValues();
expect(storage.values.length).to.eq(0);
expect(cleanupHandler).to.be.calledOnceWith("a", "A");
expect(cleanupHandler).to.be.calledOnceWith("a", "A", "timeout");
});

it("calls `onDisposedSingle` for disposed values", () => {
Expand Down

0 comments on commit da5e7e3

Please sign in to comment.