Presentation: Fix presentation rpc requests caching

common/changes/@itwin/presentation-backend/presentation-fix-presentation-rpc-requests-caching_2022-02-28-08-46.json

@@ -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"
+}

presentation/backend/src/presentation-backend/PresentationRpcImpl.ts

@@ -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}.`);
       },
     });
   }
@@ -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}`);
 

presentation/backend/src/presentation-backend/TemporaryStorage.ts

@@ -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;
@@ -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();
@@ -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);
   }
@@ -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);
   }
 
   /**

presentation/backend/src/test/PresentationRpcImpl.test.ts

@@ -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,
@@ -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 });
     });
 
@@ -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,
@@ -1473,3 +1515,9 @@ describe("PresentationRpcImpl", () => {
   });
 
 });
+
+function createIModelRpcProps(): IModelRpcProps {
+  return {
+    key: Guid.createValue(),
+  };
+}

presentation/backend/src/test/TemporaryStorage.test.ts

@@ -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", () => {
@@ -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", () => {
@@ -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", () => {