From bff801ca3c299c56a872691e7f7e6f47aaabaeaf Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Sat, 13 Mar 2021 01:40:25 +0800 Subject: [PATCH 1/5] fix memory leaks caused by closures in jest-worker --- packages/jest-worker/src/Farm.ts | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/packages/jest-worker/src/Farm.ts b/packages/jest-worker/src/Farm.ts index c87897a78d1a..ac1e38cde5ff 100644 --- a/packages/jest-worker/src/Farm.ts +++ b/packages/jest-worker/src/Farm.ts @@ -66,7 +66,14 @@ export default class Farm { }; const promise: PromiseWithCustomMessage = new Promise( - (resolve, reject) => { + // Bind args to this function so it won't reference to the parent scope. + // This prevents a memory leak in v8, because otherwise the function will + // retaine args for the closure. + (( + args: Array, + resolve: null | ((value: unknown) => void), + reject: null | ((reason?: any) => void), + ) => { const computeWorkerKey = this._computeWorkerKey; const request: ChildMessage = [CHILD_MESSAGE_CALL, false, method, args]; @@ -87,10 +94,14 @@ export default class Farm { const onEnd: OnEnd = (error: Error | null, result: unknown) => { customMessageListeners.clear(); if (error) { - reject(error); + reject?.(error); } else { - resolve(result); + resolve?.(result); } + + // Delete reference to the Promise so its result can be garbage + // collected after calling this method. + reject = resolve = null; }; const task = {onCustomMessage, onEnd, onStart, request}; @@ -101,7 +112,7 @@ export default class Farm { } else { this._push(task); } - }, + }).bind(null, args), ); promise.UNSTABLE_onCustomMessage = addCustomMessageListener; @@ -124,8 +135,12 @@ export default class Farm { throw new Error('Queue implementation returned processed task'); } + // Reference the task object outside so it won't be retained by onEnd, + // and other properties of the task object, such as task.request can be + // garbage collected. + const taskOnEnd = task.onEnd; const onEnd = (error: Error | null, result: unknown) => { - task.onEnd(error, result); + taskOnEnd(error, result); this._unlock(workerId); this._process(workerId); From 698e78e5630ab72c125f93abc72eda28c94c8a73 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Sat, 13 Mar 2021 02:14:16 +0800 Subject: [PATCH 2/5] clean up onEnd when process ends --- packages/jest-worker/src/Farm.ts | 12 ++++-------- .../jest-worker/src/workers/NodeThreadsWorker.ts | 10 ++++++++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/jest-worker/src/Farm.ts b/packages/jest-worker/src/Farm.ts index ac1e38cde5ff..146040c13dd1 100644 --- a/packages/jest-worker/src/Farm.ts +++ b/packages/jest-worker/src/Farm.ts @@ -71,8 +71,8 @@ export default class Farm { // retaine args for the closure. (( args: Array, - resolve: null | ((value: unknown) => void), - reject: null | ((reason?: any) => void), + resolve: (value: unknown) => void, + reject: (reason?: any) => void, ) => { const computeWorkerKey = this._computeWorkerKey; const request: ChildMessage = [CHILD_MESSAGE_CALL, false, method, args]; @@ -94,14 +94,10 @@ export default class Farm { const onEnd: OnEnd = (error: Error | null, result: unknown) => { customMessageListeners.clear(); if (error) { - reject?.(error); + reject(error); } else { - resolve?.(result); + resolve(result); } - - // Delete reference to the Promise so its result can be garbage - // collected after calling this method. - reject = resolve = null; }; const task = {onCustomMessage, onEnd, onStart, request}; diff --git a/packages/jest-worker/src/workers/NodeThreadsWorker.ts b/packages/jest-worker/src/workers/NodeThreadsWorker.ts index ab7178b89a8e..0d13ce958dd4 100644 --- a/packages/jest-worker/src/workers/NodeThreadsWorker.ts +++ b/packages/jest-worker/src/workers/NodeThreadsWorker.ts @@ -206,7 +206,7 @@ export default class ExperimentalWorker implements WorkerInterface { send( request: ChildMessage, onProcessStart: OnStart, - onProcessEnd: OnEnd, + onProcessEnd: OnEnd | null, onCustomMessage: OnCustomMessage, ): void { onProcessStart(this); @@ -214,7 +214,13 @@ export default class ExperimentalWorker implements WorkerInterface { // Clean the request to avoid sending past requests to workers that fail // while waiting for a new request (timers, unhandled rejections...) this._request = null; - return onProcessEnd(...args); + + const res = onProcessEnd?.(...args); + + // Clean up the reference so related closures can be garbage collected. + onProcessEnd = null; + + return res; }; this._onCustomMessage = (...arg) => onCustomMessage(...arg); From 852ec1c7dd09e4ec721f034c9c854bb71fd53594 Mon Sep 17 00:00:00 2001 From: Shu Ding Date: Sat, 13 Mar 2021 03:07:33 +0800 Subject: [PATCH 3/5] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f6c31fb0d4a..36ff4df5a7d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ - `[jest-worker]` Handle `ERR_IPC_CHANNEL_CLOSED` errors properly ([#11143](https://github.com/facebook/jest/pull/11143)) - `[pretty-format]` [**BREAKING**] Convert to ES Modules ([#10515](https://github.com/facebook/jest/pull/10515)) - `[pretty-format]` Only call `hasAttribute` if it's a function ([#11000](https://github.com/facebook/jest/pull/11000)) +- `[jest-worker]` Fix memory leaks of jest-worker ([#11187](https://github.com/facebook/jest/pull/11187)) ### Chore & Maintenance From 84cb516ca5248d6898129f23e1f2d347d47a9c8b Mon Sep 17 00:00:00 2001 From: Tim Seckinger Date: Sun, 14 Mar 2021 20:42:01 +0100 Subject: [PATCH 4/5] test(jest-worker): args memory leak regression test --- packages/jest-worker/package.json | 1 + .../src/__tests__/leak-integration.test.ts | 40 +++++++++++++++++++ packages/jest-worker/tsconfig.json | 3 +- yarn.lock | 1 + 4 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 packages/jest-worker/src/__tests__/leak-integration.test.ts diff --git a/packages/jest-worker/package.json b/packages/jest-worker/package.json index abe98a758070..e983152115e4 100644 --- a/packages/jest-worker/package.json +++ b/packages/jest-worker/package.json @@ -22,6 +22,7 @@ "@types/merge-stream": "^1.1.2", "@types/supports-color": "^7.2.0", "get-stream": "^6.0.0", + "jest-leak-detector": "^27.0.0-next.3", "worker-farm": "^1.6.0" }, "engines": { diff --git a/packages/jest-worker/src/__tests__/leak-integration.test.ts b/packages/jest-worker/src/__tests__/leak-integration.test.ts new file mode 100644 index 000000000000..ef16ae29b6a4 --- /dev/null +++ b/packages/jest-worker/src/__tests__/leak-integration.test.ts @@ -0,0 +1,40 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import {tmpdir} from 'os'; +import {join} from 'path'; +import {writeFileSync} from 'graceful-fs'; +import LeakDetector from 'jest-leak-detector'; +import {Worker} from '../..'; + +let workerFile!: string; +beforeAll(() => { + workerFile = join(tmpdir(), 'baz.js'); + writeFileSync(workerFile, `module.exports.fn = () => {};`); +}); + +let worker!: Worker; +beforeEach(() => { + worker = new Worker(workerFile, { + enableWorkerThreads: true, + exposedMethods: ['fn'], + }); +}); +afterEach(async () => { + await worker.end(); +}); + +it('does not retain arguments after a task finished', async () => { + let leakDetector!: LeakDetector; + await new Promise(resolve => { + const obj = {}; + leakDetector = new LeakDetector(obj); + (worker as any).fn(obj).then(resolve); + }); + + expect(await leakDetector.isLeaking()).toBe(false); +}); diff --git a/packages/jest-worker/tsconfig.json b/packages/jest-worker/tsconfig.json index 7bb06bce6d20..fd735f5969a8 100644 --- a/packages/jest-worker/tsconfig.json +++ b/packages/jest-worker/tsconfig.json @@ -3,5 +3,6 @@ "compilerOptions": { "rootDir": "src", "outDir": "build" - } + }, + "references": [{"path": "../jest-leak-detector"}] } diff --git a/yarn.lock b/yarn.lock index 4ca47e9c8255..98c1694f042e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -14641,6 +14641,7 @@ fsevents@^1.2.7: "@types/node": "*" "@types/supports-color": ^7.2.0 get-stream: ^6.0.0 + jest-leak-detector: ^27.0.0-next.3 merge-stream: ^2.0.0 supports-color: ^8.0.0 worker-farm: ^1.6.0 From 66a612d595f303aef89518e48f704d03b9dcddcd Mon Sep 17 00:00:00 2001 From: Tim Seckinger Date: Sun, 14 Mar 2021 20:45:05 +0100 Subject: [PATCH 5/5] changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36ff4df5a7d4..d85e4b95dbd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,7 +64,6 @@ - `[jest-worker]` Handle `ERR_IPC_CHANNEL_CLOSED` errors properly ([#11143](https://github.com/facebook/jest/pull/11143)) - `[pretty-format]` [**BREAKING**] Convert to ES Modules ([#10515](https://github.com/facebook/jest/pull/10515)) - `[pretty-format]` Only call `hasAttribute` if it's a function ([#11000](https://github.com/facebook/jest/pull/11000)) -- `[jest-worker]` Fix memory leaks of jest-worker ([#11187](https://github.com/facebook/jest/pull/11187)) ### Chore & Maintenance @@ -97,6 +96,7 @@ - `[jest-resolve]` Cache reading and parsing of `package.json`s ([#11076](https://github.com/facebook/jest/pull/11076)) - `[jest-runtime]` Load `chalk` only once per worker ([#10864](https://github.com/facebook/jest/pull/10864)) +- `[jest-worker]` Fix memory leak of previous task arguments while no new task is scheduled ([#11187](https://github.com/facebook/jest/pull/11187)) ## 26.6.3