Conversation
A single async_write() larger than kWriteChunkSize (16K) stalled forever: data_reader() answers NGHTTP3_ERR_WOULDBLOCK while the current chunk is offered but not yet confirmed, which makes nghttp3 block the stream, and nothing ever resumed it -- start_write() was the only caller of nghttp3_conn_resume_stream(), and for a multi-chunk write there is no next start_write() to reach it. The response body ended up truncated after the first chunk, with no FIN, leaving the peer waiting forever. Resume the stream from on_write_consumed() once the current chunk is confirmed and there is more of write_source left to carve up. Writes of up to one chunk are unaffected, which is why nothing hit this so far: every existing sender chunks at 16K. The client side had the same latent defect for request bodies written in one call; fixed identically to keep the two implementations parallel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
serve_file() maps the requested file into memory for as long as the coroutine frame lives, so it is unmapped on normal exit, on exception and on cancellation alike. Empty files are served without a mapping at all -- mmap() rejects a zero length, and the empty buffer that would be written already means EOF. The URL path below the mount prefix is resolved against the docroot with weakly_canonical(), which folds "..", "." and symlinks before the result is compared against the canonical root, so nothing outside of it can be reached. The prefix has to match whole path segments. Errors map to 404 (missing, directory, escape), 403 (unreadable) or 500. Mounted on "/test" in the example server, serving the test/ directory. Tests cover content, subdirectories, empty and large (multi-chunk) files and every error condition, over HTTP/1.1, HTTP/2 and HTTP/3. To let a prefix-mounted handler see sub-paths at all, the test server now routes everything below /custom to the testcase handler. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Separate build trees like build-asan sit next to build/ and should not show up as untracked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
data_reader() used to copy the caller's buffer into a bounded write_chunk (kWriteChunkSize) and hand nghttp3 pointers into that copy, so a write could complete as soon as the copy was made. Point the nghttp3_vec straight into the caller's buffer instead: the body now travels from the page cache into QUIC packets without an intermediate byte, which for serve_file() means the mmap()ed file is never copied on its way down. What that costs is when the write completes. ngtcp2 keeps pointing into that memory for as long as the bytes may still be retransmitted, so the write handler -- which is what releases the caller's buffer -- has to wait for the data to be acknowledged, per nghttp3's documented retention contract for read_data. A single async_write() is therefore done roughly a round trip later rather than roughly a memcpy later, as it still is on HTTP/2. write_chunk, write_source_copied, write_confirmed and in_flight_writes give way to write_offered (handed to nghttp3) and write_acked (reported back through the new acked_stream_data callback); the write completes once write_acked catches up with the buffer. Cancelling mid-write can no longer just abandon the remainder, because the bytes already offered are still referenced. Reset the stream instead, which makes ngtcp2 drop the queued data and stop reclaiming in-flight bytes -- a body cut short is truncated either way, which is what delete_writer() already resets for. A write that offered nothing, or whose bytes are all acknowledged, leaves the stream unharmed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither side set a TLS 1.3 ciphersuite list, so OpenSSL's default order applied and TLS_AES_256_GCM_SHA384 won -- decisively so on the server, which also sets SSL_OP_CIPHER_SERVER_PREFERENCE. Use the same order as ngtcp2's example server, which puts TLS_AES_128_GCM_SHA256 first. The extra rounds of AES-256 buy nothing here, but on hardware with AES-NI this is worth only a few percent of bulk throughput, not the factor it looks like. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ngtcp2_log_printf() checked the log level and returned early, but by then the work was already done: ngtcp2 formats every frame of every packet into a string before invoking the callback, and only skips that when log_printf is NULL. So each packet paid for formatting that was then discarded. Install the callback only when trace logging is actually enabled. Measured with callgrind on a 64 KB file benchmark, this drops the server from 344M to 269M instructions (-22%), with ngtcp2_fmt_write_str, ngtcp2_encode_uint, log_fr and strlen leaving the profile entirely. The level is now sampled when the connection is created rather than per call, so raising it at runtime does not affect connections that already exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
serve_file() resolved the path and mmap()ed the file on every request, then unmapped it on the way out. For a file already in the page cache that was the single largest per-request cost -- larger than resolving the path, and larger than the QUIC send path itself: ~17 syscalls per request, of which the map and unmap pair dominated. Cache the mapping, keyed by request path, together with the response headers derived from it. A hit still stat()s the file and compares device, inode, size and mtime against what was mapped, so a file replaced or modified on disk is picked up on the next request -- one syscall instead of seventeen. MappedFile records that identity from the fstat() on the fd it mapped, leaving no window between checking and mapping. The cache is bounded (256 entries / 64 MiB, least recently used evicted first, keeping at least the entry just inserted) and guarded by a mutex, with the mapping itself built outside the lock so concurrent misses do not serialise. serve_file() holds a shared_ptr, so an in-flight response keeps its mapping valid even if the entry is evicted or replaced mid-write. A hit deliberately does not re-run resolve(), so re-pointing a symlink along the path is only noticed once the file it pointed at changes. That direction is safe: a stale entry can only keep serving a file that already passed the containment check, never a newly escaping one. Measured on `h2load --h3 -n 10000 -c 4 -m 3` against a 64 KB file: 7.9k -> 15.5k requests/s, with system time down from 350ms to 130ms per 10k requests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
on_read() ran the whole write path -- aggregate, flush, re-arm the timer -- for every datagram, and udp_on_read() feeds it up to 32 of them per wakeup (more once a GRO-coalesced read is split). Each pass therefore saw only what ngtcp2 happened to have queued at that instant, so a response went out as several small GSO batches instead of one big one. Mark the session in on_read() instead and let udp_on_read() write once per session, after every datagram the socket had queued has been handed to ngtcp2. This is what ngtcp2's example server does with signal_write() and its writable watcher. handle_expiry() and wake_write() go through the same flush_write(), and the EAGAIN return became a break so a drained socket still reaches the write. Per request this removes all of the single-packet sendto() calls (1.45 -> 0), and takes sendmsg() from 1.96 to 1.67 and recvmsg() from 3.93 to 2.08 -- 9.85 syscalls per request down to 7.0. It does not move throughput: at this point the server is bound by user-space CPU, not syscalls, and the ~3us per request this saves does not show up against a ~57us budget. Committed because it is strictly less work for the same result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wake_write() posted a flush unconditionally, and a response submits its headers, its body and its EOF separately -- so three posts per response where one would do. The first pass wrote everything there was, and the other two walked the connection for nothing and re-armed the timer on the way out. Arm once and clear the flag when the flush runs, which is what ngtcp2's example server gets for free from ev_io_start() on an already-active watcher. Measured over 2000 requests this takes timerfd_settime from 3480 calls to 1075 (1.74 -> 0.54 per request). sendmsg and recvmsg are unchanged, confirming those extra passes never produced a packet. It is not a throughput win. Interleaved A/B against the parent commit, five runs each: 14408 vs 14459 req/s and 578ms vs 574ms of server CPU per 10k requests, both inside the run-to-run spread. It also draws ~23% more acknowledgements from the peer, for reasons not established. Committed because it is strictly less work per response, not because it made the benchmark faster. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
async_read_some() copied out of pending_read.front() and stopped there, however
much room the caller had left and however much was queued behind it. A chunk is
what arrived in one QUIC packet, so a 64k request body was handed over in ~48
reads of ~1.4k each.
For a handler that only consumes, that is 48 coroutine round trips instead of
one or two -- wasteful but survivable. For a handler that answers each read with
a write, it is fatal: a body write completes only once the peer acknowledges it
(the response body is passed to nghttp3 by reference, so the caller's buffer
cannot be released any earlier -- see the comment above write_active), so every
one of those 48 iterations costs a full round trip. The server ends up with at
most one packet of data to send at a time: strace shows one sendto() of 1444
bytes per received datagram, never a GSO burst, with the socket idle in between.
h2load --h3 https://localhost:8080/echo -d test/data/64kminus1 -n 1000 -c 4 -m 3
before 306 / 308 / 272 / 451 / 382 req/s, 890-1130 ms server CPU / 1500 req
after 7286 / 7379 / 6671 / 7292 / 7379 req/s, 130-160 ms server CPU / 1500 req
Interleaved runs, same binary pair alternating. That is ~24x the throughput at
~7x less CPU per request: the lockstep was not just waiting, it was paying a
wake, a flush, a timer re-arm and a one-packet send for every 1.4k of body.
Reads per request drop from 48 to 3.
/eat_request and the file handler are unchanged (13.0k and 15.5k req/s before
and after): the former reads into a 1k buffer, so there is nothing to coalesce,
and the latter has no request body at all.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… first
nghttp3's recv_data callback points into the packet it is parsing, so
on_data_chunk() copied the bytes into a vector of their own and pushed that onto
pending_read -- a malloc, a copy in, a copy out and a free per QUIC packet, about
fifty of each per 64k of request body. In the common case the copy out happened
immediately afterwards, because a handler that keeps a read outstanding was
already waiting for exactly those bytes.
Offer the chunk to call_read_handler() where it lies instead, through a new
`incoming` buffer that the fill loop drains after the queued chunks (which
arrived earlier and must go first). Only what the reader could not take is
parked. A callgrind profile of /eat_request showed the pending_read vectors
accounting for roughly half of all allocations, 90 per request.
h2load --h3 https://localhost:8080/eat_request -d test/data/64kminus1 -n ...
Marginal instructions per request, differenced between n=100 and n=400 runs to
cancel startup:
before 575,236
after 543,105 (-5.6%)
Not visible in wall clock at this operating point -- 4000 requests take ~220 ms
of server CPU either way, and 5.6% of that is inside the 10 ms accounting
granularity. It is strictly less work per packet, not a measured speedup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…assert start_write() asserted !write_active before the block that handles a second async_write() of EOF -- and that block exists precisely for the case where write_active is still true: a cancelled FIN keeps its write alive (a FIN cannot be un-sent) and only detaches its handler, so the re-issued EOF adopts it. The assert and the branch it guards have contradicted each other since 843593f; what changed on this branch is the write-flush timing, which now leaves the FIN waiting for credit long enough for Backpressure/HTTP3 to hit it. Only Debug builds have asserts, so the RelWithDebInfo CI job stayed green while ASAN, TSAN and Coverage all aborted with SIGABRT at the same spot: client_impl_udp.cpp:771: Http3ClientStream::start_write(...): Assertion `!write_active' failed. Move the assert below the early return, where the invariant it states actually has to hold. The server side had the same shape, not yet reachable. Verified with the CI configuration (Debug, GITHUB_ACTIONS defined): ASAN 189 passed / 5 skipped, TSAN 192 / 2 with no ThreadSanitizer warnings, coverage target 194/194. No effect on release builds, where the assert is compiled out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Received headers were logged for every protocol, submitted ones for none except the h2 client request -- so a debug log showed what came in but never what went back out, which is the half you need when a response looks wrong on the wire. Log the full submitted header block at debug level in all three response paths (ResponseWriter::async_submit, NGHttp2Writer::async_submit, Http3Stream::submit_response) and in the h3 client's request path, which had been missing the dump its h2 counterpart already did. Same style as the received ones: bold-blue name, indented under the message's start line. On the h3 receive side, the recv_header callback logged one line per header as it arrived, so the block came out *above* the request/status line and interleaved with unrelated session logging. Buffer the raw name/value pairs on the stream and dump them in end_headers instead. Buffering is guarded by should_log(debug) and the storage is released as soon as it has been logged, so nothing is allocated at info level and above. The pairs are kept verbatim rather than re-rendered from the parsed request, which no longer has them: pseudo-headers are consumed into method/url/status_code/content_length and Fields::set() collapses duplicates. The status lines now read "200 OK" in all three, matching the "POST /echo" request line, rather than each protocol picking its own phrasing. [h3:[127.0.0.1]:50266.0] POST http://127.0.0.2/echo [h3:[127.0.0.1]:50266.0] :method: POST [h3:[127.0.0.1]:50266.0] :scheme: http [h3:[127.0.0.1]:50266.0] :path: /echo [h3:[127.0.0.1]:50266.0] :authority: 127.0.0.2 Trailers are unaffected: nghttp3 routes them through separate recv_trailer / end_trailers callbacks, which this code does not register. 192 passed / 2 skipped, unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A peer that vanishes without a CONNECTION_CLOSE -- h2load interrupted mid-run, a killed client -- is noticed only by the idle timer. But the IDLE_CLOSE branch of handle_error() just signalled done: with no packet sent there is no closing period, so the cleanup in udp_on_read() never ran for such a session, and it stayed in m_quic_handlers for the lifetime of the server, holding streams whose request handlers were still suspended on a peer that was long gone. Erase it from the demux map right there instead, and cancel its timer. The client side skips its CONNECTION_CLOSE in the same situation, and both sides log the idle timeout as an ordinary end of life rather than as a warning. Server::Config gains an idle_timeout knob (default 30s, as before) so the new testcase can freeze a client mid-request and watch the server give up on it half a second later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Loss recovery -- retransmits, PTO, ACK handling -- was only ever exercised by whatever the loopback happened to do, which is nothing. Server::Config gains drop_rate_rx and drop_rate_tx, the probability (0.0 .. 1.0, default 0.0 = off) that an individual QUIC packet is thrown away instead of being processed or sent. The rates ride along on Endpoint, which every Http3Session copies, so there is no process-wide state to collide between servers in a test. RX dropping sits in the segment loop of udp_on_read(), after GRO splitting, so coalesced datagrams are dropped one packet at a time rather than 64k at once. TX dropping sits in send_udp(), the funnel all send paths share; send_udp_gso() takes its per-packet fallback path while drop_rate_tx is non-zero, because one GSO batch would otherwise be an all-or-nothing drop of up to N packets. A dropped packet reports success, so ngtcp2 simply retransmits it. The server binary exposes both as --drop-rx and --drop-tx. At 15% in both directions, an h2load run over h3 still completes every request, with the drops visible in the log. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Let claude do a series of performance tests against the
ngtcp2-serversample server. Provide a simple file-serving request handler.