Skip to content

Commit c4567d2

Browse files
authored
fix: prevent frame abort listener leak (#2737)
1 parent 4bc4a5c commit c4567d2

7 files changed

Lines changed: 215 additions & 29 deletions

File tree

.github/workflows/ci.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,9 @@ jobs:
146146
- name: Node.js tests
147147
run: pnpm test:node
148148

149+
- name: Memory tests
150+
run: pnpm test:memory
151+
149152
test-node-22:
150153
name: test (node.js) (22)
151154
needs: build-22
@@ -179,6 +182,9 @@ jobs:
179182
- name: Node.js tests
180183
run: pnpm test:node
181184

185+
- name: Memory tests
186+
run: pnpm test:memory
187+
182188
test-e2e:
183189
name: test (e2e) (20)
184190
needs: build-20

package.json

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@
183183
"build": "pnpm clean && cross-env NODE_ENV=production tsup && pnpm patch:dts",
184184
"patch:dts": "node \"./config/scripts/patch-ts.js\"",
185185
"publint": "publint",
186-
"test": "pnpm test:unit && pnpm test:node && pnpm test:browser && pnpm test:native",
186+
"test": "pnpm test:unit && pnpm test:node && pnpm test:browser && pnpm test:native && pnpm test:memory",
187187
"test:unit": "vitest",
188188
"test:node": "vitest --config=./test/node/vitest.config.ts",
189189
"test:native": "vitest --config=./test/native/vitest.config.ts",
@@ -192,6 +192,7 @@
192192
"test:modules:browser": "playwright test -c ./test/modules/browser/playwright.config.ts",
193193
"test:e2e": "vitest run --config=./test/e2e/vitest.config.ts",
194194
"test:ts": "vitest --config=./test/typings/vitest.config.ts",
195+
"test:memory": "vitest --config=./test/memory/vitest.config.ts",
195196
"prepare": "pnpm simple-git-hooks init",
196197
"prepack": "pnpm build",
197198
"release": "release publish",
@@ -256,7 +257,7 @@
256257
"outvariant": "^1.4.3",
257258
"path-to-regexp": "^6.3.0",
258259
"picocolors": "^1.1.1",
259-
"rettime": "^0.11.7",
260+
"rettime": "^0.11.11",
260261
"statuses": "^2.0.2",
261262
"strict-event-emitter": "^0.5.1",
262263
"tough-cookie": "^6.0.1",

pnpm-lock.yaml

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/browser/sources/service-worker-source.ts

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,7 @@ Please consider using a custom "serviceWorker.url" option to point to the actual
330330

331331
async #handleResponse(event: WorkerChannelResponseEvent): Promise<void> {
332332
const { request, response, isMockedResponse } = event.data
333+
const frame = this.#frames.get(request.id)
333334

334335
/**
335336
* CORS requests with `mode: "no-cors"` result in "opaque" responses.
@@ -340,10 +341,10 @@ Please consider using a custom "serviceWorker.url" option to point to the actual
340341
*/
341342
if (response.type?.includes('opaque')) {
342343
this.#frames.delete(request.id)
344+
frame?.events.removeAllListeners()
343345
return
344346
}
345347

346-
const frame = this.#frames.get(request.id)
347348
this.#frames.delete(request.id)
348349

349350
/**
@@ -379,17 +380,21 @@ Please consider using a custom "serviceWorker.url" option to point to the actual
379380
},
380381
)
381382

382-
frame.events.emit(
383-
new ResponseEvent(
384-
isMockedResponse ? 'response:mocked' : 'response:bypass',
385-
{
386-
requestId: frame.data.id,
387-
request: fetchRequest,
388-
response: fetchResponse,
389-
isMockedResponse,
390-
},
391-
),
392-
)
383+
try {
384+
frame.events.emit(
385+
new ResponseEvent(
386+
isMockedResponse ? 'response:mocked' : 'response:bypass',
387+
{
388+
requestId: frame.data.id,
389+
request: fetchRequest,
390+
response: fetchResponse,
391+
isMockedResponse,
392+
},
393+
),
394+
)
395+
} finally {
396+
frame.events.removeAllListeners()
397+
}
393398
}
394399

395400
#defaultFindWorker: FindWorker = (workerUrl, mockServiceWorkerUrl) => {

src/core/experimental/sources/interceptor-source.ts

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -87,16 +87,26 @@ export class InterceptorSource extends NetworkSource {
8787
}
8888

8989
queueMicrotask(() => {
90-
httpFrame.events.emit(
91-
new ResponseEvent(
92-
isMockedResponse ? 'response:mocked' : 'response:bypass',
93-
{
94-
requestId,
95-
request,
96-
response,
97-
},
98-
),
99-
)
90+
try {
91+
httpFrame.events.emit(
92+
new ResponseEvent(
93+
isMockedResponse ? 'response:mocked' : 'response:bypass',
94+
{
95+
requestId,
96+
request,
97+
response,
98+
},
99+
),
100+
)
101+
} finally {
102+
/**
103+
* @note Remove any listeners from this frame.
104+
* Past this point, it won't emit anything. The removal is crucial
105+
* to prevent "rettime" from keeping the abort cleanup listeners internally.
106+
* @see https://github.com/mswjs/msw/issues/2735
107+
*/
108+
httpFrame.events.removeAllListeners()
109+
}
100110
})
101111
}
102112

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
// @vitest-environment node
2+
/**
3+
* @see https://github.com/mswjs/msw/issues/2735
4+
*/
5+
import * as fs from 'node:fs'
6+
import * as os from 'node:os'
7+
import * as path from 'node:path'
8+
import { setTimeout as delay } from 'node:timers/promises'
9+
import { writeHeapSnapshot } from 'node:v8'
10+
import { http } from 'msw'
11+
import { setupServer } from 'msw/node'
12+
13+
const TOTAL_REQUESTS = 5_000
14+
const CONCURRENCY = 20
15+
const URL = 'https://localhost/leak'
16+
17+
async function fireBatch(count: number, concurrency: number): Promise<void> {
18+
let inflight = 0
19+
let started = 0
20+
let failed = false
21+
22+
return new Promise((resolve, reject) => {
23+
const next = () => {
24+
if (failed) {
25+
return
26+
}
27+
if (started >= count && inflight === 0) {
28+
return resolve()
29+
}
30+
31+
while (inflight < concurrency && started < count) {
32+
started++
33+
inflight++
34+
fetch(URL)
35+
.then((r) => r.text())
36+
.catch((error) => {
37+
failed = true
38+
reject(error)
39+
})
40+
.finally(() => {
41+
inflight--
42+
next()
43+
})
44+
}
45+
}
46+
next()
47+
})
48+
}
49+
50+
async function forceGc(): Promise<void> {
51+
for (let i = 0; i < 6; i++) {
52+
global.gc?.()
53+
await delay(30)
54+
}
55+
}
56+
57+
function countConstructors(snapPath: string): Map<string, number> {
58+
const json = JSON.parse(fs.readFileSync(snapPath, 'utf8'))
59+
const meta = json.snapshot.meta
60+
const F = meta.node_fields.length
61+
const fT = meta.node_fields.indexOf('type')
62+
const fN = meta.node_fields.indexOf('name')
63+
const objIdx = meta.node_types[0].indexOf('object')
64+
const counts = new Map<string, number>()
65+
66+
for (let i = 0; i < json.snapshot.node_count; i++) {
67+
const b = i * F
68+
if (json.nodes[b + fT] !== objIdx) {
69+
continue
70+
}
71+
const name = json.strings[json.nodes[b + fN]]
72+
counts.set(name, (counts.get(name) ?? 0) + 1)
73+
}
74+
75+
return counts
76+
}
77+
78+
const server = setupServer()
79+
80+
beforeAll(() => {
81+
server.listen()
82+
})
83+
84+
afterAll(() => {
85+
server.close()
86+
})
87+
88+
it('does not retain a per-request Emitter after the response is delivered', async () => {
89+
server.use(
90+
http.get('https://localhost/leak', () => {
91+
return new Response()
92+
}),
93+
)
94+
95+
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'msw-emitter-leak-'))
96+
97+
// Warm-up so JIT settles and we don't count startup allocation.
98+
await fireBatch(200, CONCURRENCY)
99+
await forceGc()
100+
101+
const baseSnap = path.join(tmp, 'baseline.heapsnapshot')
102+
writeHeapSnapshot(baseSnap)
103+
const baseCounts = countConstructors(baseSnap)
104+
105+
// The actual measurement burst.
106+
await fireBatch(TOTAL_REQUESTS, CONCURRENCY)
107+
await delay(2000)
108+
await forceGc()
109+
await delay(1000)
110+
await forceGc()
111+
112+
const settledSnap = path.join(tmp, 'settled.heapsnapshot')
113+
writeHeapSnapshot(settledSnap)
114+
const settledCounts = countConstructors(settledSnap)
115+
116+
const watch = [
117+
'Emitter',
118+
'Listener',
119+
'LensList',
120+
'WeakMap',
121+
'WeakSet',
122+
] as const
123+
console.log(`\nConstructor count after ${TOTAL_REQUESTS} requests:\n`)
124+
console.log(
125+
' ' +
126+
'name'.padEnd(12) +
127+
'baseline'.padStart(10) +
128+
'settled'.padStart(10) +
129+
'delta'.padStart(10),
130+
)
131+
console.log(' ' + '-'.repeat(42))
132+
for (const name of watch) {
133+
const b = baseCounts.get(name) ?? 0
134+
const s = settledCounts.get(name) ?? 0
135+
const d = s - b
136+
const dStr = (d >= 0 ? '+' : '') + d
137+
console.log(
138+
' ' +
139+
name.padEnd(12) +
140+
String(b).padStart(10) +
141+
String(s).padStart(10) +
142+
dStr.padStart(10),
143+
)
144+
}
145+
console.log(`\nHeap snapshots written to: ${tmp}\n`)
146+
147+
const emitterDelta =
148+
(settledCounts.get('Emitter') ?? 0) - (baseCounts.get('Emitter') ?? 0)
149+
150+
// After the fix, this delta is essentially 0. Today, it's ~= TOTAL.
151+
// Threshold: 5% of request count to allow for noise.
152+
expect(emitterDelta).toBeLessThan(TOTAL_REQUESTS * 0.05)
153+
})

test/memory/vitest.config.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import { defineConfig } from 'vitest/config'
2+
3+
export default defineConfig({
4+
test: {
5+
dir: './test/memory',
6+
globals: true,
7+
testTimeout: 120_000,
8+
pool: 'forks',
9+
execArgv: ['--expose-gc'],
10+
},
11+
})

0 commit comments

Comments
 (0)