Skip to content

Commit 3850323

Browse files
sheremet-vasheremet-va
authored
fix(vm): stop retaining every finished test file in vm pool workers (#10854)
1 parent 62c795d commit 3850323

25 files changed

Lines changed: 483 additions & 103 deletions

File tree

‎packages/browser/src/client/client.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,14 @@ export const ENTRY_URL: string = `${
2323

2424
const onCancelCallbacks: ((reason: CancelReason) => void)[] = []
2525

26-
export function onCancel(callback: (reason: CancelReason) => void): void {
26+
export function onCancel(callback: (reason: CancelReason) => void): () => void {
2727
onCancelCallbacks.push(callback)
28+
return () => {
29+
const index = onCancelCallbacks.indexOf(callback)
30+
if (index !== -1) {
31+
onCancelCallbacks.splice(index, 1)
32+
}
33+
}
2834
}
2935

3036
let pageMarkHandler: ((name: string, options?: MarkOptions) => Promise<void>) | null = null

‎packages/expect/src/state.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,11 @@ if (!Object.hasOwn(globalThis, MATCHERS_OBJECT)) {
1111
const matchers = Object.create(null)
1212
const customEqualityTesters: Array<Tester> = []
1313
const asymmetricMatchers = Object.create(null)
14+
// `configurable` so that vm pools can strip the accessors from a disposed
15+
// context: the getters capture the expect state, which would otherwise keep
16+
// the whole test-file world reachable from the leaked context shell
1417
Object.defineProperty(globalThis, MATCHERS_OBJECT, {
18+
configurable: true,
1519
get: () => globalState,
1620
})
1721
Object.defineProperty(globalThis, JEST_MATCHERS_OBJECT, {
@@ -23,6 +27,7 @@ if (!Object.hasOwn(globalThis, MATCHERS_OBJECT)) {
2327
}),
2428
})
2529
Object.defineProperty(globalThis, ASYMMETRIC_MATCHERS_OBJECT, {
30+
configurable: true,
2631
get: () => asymmetricMatchers,
2732
})
2833
}

‎packages/vitest/src/runtime/external-executor.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { lookupPackageScopeType } from '@vitest/utils/resolver'
1111
import { extname, normalize } from 'pathe'
1212
import { CommonjsExecutor } from './vm/commonjs-executor'
1313
import { EsmExecutor } from './vm/esm-executor'
14+
import { setActiveVmExecutor } from './vm/utils'
1415
import { ViteExecutor } from './vm/vite-executor'
1516

1617
const { existsSync } = fs
@@ -72,9 +73,9 @@ export class ExternalModulesExecutor {
7273
this.esm = new EsmExecutor(this, {
7374
context: this.context,
7475
})
76+
setActiveVmExecutor(this)
7577
this.cjs = new CommonjsExecutor({
7678
context: this.context,
77-
importModuleDynamically: this.importModuleDynamically,
7879
fileMap: options.fileMap,
7980
codeCache: options.codeCache,
8081
interopDefault: options.interopDefault,

‎packages/vitest/src/runtime/rpc.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,14 @@ export async function rpcDone(): Promise<unknown[] | undefined> {
6363

6464
const onCancelCallbacks: ((reason: CancelReason) => void)[] = []
6565

66-
export function onCancel(callback: (reason: CancelReason) => void): void {
66+
export function onCancel(callback: (reason: CancelReason) => void): () => void {
6767
onCancelCallbacks.push(callback)
68+
return () => {
69+
const index = onCancelCallbacks.indexOf(callback)
70+
if (index !== -1) {
71+
onCancelCallbacks.splice(index, 1)
72+
}
73+
}
6874
}
6975

7076
export function createRuntimeRpc(

‎packages/vitest/src/runtime/runBaseTests.ts‎

Lines changed: 40 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -40,53 +40,58 @@ export async function run(
4040
}),
4141
])
4242

43-
workerState.onCancel((reason) => {
43+
const offCancel = workerState.onCancel((reason) => {
4444
closeInspector(config)
4545
testRunner.cancel?.(reason)
4646
})
4747

4848
workerState.durations.prepare = performance.now() - workerState.durations.prepare
49-
await traces.$(
50-
`vitest.test.runner.${method}`,
51-
async () => {
52-
for (const file of files) {
53-
if (config.isolate) {
54-
moduleRunner.mocker?.reset()
55-
resetModules(workerState.evaluatedModules, true)
56-
}
49+
try {
50+
await traces.$(
51+
`vitest.test.runner.${method}`,
52+
async () => {
53+
for (const file of files) {
54+
if (config.isolate) {
55+
moduleRunner.mocker?.reset()
56+
resetModules(workerState.evaluatedModules, true)
57+
}
5758

58-
workerState.filepath = file.filepath
59+
workerState.filepath = file.filepath
5960

60-
if (method === 'run') {
61-
const collectAsyncLeaks = config.detectAsyncLeaks ? detectAsyncLeaks(file.filepath, workerState.ctx.projectName) : undefined
61+
if (method === 'run') {
62+
const collectAsyncLeaks = config.detectAsyncLeaks ? detectAsyncLeaks(file.filepath, workerState.ctx.projectName) : undefined
6263

63-
await traces.$(
64-
`vitest.test.runner.${method}.module`,
65-
{ attributes: { 'code.file.path': file.filepath } },
66-
() => startTests([file], testRunner),
67-
)
64+
await traces.$(
65+
`vitest.test.runner.${method}.module`,
66+
{ attributes: { 'code.file.path': file.filepath } },
67+
() => startTests([file], testRunner),
68+
)
6869

69-
const leaks = await collectAsyncLeaks?.()
70+
const leaks = await collectAsyncLeaks?.()
7071

71-
if (leaks?.length) {
72-
workerState.rpc.onAsyncLeaks(leaks)
72+
if (leaks?.length) {
73+
workerState.rpc.onAsyncLeaks(leaks)
74+
}
75+
}
76+
else {
77+
await traces.$(
78+
`vitest.test.runner.${method}.module`,
79+
{ attributes: { 'code.file.path': file.filepath } },
80+
() => collectTests([file], testRunner),
81+
)
7382
}
74-
}
75-
else {
76-
await traces.$(
77-
`vitest.test.runner.${method}.module`,
78-
{ attributes: { 'code.file.path': file.filepath } },
79-
() => collectTests([file], testRunner),
80-
)
81-
}
8283

83-
// reset after tests, because user might call `vi.setConfig` in setupFile
84-
vi.resetConfig()
85-
// mocks should not affect different files
86-
vi.restoreAllMocks()
87-
}
88-
},
89-
)
84+
// reset after tests, because user might call `vi.setConfig` in setupFile
85+
vi.resetConfig()
86+
// mocks should not affect different files
87+
vi.restoreAllMocks()
88+
}
89+
},
90+
)
91+
}
92+
finally {
93+
offCancel()
94+
}
9095

9196
await traces.$('vitest.runtime.coverage.stop', () => stopCoverageInsideWorker(config.coverage, moduleRunner, { isolate: config.isolate }))
9297
}

‎packages/vitest/src/runtime/runVmTests.ts‎

Lines changed: 41 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ export async function run(
3636
Object.defineProperty(globalThis, '__vitest_index__', {
3737
value: VitestIndex,
3838
enumerable: false,
39+
configurable: true,
40+
writable: true,
3941
})
4042

4143
const viteEnvironment = workerState.environment.viteEnvironment || workerState.environment.name
@@ -79,7 +81,10 @@ export async function run(
7981

8082
config.snapshotOptions.snapshotEnvironment = snapshotEnvironment
8183

82-
workerState.onCancel((reason) => {
84+
// the callback captures this file's runner: unsubscribe once the run is
85+
// over, or every finished file's world stays reachable from the worker's
86+
// cancel listeners for the lifetime of the worker
87+
const offCancel = workerState.onCancel((reason) => {
8388
closeInspector(config)
8489
testRunner.cancel?.(reason)
8590
})
@@ -89,42 +94,47 @@ export async function run(
8994

9095
const { vi } = VitestIndex
9196

92-
await traces.$(
93-
`vitest.test.runner.${method}`,
94-
async () => {
95-
for (const file of files) {
96-
workerState.filepath = file.filepath
97+
try {
98+
await traces.$(
99+
`vitest.test.runner.${method}`,
100+
async () => {
101+
for (const file of files) {
102+
workerState.filepath = file.filepath
97103

98-
if (method === 'run') {
99-
const collectAsyncLeaks = config.detectAsyncLeaks ? detectAsyncLeaks(file.filepath, workerState.ctx.projectName) : undefined
104+
if (method === 'run') {
105+
const collectAsyncLeaks = config.detectAsyncLeaks ? detectAsyncLeaks(file.filepath, workerState.ctx.projectName) : undefined
100106

101-
await traces.$(
102-
`vitest.test.runner.${method}.module`,
103-
{ attributes: { 'code.file.path': file.filepath } },
104-
() => startTests([file], testRunner),
105-
)
107+
await traces.$(
108+
`vitest.test.runner.${method}.module`,
109+
{ attributes: { 'code.file.path': file.filepath } },
110+
() => startTests([file], testRunner),
111+
)
106112

107-
const leaks = await collectAsyncLeaks?.()
113+
const leaks = await collectAsyncLeaks?.()
108114

109-
if (leaks?.length) {
110-
workerState.rpc.onAsyncLeaks(leaks)
115+
if (leaks?.length) {
116+
workerState.rpc.onAsyncLeaks(leaks)
117+
}
118+
}
119+
else {
120+
await traces.$(
121+
`vitest.test.runner.${method}.module`,
122+
{ attributes: { 'code.file.path': file.filepath } },
123+
() => collectTests([file], testRunner),
124+
)
111125
}
112-
}
113-
else {
114-
await traces.$(
115-
`vitest.test.runner.${method}.module`,
116-
{ attributes: { 'code.file.path': file.filepath } },
117-
() => collectTests([file], testRunner),
118-
)
119-
}
120126

121-
// reset after tests, because user might call `vi.setConfig` in setupFile
122-
vi.resetConfig()
123-
// mocks should not affect different files
124-
vi.restoreAllMocks()
125-
}
126-
},
127-
)
127+
// reset after tests, because user might call `vi.setConfig` in setupFile
128+
vi.resetConfig()
129+
// mocks should not affect different files
130+
vi.restoreAllMocks()
131+
}
132+
},
133+
)
134+
}
135+
finally {
136+
offCancel()
137+
}
128138

129139
await traces.$('vitest.runtime.coverage.stop', () => stopCoverageInsideWorker(config.coverage, moduleRunner, { isolate: false }))
130140
}

‎packages/vitest/src/runtime/runners/test.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ export class TestRunner implements VitestTestRunner {
5454
const environment = this.workerState.environment
5555
this.viteEnvironment = environment.viteEnvironment || environment.name
5656
this.viteModuleRunner = config.experimental.viteModuleRunner
57+
// vm pools downgrade worker-scoped fixtures to file scope, so the hook has
58+
// nothing to tear down there; registering it anyway would keep the
59+
// listener, an in-context closure, alive for the lifetime of the worker
60+
if (this.pool !== 'vmThreads' && this.pool !== 'vmForks') {
61+
this.onCleanupWorkerContext = listener => this.workerState.onCleanup(listener)
62+
}
5763
}
5864

5965
importFile(filepath: string, source: VitestRunnerImportSource): unknown {
@@ -81,9 +87,7 @@ export class TestRunner implements VitestTestRunner {
8187
this.workerState.current = file
8288
}
8389

84-
onCleanupWorkerContext(listener: () => unknown): void {
85-
this.workerState.onCleanup(listener)
86-
}
90+
onCleanupWorkerContext?: (listener: () => unknown) => void
8791

8892
onAfterRunFiles(_files: File[]): void {
8993
this.snapshotClient.clear()

‎packages/vitest/src/runtime/vm/commonjs-executor.ts‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,16 @@
11
import type { CodeCache } from './code-cache'
22
import type { FileMap } from './file-map'
3-
import type { ImportModuleDynamically, VMSyntheticModule } from './types'
3+
import type { VMSyntheticModule } from './types'
44
import { Module as _Module, createRequire, isBuiltin } from 'node:module'
55
import vm from 'node:vm'
66
import { basename, dirname, extname } from 'pathe'
7-
import { interopCommonJsModule, SyntheticModule } from './utils'
7+
import { activeImportModuleDynamically, interopCommonJsModule, SyntheticModule } from './utils'
88

99
interface CommonjsExecutorOptions {
1010
fileMap: FileMap
1111
codeCache?: CodeCache
1212
interopDefault?: boolean
1313
context: vm.Context
14-
importModuleDynamically: ImportModuleDynamically
1514
}
1615

1716
const _require = createRequire(import.meta.url)
@@ -22,6 +21,11 @@ interface PrivateNodeModule extends NodeJS.Module {
2221

2322
const requiresCache = new WeakMap<NodeJS.Module, NodeJS.Require>()
2423

24+
// Compiled scripts of commonjs modules, shared across vm contexts: only the
25+
// evaluation has to happen per context. No invalidation is needed because
26+
// watch mode reruns destroy the worker.
27+
const cjsScriptCache = new Map<string, vm.Script>()
28+
2529
export class CommonjsExecutor {
2630
private context: vm.Context
2731
private requireCache = new Map<string, NodeJS.Module>()
@@ -114,17 +118,24 @@ export class CommonjsExecutor {
114118
_compile(code: string, filename: string) {
115119
const cjsModule = Module.wrap(code)
116120
const codeCache = executor.codeCache
117-
const cachedData = codeCache?.get(filename, cjsModule)
118-
const script = new vm.Script(cjsModule, {
119-
filename,
120-
cachedData,
121-
importModuleDynamically: options.importModuleDynamically,
122-
} as any)
123-
if (cachedData && script.cachedDataRejected) {
124-
codeCache!.delete(filename)
121+
let script = cjsScriptCache.get(filename)
122+
if (!script) {
123+
const cachedData = codeCache?.get(filename, cjsModule)
124+
// the dynamic import callback is a static function (the executor is
125+
// resolved when it is called), so the compiled script holds no
126+
// per-context state and can be reused by every vm context
127+
script = new vm.Script(cjsModule, {
128+
filename,
129+
cachedData,
130+
importModuleDynamically: activeImportModuleDynamically,
131+
} as any)
132+
if (cachedData && script.cachedDataRejected) {
133+
codeCache!.delete(filename)
134+
}
135+
// @ts-expect-error mark script with current identifier
136+
script.identifier = filename
137+
cjsScriptCache.set(filename, script)
125138
}
126-
// @ts-expect-error mark script with current identifier
127-
script.identifier = filename
128139
const fn = script.runInContext(executor.context)
129140
const __dirname = dirname(filename)
130141
executor.requireCache.set(filename, this)

0 commit comments

Comments
 (0)
Sponsor
SponsoredKunjungi sekarang
Promo