mirror of
https://github.com/oven-sh/bun
synced 2026-02-09 18:38:55 +00:00
370e6fb9fa
## Summary
This PR fixes a memory leak that occurs when `fetch()` is called with a
`ReadableStream` body. The ReadableStream objects were not being
properly released, causing them to accumulate in memory.
## Problem
When using `fetch()` with a ReadableStream body:
```javascript
const stream = new ReadableStream({
start(controller) {
controller.enqueue(new TextEncoder().encode("data"));
controller.close();
}
});
await fetch(url, { method: "POST", body: stream });
```
The ReadableStream objects leak because `FetchTasklet.clearData()` has a
conditional that prevents `detach()` from being called on ReadableStream
request bodies after streaming has started.
### Root Cause
The problematic condition in `clearData()`:
```zig
if (this.request_body != .ReadableStream or this.is_waiting_request_stream_start) {
this.request_body.detach();
}
```
After `startRequestStream()` is called:
- `is_waiting_request_stream_start` becomes `false`
- `request_body` is still `.ReadableStream`
- The condition evaluates to `(false or false) = false`
- `detach()` is skipped → **memory leak**
### Why the Original Code Was Wrong
The original code appears to assume that when `startRequestStream()` is
called, ownership of the Strong reference is transferred to
`ResumableSink`. However, this is incorrect:
1. `startRequestStream()` creates a **new independent** Strong reference
in `ResumableSink` (see `ResumableSink.zig:119`)
2. The FetchTasklet's original reference is **not transferred** - it
becomes redundant
3. Strong references in Bun are independent - calling `deinit()` on one
does not affect the other
## Solution
Remove the conditional and always call `detach()`:
```zig
// Always detach request_body regardless of type.
// When request_body is a ReadableStream, startRequestStream() creates
// an independent Strong reference in ResumableSink, so FetchTasklet's
// reference becomes redundant and must be released to avoid leaks.
this.request_body.detach();
```
### Safety Analysis
This change is safe because:
1. **Strong references are independent**: Each Strong reference
maintains its own ref count. Detaching FetchTasklet's reference doesn't
affect ResumableSink's reference
2. **Idempotency**: `detach()` is safe to call on already-detached
references
3. **Timing**: `clearData()` is only called from `deinit()` after
streaming has completed (ref_count = 0)
4. **No UAF risk**: `deinit()` only runs when ref_count reaches 0, which
means all streaming operations have completed
## Test Results
Before fix (with system Bun):
```
Expected: <= 100
Received: 501 (Request objects leaked)
Received: 1002 (ReadableStream objects leaked)
```
After fix:
```
6 pass
0 fail
```
## Test Coverage
Added comprehensive tests in
`test/js/web/fetch/fetch-cyclic-reference.test.ts` covering:
- Response stream leaks with cyclic references
- Streaming response body leaks
- Request body stream leaks with cyclic references
- ReadableStream body leaks (no cyclic reference needed to reproduce)
- Concurrent fetch operations with cyclic references
---------
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
Co-authored-by: Claude Bot <claude-bot@bun.sh>
Co-authored-by: Claude <noreply@anthropic.com>
95 lines
2.5 KiB
TypeScript
95 lines
2.5 KiB
TypeScript
import { heapStats } from "bun:jsc";
|
|
import { describe, expect, test } from "bun:test";
|
|
|
|
describe("FetchTasklet cyclic reference", () => {
|
|
test("fetch with request body stream should not leak with cyclic reference", async () => {
|
|
await using server = Bun.serve({
|
|
port: 0,
|
|
async fetch(req) {
|
|
const body = await req.text();
|
|
return new Response(`received: ${body}`);
|
|
},
|
|
});
|
|
|
|
const url = `http://localhost:${server.port}/`;
|
|
|
|
async function leak() {
|
|
const requestBody = new ReadableStream({
|
|
start(controller) {
|
|
controller.enqueue(new TextEncoder().encode("request body"));
|
|
controller.close();
|
|
},
|
|
});
|
|
|
|
const request = new Request(url, {
|
|
method: "POST",
|
|
body: requestBody,
|
|
});
|
|
|
|
// Create cyclic reference
|
|
// @ts-ignore
|
|
requestBody.request = request;
|
|
// @ts-ignore
|
|
request.bodyStream = requestBody;
|
|
|
|
const response = await fetch(request);
|
|
return await response.text();
|
|
}
|
|
|
|
for (let i = 0; i < 500; i++) {
|
|
await leak();
|
|
}
|
|
|
|
await Bun.sleep(10);
|
|
Bun.gc(true);
|
|
await Bun.sleep(10);
|
|
Bun.gc(true);
|
|
|
|
const requestCount = heapStats().objectTypeCounts.Request || 0;
|
|
const readableStreamCount = heapStats().objectTypeCounts.ReadableStream || 0;
|
|
expect(requestCount).toBeLessThanOrEqual(100);
|
|
expect(readableStreamCount).toBeLessThanOrEqual(100);
|
|
});
|
|
|
|
test("fetch with ReadableStream body should not leak streams", async () => {
|
|
await using server = Bun.serve({
|
|
port: 0,
|
|
async fetch(req) {
|
|
const body = await req.text();
|
|
return new Response(`received: ${body}`);
|
|
},
|
|
});
|
|
|
|
const url = `http://localhost:${server.port}/`;
|
|
|
|
async function leak() {
|
|
const requestBody = new ReadableStream({
|
|
start(controller) {
|
|
controller.enqueue(new TextEncoder().encode("request body"));
|
|
controller.close();
|
|
},
|
|
});
|
|
|
|
// Use ReadableStream directly with fetch, no Request object, no cyclic reference
|
|
const response = await fetch(url, {
|
|
method: "POST",
|
|
body: requestBody,
|
|
});
|
|
return await response.text();
|
|
}
|
|
|
|
for (let i = 0; i < 500; i++) {
|
|
await leak();
|
|
}
|
|
|
|
await Bun.sleep(10);
|
|
Bun.gc(true);
|
|
await Bun.sleep(10);
|
|
Bun.gc(true);
|
|
|
|
const readableStreamCount = heapStats().objectTypeCounts.ReadableStream || 0;
|
|
// This currently fails with ~502 streams leaked
|
|
expect(readableStreamCount).toBeLessThanOrEqual(100);
|
|
});
|
|
});
|