Repository navigation
Bun.serve: do not idle-time-out a response the peer is still reading #41602
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
6c65bbc
57bf013
bb25e5e
98535ba
63b92f6
000634e
4303d93
e698b60
d92a750
c923dbb
8fc1424
9584615
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { afterAll, beforeAll, beforeEach, describe, expect, test } from "bun:test"; | ||
| import { randomBytes } from "crypto"; | ||
| import { bunEnv, bunExe, tempDir, tls as tlsCert } from "harness"; | ||
| import { bunEnv, bunExe, isWindows, tempDir, tls as tlsCert } from "harness"; | ||
| import http2 from "node:http2"; | ||
| import net from "node:net"; | ||
| import { join } from "node:path"; | ||
|
|
@@ -855,4 +855,59 @@ describe("Bun.serve http2 in-process", () => { | |
| expect(aborted).toBe(1); | ||
| await new Promise<void>(r => session.close(() => r())); | ||
| }); | ||
|
|
||
| // The peer opens a window far larger than the kernel send buffer, so the TCP | ||
| // send buffer throttles the response, like an HTTP/1.1 client reading slowly. | ||
| // No write and no writable event re-arms the idle timer while the buffer | ||
| // stays full, yet the peer keeps taking bytes and must not be aborted. The | ||
| // client reads at a fixed low rate for longer than one idle period; the body | ||
| // is larger than the window so it never completes here, surviving the window | ||
| // is the point. Skipped on Windows (the buffered remainder gets no writable | ||
| // event there while the peer reads slowly, a separate Windows issue). | ||
| test.skipIf(isWindows)( | ||
| "idleTimeout keeps a slow reader alive", | ||
| async () => { | ||
| const TOTAL = 64 << 20; | ||
| const IDLE_S = 8; | ||
| const RATE = 128 * 1024; // bytes/sec, below the writable-event rate, above the 16 KB/s floor | ||
| let aborted = false; | ||
| await using server = Bun.serve({ | ||
| hostname: "127.0.0.1", | ||
| port: 0, | ||
| http2: true, | ||
| idleTimeout: IDLE_S, | ||
| fetch(req) { | ||
| req.signal.addEventListener("abort", () => (aborted = true)); | ||
| return new Response(new Uint8Array(TOTAL)); | ||
| }, | ||
| }); | ||
| const settings = Buffer.alloc(6); | ||
| settings.writeUInt16BE(4, 0); // INITIAL_WINDOW_SIZE | ||
| settings.writeUInt32BE(64 << 20, 2); | ||
| const raw = await RawH2.connect(server.port, false, { settings }); | ||
| const inc = Buffer.alloc(4); | ||
| inc.writeUInt32BE(64 << 20, 0); | ||
| raw.write(frame(T.WINDOW_UPDATE, 0, 0, inc)); | ||
| const RUN_MS = IDLE_S * 1000 + 8_000; | ||
| const start = performance.now(); | ||
| let received = 0; | ||
|
claude[bot] marked this conversation as resolved.
Resolved
|
||
| raw.socket.on("data", (d: Buffer) => { | ||
| received += d.length; | ||
| if (performance.now() - start < RUN_MS) { | ||
| raw.socket.pause(); | ||
| setTimeout(() => raw.socket.resume(), Math.max(1, Math.round((d.length / RATE) * 1000))); | ||
| } | ||
| }); | ||
| raw.headers(1, baseHeaders("/")); | ||
| const { promise, resolve } = Promise.withResolvers<void>(); | ||
| raw.socket.on("close", () => resolve()); // the server aborting fires this early | ||
| const deadline = setTimeout(() => resolve(), RUN_MS + 2_000); | ||
| await promise; | ||
| clearTimeout(deadline); | ||
| const gotData = raw.frames.some(f => f.type === T.DATA && f.streamId === 1); | ||
| expect({ aborted, gotData }).toEqual({ aborted: false, gotData: true }); | ||
| raw.close(); | ||
| }, | ||
| 30_000, | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) The HTTP/2 side gets only the positive slow-reader case; the negative contract — a fully-stalled peer is still aborted within one period — is tested for HTTP/1 (serve.test.ts "still times out a reader that stops") but not here, even though Extended reasoning...REVIEW.md: "Cover the variant matrix, not just the repro … the negative contract" and "Fix the whole class in the same PR … HTTP/1 and HTTP/2." Verification: nit — The claim is accurate.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The stalled-peer contract is shared code: both HttpContext::onTimeout and Http2Context::onTimeout call the same us_socket_send_progressed with the same floor, and the HTTP/1 case "still times out a reader that stops" exercises it. An h2 twin would add another 16 s plus case for the same function. Leaving this one open for a maintainer to weigh. |
||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.