-
Notifications
You must be signed in to change notification settings - Fork 4.7k
s3: use .temporary lifetime for downloaded body so it gets freed #29923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5279288
s3: use .temporary lifetime for downloaded body so it gets freed
robobun f353f4b
test(s3): address review on s3-body-leak test + text-leak fixture
robobun 1fdfa07
test(s3): make warmup match the measured phase to remove allocator gr…
robobun 6e12bd3
test(s3): make text-leak fixture sequential with symmetric warmup
robobun b76e5e6
Revert s3-text-leak-fixture.js changes
robobun 0c76897
test(s3): fix misleading comment about urlencoded parsing of the fixt…
robobun 802b91c
test(s3): filter benign ASAN startup warning from stderr
robobun 7463a13
ci: retrigger (previous build expired — all jobs timed out waiting fo…
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe } from "harness"; | ||
|
|
||
| // S3 file body methods (.text() / .json() / .arrayBuffer() / .bytes() / | ||
| // .formData()) must not leak the downloaded body. | ||
| // | ||
| // On a successful download, S3HttpSimpleTask moves ownership of its heap | ||
| // response_buffer into the S3DownloadResult passed to the callback. | ||
| // S3BlobDownloadTask.onS3DownloadResolved forwards the body's bytes to the | ||
| // to*WithBytes handler via doReadFromS3's wrapper. The wrapper must use the | ||
| // `.temporary` lifetime so the handler frees (or adopts) the allocation; the | ||
| // S3 Blob Store only holds path/credentials and cannot free it via `.clone`. | ||
| // | ||
| // With the leak, each call orphaned a buffer the size of the object. Downloading | ||
| // a 1 MiB object 50 times grew RSS by ~50 MiB and never released it. | ||
| // | ||
| // Uses a local Bun.serve as a mock S3 endpoint so this runs without credentials. | ||
|
|
||
| const fixture = /* js */ ` | ||
| const { S3Client } = require("bun"); | ||
|
|
||
| const method = process.env.S3_BODY_LEAK_METHOD; | ||
| const payloadSize = 1024 * 1024; // 1 MiB | ||
| const filler = Buffer.alloc(payloadSize - 8, "a").toString("latin1"); | ||
| // Valid JSON; also accepted by application/x-www-form-urlencoded parsing | ||
| // (no '=' present, so the whole body becomes a single key with empty value). | ||
| const body = '{"x":"' + filler + '"}'; | ||
|
|
||
| const server = Bun.serve({ | ||
| port: 0, | ||
| fetch() { | ||
| return new Response(body, { | ||
| status: 200, | ||
| headers: { "Content-Type": "application/json" }, | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| const s3 = new S3Client({ | ||
| accessKeyId: "test", | ||
| secretAccessKey: "test", | ||
| endpoint: "http://127.0.0.1:" + server.port, | ||
| bucket: "test", | ||
| }); | ||
|
|
||
| async function readOnce() { | ||
| switch (method) { | ||
| case "arrayBuffer": { | ||
| const v = await s3.file("key").arrayBuffer(); | ||
| if (v.byteLength !== body.length) throw new Error("bad arrayBuffer length: " + v.byteLength); | ||
| break; | ||
| } | ||
| case "bytes": { | ||
| const v = await s3.file("key").bytes(); | ||
| if (v.byteLength !== body.length) throw new Error("bad bytes length: " + v.byteLength); | ||
| break; | ||
| } | ||
| case "text": { | ||
| const v = await s3.file("key").text(); | ||
| if (v.length !== body.length) throw new Error("bad text length: " + v.length); | ||
| break; | ||
| } | ||
| case "json": { | ||
| const v = await s3.file("key").json(); | ||
| if (v.x.length !== body.length - 8) throw new Error("bad json length"); | ||
| break; | ||
| } | ||
| case "formData": { | ||
| const f = s3.file("key", { type: "application/x-www-form-urlencoded" }); | ||
| const v = await f.formData(); | ||
| // '{"x":"aaa..."}' parsed as urlencoded yields key '{"x":"aaa..."}' with value ''. | ||
| const [entry] = v.keys(); | ||
| if (entry.length !== body.length) throw new Error("bad formData length: " + entry.length); | ||
| break; | ||
| } | ||
| default: | ||
| throw new Error("unknown method: " + method); | ||
| } | ||
| } | ||
|
|
||
| async function settle() { | ||
| // External strings / ArrayBuffers need their finalizers to run before | ||
| // the backing allocation is released; cycle GC a few times. | ||
| for (let i = 0; i < 4; i++) { | ||
| Bun.gc(true); | ||
| await Bun.sleep(10); | ||
| } | ||
| } | ||
|
|
||
| const iterations = 50; | ||
|
|
||
| // Warmup runs the same number of iterations as the measured phase so that | ||
| // allocator segments, the HTTP connection pool, and JIT state are already | ||
| // sized for steady state when the baseline sample is taken. If the body is | ||
| // being freed, the measured phase reuses the same segments and growth is | ||
| // near zero; if it leaks, the measured phase adds another ~iterations MiB. | ||
| for (let i = 0; i < iterations; i++) await readOnce(); | ||
| await settle(); | ||
| const baseline = process.memoryUsage.rss(); | ||
|
|
||
| for (let i = 0; i < iterations; i++) await readOnce(); | ||
| await settle(); | ||
| const after = process.memoryUsage.rss(); | ||
|
|
||
| const growthMiB = (after - baseline) / 1024 / 1024; | ||
| process.stdout.write(JSON.stringify({ method, iterations, growthMiB })); | ||
| server.stop(true); | ||
| `; | ||
|
|
||
| describe.concurrent("s3 body methods do not leak the downloaded buffer", () => { | ||
| const env = { | ||
| ...bunEnv, | ||
| // S3 download picks up HTTP(S)_PROXY from the environment without | ||
| // checking NO_PROXY for the endpoint host. Clear them so the request | ||
| // goes straight to our local mock server. | ||
| HTTP_PROXY: undefined, | ||
| http_proxy: undefined, | ||
| HTTPS_PROXY: undefined, | ||
| https_proxy: undefined, | ||
| }; | ||
|
|
||
| for (const method of ["arrayBuffer", "bytes", "text", "json", "formData"] as const) { | ||
| test( | ||
| method, | ||
| async () => { | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "--smol", "-e", fixture], | ||
| env: { ...env, S3_BODY_LEAK_METHOD: method }, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| const filteredStderr = stderr | ||
| .split("\n") | ||
| .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) | ||
| .join("\n"); | ||
| expect(filteredStderr).toBe(""); | ||
| const { iterations, growthMiB } = JSON.parse(stdout); | ||
| // When leaking, growth is ~`iterations` MiB (one 1 MiB buffer per call). | ||
| // When fixed, the buffer is reused by the allocator and growth stays | ||
| // near zero. Allow generous slack for allocator / GC noise. | ||
| expect(growthMiB).toBeLessThan(iterations / 2); | ||
| expect(exitCode).toBe(0); | ||
| }, | ||
| 60_000, | ||
| ); | ||
|
robobun marked this conversation as resolved.
|
||
| } | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.