Repository navigation
usockets(udp): bound a readable event at 32 datagrams #37103
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
Open
robobun
wants to merge
6
commits into
main
Choose a base branch
from
farm/7c2d84da/udp-recv-budget
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
10b3082
udp: bound one readable dispatch at 32 datagrams like libuv
robobun 700fd13
test: race fixture readiness against flooder exit; assert stderr/stdo…
robobun 054933f
Merge remote-tracking branch 'origin/main' into farm/7c2d84da/udp-rec…
robobun 2292940
usockets(udp): bound a readable event at 32 datagrams
robobun 6d78a04
test(udp): bound the retries of the recv budget fixtures in wall time
robobun 4d930f3
test(udp): hand the last attempt of firstUsable the time that is left…
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
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,68 @@ | ||
| // Helpers for fixtures that measure what one iteration of the event loop does: | ||
| // how many datagrams, reads or connections a socket hands over before the loop | ||
| // goes on to its timers, immediates and other sockets. For processes that run | ||
| // with bunEnv, which makes bun:internal-for-testing available. | ||
| import { getEventLoopStats } from "bun:internal-for-testing"; | ||
|
|
||
| /** | ||
| * Counts events by the iteration of the event loop they happen in. Call | ||
| * `count()` from the callback to measure. `perIteration` has one entry for each | ||
| * iteration that counted something, in order. | ||
| */ | ||
| export function iterationCounter() { | ||
| const counts = new Map<number, number>(); | ||
| let total = 0; | ||
| return { | ||
| count(events = 1) { | ||
| const { iteration } = getEventLoopStats(); | ||
| counts.set(iteration, (counts.get(iteration) ?? 0) + events); | ||
| total += events; | ||
| }, | ||
| get total() { | ||
| return total; | ||
| }, | ||
| summary() { | ||
| const perIteration = [...counts.values()]; | ||
| return { total, max: Math.max(0, ...perIteration), perIteration }; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Lets the event loop iterate until `done()` holds. Resolves to false when it | ||
| * still does not hold after `seconds`, so that the caller can report what it | ||
| * has and not hang. | ||
| */ | ||
| export async function iterateUntil(done: () => boolean, seconds = 10) { | ||
| const deadline = performance.now() + seconds * 1000; | ||
| while (!done()) { | ||
| if (performance.now() > deadline) return false; | ||
| await new Promise<void>(resolve => setImmediate(resolve)); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Runs `scenario` until a run is `usable`, at most `attempts` times and within | ||
| * `seconds` in all, and resolves to that run. A scenario that depends on what | ||
| * the kernel had queued before the loop polled cannot promise it on every | ||
| * platform and under every load, so a run that did not get there is set up | ||
| * again. When no run is usable the last one is the result, for the test to | ||
| * fail on. | ||
| * | ||
| * The scenario gets the seconds that are left of the budget. It passes them to | ||
| * `iterateUntil`, so that a run whose datagrams never arrive ends with the | ||
| * budget and not 20 deadlines later. | ||
| */ | ||
| export async function firstUsable<T extends object>( | ||
| scenario: (secondsLeft: number) => Promise<T>, | ||
| usable: (run: T) => boolean, | ||
| { attempts = 20, seconds = 20 } = {}, | ||
| ) { | ||
| const deadline = performance.now() + seconds * 1000; | ||
| for (let attempt = 1; ; attempt++) { | ||
|
robobun marked this conversation as resolved.
|
||
| const secondsLeft = Math.max(0, (deadline - performance.now()) / 1000); | ||
| const run = await scenario(secondsLeft); | ||
| if (usable(run) || attempt === attempts || performance.now() > deadline) return { ...run, attempt }; | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Operators of
Bun.serve({ http3: true })andnode:quicservers get kernel-side packet loss under concurrent load that the base branch does not have. The cap at packages/bun-usockets/src/loop.c:1033 is per socket, and a QUIC listener is one socket for every connection, so 32 packets per loop iteration is the whole server's inbound ceiling. Fix: keep the bound where on_data runs user JS, but let QUIC sockets drain what the base drained, e.g. a per-socket budget that quic.c raises, a time-bounded continuation past 32, or a larger SO_RCVBUF on QUIC sockets. The PR's Downsides put the loss at 6.5 % to 16.2 % for 64 uploads; the author calls it accepted, butus_quic_udp_on_dataruns no user code.Why this was flagged
An HTTP/3 server started with Bun.serve({ http3: true }) listens through one QUIC UDP socket for all its connections (packages/bun-uws/src/Http3Context.h:100 calls us_quic_socket_context_listen, packages/bun-usockets/src/quic.c:1017 creates the single us_udp_socket_t); node:quic listen() does the…
Verification: normal — acknowledged in diff: the PR description's "Downsides" section states "An HTTP/3 listener is one socket. 64 concurrent uploads: kernel loss 6.5 % to 16.2 %, CPU per MiB +10 %" and the Notes leave "the form of the bound" as an open question; the mechanism the note describes is accurate and nothing in the code bounds it further. Triggering condition: an HTTP/3 (`Bun.serve({ http3:…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cost is real and the Downsides and the Notes of the description state it with the numbers. The bound applies to the QUIC sockets on purpose, and the choice of its form is the open question for a maintainer at the end of the description.
Why the same bound:
us_quic_udp_on_dataruns no JS, but each packet goes throughlsquic_engine_packet_in(decrypt, frame parse, stream buffering) before the event ends, andloop_postthen runsprocess_connsover all of it. The starvation was reproduced onnode:quic, not only onnode:dgram: a 4 MiB body next to a loop that blocks 20 ms between turns read 181 to 555 packets in one turn on main (comment above, Aug 11). A budget that quic.c raises brings that back. Node's QUIC endpoint receives through libuv'suv_udp_tand so has the same count per event.What reduces the loss on a listener without moving the bound: a larger receive buffer for the QUIC sockets (
quic.csets none, so the listener has the default 208 KiB for all its connections). The Notes list it as the follow-up that comes after the bound, because on main a larger buffer makes one event longer. The other form in the Notes, 32 as a floor and then a short time bound, also removes the ceiling for a cheap handler. Both are small changes on top of this one. I leave this thread open for the maintainer's answer on the form.