From 651c585c64c8e60624b8b0f60dda3b3459dd7375 Mon Sep 17 00:00:00 2001 From: Abderraouf El Gasser Date: Fri, 21 Aug 2020 18:33:10 +0200 Subject: [PATCH 1/3] Handle terminate on the proxy to terminate workers --- karma.conf.js | 4 +++- src/comlink.ts | 6 ++++++ tests/worker.comlink.test.js | 17 +++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/karma.conf.js b/karma.conf.js index c7840f40..16eae6d4 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -50,8 +50,10 @@ module.exports = function (config) { // local issues and I have no idea how to fix them. // I know that’s not a good reason to disable tests, // but Safari TP is relatively unimportant. + // Filtering Safari because of this https://github.com/karma-runner/karma-safari-launcher/issues/24 + // Running Safari 13.1.2 (15609.3.5.1.3) on Mac OS Catalina (10.15.6 (19G73)) return availableBrowsers.filter( - (browser) => browser !== "SafariTechPreview" + (browser) => browser !== "SafariTechPreview" && browser !== "Safari" ); } }, diff --git a/src/comlink.ts b/src/comlink.ts index fbecf655..05f9811c 100644 --- a/src/comlink.ts +++ b/src/comlink.ts @@ -421,6 +421,12 @@ function createProxy( apply(_target, _thisArg, rawArgumentList) { throwIfProxyReleased(isProxyReleased); const last = path[path.length - 1]; + // Users would expect terminate to kill the Worker not call a terminate method inside the worker + if (last === "terminate" && ep instanceof Worker) { + isProxyReleased = true; + ep.terminate(); + return Promise.resolve(); + } if ((last as any) === createEndpoint) { return requestResponseMessage(ep, { type: MessageType.ENDPOINT, diff --git a/tests/worker.comlink.test.js b/tests/worker.comlink.test.js index 3d87ef98..349995bf 100644 --- a/tests/worker.comlink.test.js +++ b/tests/worker.comlink.test.js @@ -33,4 +33,21 @@ describe("Comlink across workers", function () { const otherProxy = Comlink.wrap(otherEp); expect(await otherProxy(20, 1)).to.equal(21); }); + + it("honors terminate on the proxy", function () { + const originalTerminate = this.worker.terminate; + + let terminateCalled = false; + // poorman's mock + this.worker.terminate = function () { + terminateCalled = true; + }; + + const proxy = Comlink.wrap(this.worker); + expect(terminateCalled).to.be.false; + proxy.terminate(); + expect(terminateCalled).to.be.true; + + this.worker.terminate = originalTerminate; + }); }); From adcc7d3cff17e2912aeafa1be979427d8f68b424 Mon Sep 17 00:00:00 2001 From: Abderraouf El Gasser Date: Fri, 21 Aug 2020 18:39:56 +0200 Subject: [PATCH 2/3] Revert karma configuration --- karma.conf.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/karma.conf.js b/karma.conf.js index 16eae6d4..c7840f40 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -50,10 +50,8 @@ module.exports = function (config) { // local issues and I have no idea how to fix them. // I know that’s not a good reason to disable tests, // but Safari TP is relatively unimportant. - // Filtering Safari because of this https://github.com/karma-runner/karma-safari-launcher/issues/24 - // Running Safari 13.1.2 (15609.3.5.1.3) on Mac OS Catalina (10.15.6 (19G73)) return availableBrowsers.filter( - (browser) => browser !== "SafariTechPreview" && browser !== "Safari" + (browser) => browser !== "SafariTechPreview" ); } }, From 2f6913f131e3f962824d9b9660347c490b91964d Mon Sep 17 00:00:00 2001 From: Abderraouf El Gasser Date: Fri, 21 Aug 2020 18:53:41 +0200 Subject: [PATCH 3/3] Updated documentation --- README.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/README.md b/README.md index dda9e10f..7e3df9cd 100644 --- a/README.md +++ b/README.md @@ -184,6 +184,19 @@ Windows and Web Workers have a slightly different variants of `postMessage`. If For a usage example, take a look at the non-worker examples in the `docs` folder. +### `.terminate()` + +Web Workers must be terminated to release resources and threads. When using comlink with [`comlink-loader`](https://github.com/GoogleChromeLabs/comlink-loader) call the terminate method on the proxy returned by `comlink-loader` as comlink-loader doesn't give access to the underlying Worker object. + +```ts +import MyWorker from "comlink-loader!./worker"; +const workerProxy = new MyWorker() as any; +const result = await workerProxy.doSomething(); +workerProxy.terminate(); +``` + +Note: `workerProxy[Comlink.releaseProxy]();` wouldn't work in this case as it only releases the endpoint created for the proxy. + ## TypeScript Comlink does provide TypeScript types. When you `expose()` something of type `T`, the corresponding `wrap()` call will return something of type `Comlink.Remote`. While this type has been battle-tested over some time now, it is implemented on a best-effort basis. There are some nuances that are incredibly hard if not impossible to encode correctly in TypeScript’s type system. It _may_ sometimes be necessary to force a certain type using `as unknown as `.