From 12a02263ad3e79ab429e80c5518edc1100ad19b4 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 17 Apr 2023 18:31:49 -0400 Subject: [PATCH] IPC blocking fix (backport #5400) [release/3.7.x] (#5404) Co-authored-by: swbsi <69857376+swbsi@users.noreply.github.com> --- common/api/core-backend.api.md | 3 ++ .../ipc-fix_2023-04-14-19-56.json | 10 +++++ .../core-common/ipc-fix_2023-04-14-19-56.json | 10 +++++ core/backend/src/LocalhostIpcHost.ts | 6 +-- core/common/src/ipc/IpcWebSocket.ts | 7 +-- full-stack-tests/rpc/src/backend/electron.ts | 2 + full-stack-tests/rpc/src/backend/ipc.ts | 26 ++++++++++- full-stack-tests/rpc/src/backend/websocket.ts | 2 + .../{IpcWebSocket.test.ts => Ipc.test.ts} | 45 +++++++++++++------ 9 files changed, 86 insertions(+), 25 deletions(-) create mode 100644 common/changes/@itwin/core-backend/ipc-fix_2023-04-14-19-56.json create mode 100644 common/changes/@itwin/core-common/ipc-fix_2023-04-14-19-56.json rename full-stack-tests/rpc/src/frontend/{IpcWebSocket.test.ts => Ipc.test.ts} (60%) diff --git a/common/api/core-backend.api.md b/common/api/core-backend.api.md index f188cbc4ad1a..8ee565c9568e 100644 --- a/common/api/core-backend.api.md +++ b/common/api/core-backend.api.md @@ -123,6 +123,7 @@ import { InternetConnectivityStatus } from '@itwin/core-common'; import { IpcAppNotifications } from '@itwin/core-common'; import { IpcListener } from '@itwin/core-common'; import { IpcSocketBackend } from '@itwin/core-common'; +import { IpcWebSocketBackend } from '@itwin/core-common'; import { JSONSchema } from '@itwin/core-bentley'; import { JSONSchemaType } from '@itwin/core-bentley'; import { JSONSchemaTypeName } from '@itwin/core-bentley'; @@ -3636,6 +3637,8 @@ export class LocalhostIpcHost { // (undocumented) static connect(connection: ws): void; // (undocumented) + static socket: IpcWebSocketBackend; + // (undocumented) static startup(opts?: { localhostIpcHost?: LocalhostIpcHostOpts; iModelHost?: IModelHostOptions; diff --git a/common/changes/@itwin/core-backend/ipc-fix_2023-04-14-19-56.json b/common/changes/@itwin/core-backend/ipc-fix_2023-04-14-19-56.json new file mode 100644 index 000000000000..3280e637e0dd --- /dev/null +++ b/common/changes/@itwin/core-backend/ipc-fix_2023-04-14-19-56.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@itwin/core-backend", + "comment": "Prevent IPC requests from blocking the backend.", + "type": "none" + } + ], + "packageName": "@itwin/core-backend" +} diff --git a/common/changes/@itwin/core-common/ipc-fix_2023-04-14-19-56.json b/common/changes/@itwin/core-common/ipc-fix_2023-04-14-19-56.json new file mode 100644 index 000000000000..ad2a7ca7c0e6 --- /dev/null +++ b/common/changes/@itwin/core-common/ipc-fix_2023-04-14-19-56.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "@itwin/core-common", + "comment": "Prevent IPC requests from blocking the backend.", + "type": "none" + } + ], + "packageName": "@itwin/core-common" +} diff --git a/core/backend/src/LocalhostIpcHost.ts b/core/backend/src/LocalhostIpcHost.ts index b05dde13fd5a..0ca6862dcf65 100644 --- a/core/backend/src/LocalhostIpcHost.ts +++ b/core/backend/src/LocalhostIpcHost.ts @@ -73,7 +73,7 @@ class RpcHandler extends IpcHandler { /** @internal */ export class LocalhostIpcHost { private static _initialized = false; - private static _socket: IpcWebSocketBackend; + public static socket: IpcWebSocketBackend; public static connect(connection: ws) { (IpcWebSocket.transport as LocalTransport).connect(connection); @@ -85,11 +85,11 @@ export class LocalhostIpcHost { if (!this._initialized) { registerHandler = true; IpcWebSocket.transport = new LocalTransport(opts?.localhostIpcHost ?? {}); - this._socket = new IpcWebSocketBackend(); + this.socket = new IpcWebSocketBackend(); this._initialized = true; } - await IpcHost.startup({ ipcHost: { socket: this._socket }, iModelHost: opts?.iModelHost }); + await IpcHost.startup({ ipcHost: { socket: this.socket }, iModelHost: opts?.iModelHost }); if (registerHandler) { RpcHandler.register(); diff --git a/core/common/src/ipc/IpcWebSocket.ts b/core/common/src/ipc/IpcWebSocket.ts index 3f34a103933c..d7986e847b21 100644 --- a/core/common/src/ipc/IpcWebSocket.ts +++ b/core/common/src/ipc/IpcWebSocket.ts @@ -132,7 +132,6 @@ export class IpcWebSocketFrontend extends IpcWebSocket implements IpcSocketFront export class IpcWebSocketBackend extends IpcWebSocket implements IpcSocketBackend { private _handlers = new Map Promise>(); private _processingQueue: IpcWebSocketMessage[] = []; - private _processing: IpcWebSocketMessage | undefined; public constructor() { super(); @@ -161,7 +160,7 @@ export class IpcWebSocketBackend extends IpcWebSocket implements IpcSocketBacken } private async processMessages() { - if (this._processing || !this._processingQueue.length) { + if (!this._processingQueue.length) { return; } @@ -169,8 +168,6 @@ export class IpcWebSocketBackend extends IpcWebSocket implements IpcSocketBacken if (message && message.method) { const handler = this._handlers.get(message.channel); if (handler) { - this._processing = message; - let args = message.data; if (typeof (args) === "undefined") args = []; @@ -184,8 +181,6 @@ export class IpcWebSocketBackend extends IpcWebSocket implements IpcSocketBacken data: response, sequence: -1, }); - - this._processing = undefined; } } diff --git a/full-stack-tests/rpc/src/backend/electron.ts b/full-stack-tests/rpc/src/backend/electron.ts index d951821ca2ac..73a1872e2862 100644 --- a/full-stack-tests/rpc/src/backend/electron.ts +++ b/full-stack-tests/rpc/src/backend/electron.ts @@ -6,6 +6,7 @@ import { registerBackendCallback } from "@itwin/certa/lib/utils/CallbackUtils"; import { BackendTestCallbacks } from "../common/SideChannels"; import { commonSetup } from "./CommonBackendSetup"; import { ElectronHost } from "@itwin/core-electron/lib/cjs/ElectronBackend"; +import { setupIpcTestElectron } from "./ipc"; async function init() { await commonSetup(); @@ -14,6 +15,7 @@ async function init() { ElectronHost.rpcConfig.protocol.transferChunkThreshold = value; return true; }); + setupIpcTestElectron(); } module.exports = init(); diff --git a/full-stack-tests/rpc/src/backend/ipc.ts b/full-stack-tests/rpc/src/backend/ipc.ts index 3c9ab4236307..a78302e4af98 100644 --- a/full-stack-tests/rpc/src/backend/ipc.ts +++ b/full-stack-tests/rpc/src/backend/ipc.ts @@ -6,7 +6,27 @@ import { registerBackendCallback } from "@itwin/certa/lib/utils/CallbackUtils"; import { IpcWebSocketBackend, iTwinChannel } from "@itwin/core-common"; import { BackendTestCallbacks } from "../common/SideChannels"; -export async function setupIpcTest(before = async () => { }) { +function orderTest(socket: { handle(channel: string, listener: (event: any, ...args: any[]) => Promise): void }) { + socket.handle("a", async (_event: Event, methodName: string, ..._args: any[]) => { + return [methodName, "a"]; + }); + + socket.handle("b", async (_event: Event, methodName: string, ..._args: any[]) => { + return new Promise((resolve) => { + setTimeout(() => resolve([methodName, "b"]), 1000); + }); + }); + + socket.handle("c", async (_event: Event, methodName: string, ..._args: any[]) => { + return [methodName, "c"]; + }); +} + +export function setupIpcTestElectron() { + orderTest(require("electron").ipcMain); // eslint-disable-line @typescript-eslint/no-var-requires +} + +export async function setupIpcTest(before = async () => { }, socketOverride?: IpcWebSocketBackend) { let socket: IpcWebSocketBackend; let ready: () => void; const started = new Promise((resolve) => ready = resolve); @@ -14,7 +34,7 @@ export async function setupIpcTest(before = async () => { }) { registerBackendCallback(BackendTestCallbacks.startIpcTest, () => { setTimeout(async () => { await before(); - socket = new IpcWebSocketBackend(); + socket = socketOverride || (new IpcWebSocketBackend()); socket.addListener("test", (_evt: Event, ...arg: any[]) => { if (arg[0] !== 1 || arg[1] !== 2 || arg[2] !== 3) { @@ -26,6 +46,8 @@ export async function setupIpcTest(before = async () => { }) { return [methodName, ...args]; }); + orderTest(socket); + socket.handle(iTwinChannel("ipc-app"), async (_event: Event, _methodName: string, ..._args: any[]) => { return { result: undefined }; }); diff --git a/full-stack-tests/rpc/src/backend/websocket.ts b/full-stack-tests/rpc/src/backend/websocket.ts index 1f58f24a7ad5..51ea9f2bad40 100644 --- a/full-stack-tests/rpc/src/backend/websocket.ts +++ b/full-stack-tests/rpc/src/backend/websocket.ts @@ -9,6 +9,7 @@ import { WebEditServer } from "@itwin/express-server"; import { BackendTestCallbacks } from "../common/SideChannels"; import { AttachedInterface, rpcInterfaces } from "../common/TestRpcInterface"; import { commonSetup } from "./CommonBackendSetup"; +import { setupIpcTest } from "./ipc"; import { AttachedInterfaceImpl } from "./TestRpcImpl"; async function init() { @@ -29,6 +30,7 @@ async function init() { console.log(`Web backend for rpc full-stack-tests listening on port ${port}`); initializeAttachedInterfacesTest(rpcConfig); + setupIpcTest(async () => Promise.resolve(), LocalhostIpcHost.socket); // eslint-disable-line @typescript-eslint/no-floating-promises return () => { httpServer.close(); diff --git a/full-stack-tests/rpc/src/frontend/IpcWebSocket.test.ts b/full-stack-tests/rpc/src/frontend/Ipc.test.ts similarity index 60% rename from full-stack-tests/rpc/src/frontend/IpcWebSocket.test.ts rename to full-stack-tests/rpc/src/frontend/Ipc.test.ts index b3b8d281956a..09f942258da9 100644 --- a/full-stack-tests/rpc/src/frontend/IpcWebSocket.test.ts +++ b/full-stack-tests/rpc/src/frontend/Ipc.test.ts @@ -7,26 +7,45 @@ import { IpcWebSocketFrontend } from "@itwin/core-common"; import { executeBackendCallback } from "@itwin/certa/lib/utils/CallbackUtils"; import { assert } from "chai"; import { BackendTestCallbacks } from "../common/SideChannels"; -import { currentEnvironment } from "./_Setup.test"; -if (!ProcessDetector.isElectronAppFrontend) { +function orderTest(it: Mocha.TestFunction, socketSource: () => { invoke(channel: string, ...args: any[]): Promise }) { + async function onResponse(request: Promise, responses: string[]) { + const data = await request; + responses.push(data[0]); + } + + it("should preserve order", async () => { + const socket = socketSource(); + + const responses: string[] = []; + + const a = socket.invoke("a", "a"); + const b = socket.invoke("b", "b"); + const c = socket.invoke("c", "c"); + + onResponse(a, responses); // eslint-disable-line @typescript-eslint/no-floating-promises + onResponse(b, responses); // eslint-disable-line @typescript-eslint/no-floating-promises + onResponse(c, responses); // eslint-disable-line @typescript-eslint/no-floating-promises + + await Promise.all([a, b, c]); + assert.deepEqual(responses, ["a", "c", "b"]); + }); +} + +if (ProcessDetector.isElectronAppFrontend) { + describe("ElectronIpc", () => { + orderTest(it, () => require("electron").ipcRenderer); // eslint-disable-line @typescript-eslint/no-var-requires + }); +} else { describe("IpcWebSocket", () => { let socket: IpcWebSocketFrontend; before(async () => { - if (currentEnvironment === "websocket") { - return; - } - assert(await executeBackendCallback(BackendTestCallbacks.startIpcTest)); socket = new IpcWebSocketFrontend(); }); it("should support send/receive", async () => { - if (currentEnvironment === "websocket") { - return; - } - return new Promise(async (resolve) => { socket.addListener("test", (_evt: Event, ...arg: any[]) => { assert.equal(arg[0], 4); @@ -42,10 +61,6 @@ if (!ProcessDetector.isElectronAppFrontend) { }); it("should support invoke", async () => { - if (currentEnvironment === "websocket") { - return; - } - return new Promise(async (resolve) => { const invoked = await socket.invoke("testinvoke", "hi", 1, 2, 3); assert.equal(invoked[0], "hi"); @@ -55,5 +70,7 @@ if (!ProcessDetector.isElectronAppFrontend) { resolve(); }); }); + + orderTest(it, () => socket); }); }