mirror of
https://github.com/oven-sh/bun
synced 2026-02-09 10:28:47 +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>
Tests
Finding tests
Tests are located in the test/ directory and are organized using the following structure:
test/js/- tests for JavaScript APIs.cli/- tests for commands, configs, and stdout.bundler/- tests for the transpiler/bundler.regression/- tests that reproduce a specific issue.harness.ts- utility functions that can be imported from any test.
The tests in test/js/ directory are further categorized by the type of API.
test/js/bun/- tests forBun-specific APIs.node/- tests for Node.js APIs.web/- tests for Web APIs, likefetch().first_party/- tests for npm packages that are built-in, likeundici.third_party/- tests for npm packages that are not built-in, but are popular, likeesbuild.
Running tests
To run a test, use Bun's built-in test command: bun test.
bun test # Run all tests
bun test js/bun # Only run tests in a directory
bun test sqlite.test.ts # Only run a specific test
If you encounter lots of errors, try running bun install, then trying again.
Writing tests
Tests are written in TypeScript (preferred) or JavaScript using Jest's describe(), test(), and expect() APIs.
import { describe, test, expect } from "bun:test";
import { gcTick } from "harness";
describe("TextEncoder", () => {
test("can encode a string", async () => {
const encoder = new TextEncoder();
const actual = encoder.encode("bun");
await gcTick();
expect(actual).toBe(new Uint8Array([0x62, 0x75, 0x6E]));
});
});
If you are fixing a bug that was reported from a GitHub issue, remember to add a test in the test/regression/ directory.
// test/regression/issue/02005.test.ts
import { it, expect } from "bun:test";
it("regex literal should work with non-latin1", () => {
const text = "这是一段要替换的文字";
expect(text.replace(new RegExp("要替换"), "")).toBe("这是一段的文字");
expect(text.replace(/要替换/, "")).toBe("这是一段的文字");
});
In the future, a bot will automatically close or re-open issues when a regression is detected or resolved.
Zig tests
These tests live in various .zig files throughout Bun's codebase, leveraging Zig's builtin test keyword.
Currently, they're not run automatically nor is there a simple way to run all of them. We will make this better soon.
TypeScript
Test files should be written in TypeScript. The types in packages/bun-types should be updated to support all new APIs. Changes to the .d.ts files in packages/bun-types will be immediately reflected in test files; no build step is necessary.
Writing a test will often require using invalid syntax, e.g. when checking for errors when an invalid input is passed to a function. TypeScript provides a number of escape hatches here.
// @ts-expect-error- This should be your first choice. It tells TypeScript that the next line should fail typechecking.// @ts-ignore- Ignore the next line entirely.// @ts-nocheck- Put this at the top of the file to disable typechecking on the entire file. Useful for autogenerated test files, or when ignoring/disabling type checks an a per-line basis is too onerous.