-
Notifications
You must be signed in to change notification settings - Fork 5.1k
http3: stop one dead peer from stalling the shared fetch client engine #35255
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
88a6b74
http3: disable IP_RECVERR on QUIC UDP sockets and fix lsquic requeue …
robobun 9536286
gate IP_RECVERR on recv_error_cb; hook quic packets_out into the faul…
robobun 34f4fbc
review: clear LIBUS_UDP_LINUX_RECVERR when recv_error_cb is NULL; can…
robobun d020497
test: withServer pattern in fetch-http3-syscall-fault; drop per-test …
robobun 6ee72b4
[autofix.ci] apply automated fixes
autofix-ci[bot] 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
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
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,34 @@ | ||
| send_batch: requeue every packet in an unsent coalesced datagram | ||
|
|
||
| `off` is `unsigned`, so when the first unsent spec in a batch is at | ||
| index 0 and coalesces multiple packets, `&batch->packets[off - 1]` | ||
| indexes with UINT_MAX and `end` lands far past the array. The | ||
| `--packet_out > end` condition is then false after the first iteration | ||
| and only the last packet of the coalesced group is returned to the | ||
| connection; the earlier ones (typically the INIT ACK and the HSK | ||
| CRYPTO carrying the client Finished) are silently dropped and never | ||
| retransmitted, so the peer never completes the handshake. | ||
|
|
||
| Rewriting the bounds as [off, off+count) avoids the underflow while | ||
| preserving the reverse iteration order that send_ctl_sched_prepend | ||
| relies on. | ||
|
|
||
| --- a/src/liblsquic/lsquic_engine.c | ||
| +++ b/src/liblsquic/lsquic_engine.c | ||
| @@ -2739,12 +2739,12 @@ | ||
| off = batch->pack_off[i]; | ||
| count = batch->outs[i].iovlen; | ||
| assert(count > 0); | ||
| - packet_out = &batch->packets[off + count - 1]; | ||
| - end = &batch->packets[off - 1]; | ||
| + packet_out = &batch->packets[off + count]; | ||
| + end = &batch->packets[off]; | ||
| do | ||
| batch->conns[i]->cn_if->ci_packet_not_sent(batch->conns[i], | ||
| - *packet_out); | ||
| - while (--packet_out > end); | ||
| + *--packet_out); | ||
| + while (packet_out > end); | ||
| if (!(batch->conns[i]->cn_flags & (LSCONN_COI_ACTIVE|LSCONN_EVANESCENT))) | ||
| coi_reactivate(sb_ctx->conns_iter, batch->conns[i]); | ||
| } |
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
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,131 @@ | ||
| /** | ||
| * HTTP/3 fetch under injected UDP send faults. Exercises lsquic's | ||
| * packets_out short-return path, which is otherwise only reachable when the | ||
| * UDP send buffer is genuinely full or an ICMP from a dead peer is queued on | ||
| * the shared socket. | ||
| * | ||
| * The first case pins the lsquic send_batch requeue-underflow patch: lsquic | ||
| * coalesces the client's INIT-ACK, HSK CRYPTO (TLS Finished) and a SHORT | ||
| * packet into one datagram with pack_off[0]==0 and iovlen>1. If packets_out | ||
| * returns 0 for that spec, the unpatched requeue loop computed | ||
| * &batch->packets[off - 1] with unsigned off and only returned the last | ||
| * packet of the group to the connection; the Finished was silently dropped | ||
| * and the server could never complete the handshake. | ||
| */ | ||
| import { socketFaultInjection as fault } from "bun:internal-for-testing"; | ||
| import { afterEach, expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isWindows, tempDir, tls } from "harness"; | ||
|
|
||
| const skip = !fault.available() || isWindows; | ||
|
|
||
| afterEach(() => fault.clear()); | ||
|
|
||
| async function withServer(fn: (port: number) => Promise<void>) { | ||
| using dir = tempDir("h3-fault", { | ||
| "server.mjs": ` | ||
| const server = Bun.serve({ | ||
| port: 0, hostname: "127.0.0.1", | ||
| ...${JSON.stringify({ tls, http3: true, http1: false })}, | ||
| fetch: () => new Response("ok"), | ||
| }); | ||
| console.error("PORT=" + server.port); | ||
| process.stdin.on("end", () => { server.stop(true); setTimeout(() => process.exit(0), 50); }); | ||
| process.stdin.resume(); | ||
| `, | ||
| }); | ||
| const proc = Bun.spawn({ | ||
| cmd: [bunExe(), "server.mjs"], | ||
| cwd: String(dir), | ||
| env: bunEnv, | ||
| stdout: "ignore", | ||
| stderr: "pipe", | ||
| stdin: "pipe", | ||
| }); | ||
| let port = 0; | ||
| let buf = ""; | ||
| for await (const chunk of proc.stderr) { | ||
| buf += new TextDecoder().decode(chunk); | ||
| const m = buf.match(/PORT=(\d+)/); | ||
| if (m) { | ||
| port = Number(m[1]); | ||
| break; | ||
| } | ||
| if (buf.length > 4096) break; | ||
| } | ||
| if (!port) { | ||
| proc.kill(); | ||
| await proc.exited; | ||
| throw new Error("server did not report a port:\n" + buf); | ||
| } | ||
| try { | ||
| await fn(port); | ||
| } finally { | ||
| proc.stdin?.end(); | ||
| const killTimer = setTimeout(() => proc.kill(), 500); | ||
| try { | ||
| await proc.exited; | ||
| } finally { | ||
| clearTimeout(killTimer); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const h3 = (port: number, init: RequestInit = {}) => | ||
| fetch(`https://127.0.0.1:${port}/`, { | ||
| ...init, | ||
| protocol: "http3", | ||
| tls: { rejectUnauthorized: false }, | ||
| signal: AbortSignal.timeout(8000), | ||
| } as RequestInit); | ||
|
|
||
| test.skipIf(skip)("EAGAIN on the coalesced handshake datagram is requeued and the fetch completes", async () => { | ||
| await withServer(async port => { | ||
| // Arm an EAGAIN on the second UDP send the client engine makes. The | ||
| // first is the padded Initial (CRYPTO ClientHello). The second is the | ||
| // response to the server's flight: an INIT ACK coalesced with the HSK | ||
| // CRYPTO (Finished) and a SHORT NEW_CONNECTION_ID, i.e. the | ||
| // pack_off[0]==0, iovlen>1 spec whose requeue the patch fixes. The | ||
| // retry-once in us_quic_packets_out is gated on non-EAGAIN, so EAGAIN | ||
| // reaches lsquic as a genuine 0-of-N return. | ||
| fault.set({ syscall: "sendmsg", action: "errno", errno: "EAGAIN", after: 1, repeat: 1 }); | ||
|
|
||
| const res = await h3(port); | ||
| expect(await res.text()).toBe("ok"); | ||
| expect(res.status).toBe(200); | ||
| }); | ||
| }); | ||
|
|
||
| test.skipIf(skip)( | ||
| "a non-backpressure send error on the first datagram recovers without stalling the engine", | ||
| async () => { | ||
| await withServer(async port => { | ||
| // ECONNREFUSED on the very first send is what a stale ICMP on the shared | ||
| // client socket looks like. The errno is remapped to EAGAIN for lsquic, | ||
| // the UDP poll is re-armed writable, and on_drain → send_unsent_packets | ||
| // resends once the single-shot fault is consumed. | ||
| fault.set({ syscall: "sendmsg", action: "errno", errno: "ECONNREFUSED", after: 0, repeat: 1 }); | ||
|
|
||
| const res = await h3(port); | ||
| expect(await res.text()).toBe("ok"); | ||
| expect(res.status).toBe(200); | ||
| }); | ||
| }, | ||
| ); | ||
|
|
||
| test.skipIf(skip)( | ||
| "repeated EAGAIN over several loop iterations recovers via on_drain without stalling the fetch", | ||
| async () => { | ||
| await withServer(async port => { | ||
| // Fail the first handful of sends with EAGAIN. Each failure re-arms the | ||
| // UDP poll's writable interest; on_drain → send_unsent_packets runs on | ||
| // the next iteration, so progress resumes as soon as the rule disarms. | ||
| // The 8s abort is well above lsquic's one-second resume_sending_at | ||
| // failsafe, so the only way to time out is an engine-level stall. | ||
| fault.set({ syscall: "sendmsg", action: "errno", errno: "EAGAIN", after: 0, repeat: 5 }); | ||
|
|
||
| const res = await h3(port); | ||
| expect(await res.text()).toBe("ok"); | ||
| expect(res.status).toBe(200); | ||
| }); | ||
| }, | ||
| ); |
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.