http2, nghttp3: deliver the response bodies a static file server produces - #218
Merged
Conversation
…uces Three defects, each of which turned a correct response into a 200 that carried no body - the shape that is hardest to notice, because the status and the content-length are both right and nothing is logged. hpack: field names went on the wire in whatever case the caller held them. RFC 9113 8.2.1 makes an uppercase letter in a field name malformed, so a handler adding "Vary" or "Content-Type" produced a response a strict peer refuses; curl fails it with CURLE_HTTP2_STREAM and delivers nothing. QPACK had always lowercased for HTTP/3 and the two had simply drifted. The static table is now matched case-insensitively as well, so a canonically spelled name still costs one byte instead of being written out. flow control: the per-stream send window lived on the PendingRequest, but a request that arrives complete - every GET, so every static file - is moved out of _streams the moment it becomes ready, which is before its handler runs. From then on a streamed response could find no window: nothing was charged when sending and a stream-level WINDOW_UPDATE credited nothing. Only the connection window was left, so a body past the peer's 65535-byte stream window overran it and the stream was killed with FLOW_CONTROL_ERROR. The window belongs to the response, and now lives with it. Relatedly, FlushCore parked on credit without first flushing what it had already staged - waiting for a WINDOW_UPDATE that the wait itself prevented, since a peer only credits what it has consumed. nghttp3: Nghttp3ResponseWriter threw from GetMemory, which made it a non-conforming IBufferWriter. A Stream reads into Memory<byte> and cannot read into Span<byte>, so every "copy this stream to the response" helper reaches for GetMemory - exactly how a framework serves a static file. The headers were already sent by then, so the throw could not even become a 500. UnmanagedMemoryManager presents the native staging block as Memory<byte> without copying or moving it, which is how the connection's own buffers are already exposed; one manager per writer, re-pointed per call. Tests assert on the RECEIVED BYTES rather than on frame shape, which is the coverage gap that let all three through: a body that never leaves still produces well-formed HEADERS and END_STREAM, so a test that walks frame headers passes while the peer gets nothing. Http2BodyTests and Http3BodyTests run against real servers over real sockets and cover small, multi-chunk, past-the-window, flush-per-chunk, staged-whole and GetMemory-copied bodies; nghttp3's streamed writer had no server-side coverage at all before this. No measurable throughput change: 1 KB responses 1,611,974 vs 1,611,526 req/s, 1 MB responses 10,488 vs 10,540 req/s, both inside run-to-run noise.
…lock The first version of this handed out a pointer into the native staging block, which is not safe for the one thing a Memory<byte> exists to do here. A caller asks for a Memory in order to await a read into it, so it holds the buffer across that await - and in that window the writer's own machinery moves the block underneath it. A flush swaps staging with the in-flight chunk, and connection teardown frees every writer's block in a finally that does not wait for handlers still parked in a read. The read then completes into memory that has moved or been freed, which is heap corruption rather than the empty body this set out to fix. GetMemory now returns a pooled array and Advance copies it into staging. The GC cannot take an array away while a Memory still refers to it, and the array is only returned to the pool when nothing may still be writing into it. One memcpy on a path that is copying already; GetSpan is untouched and stays zero-copy, since a Span cannot cross an await to begin with. Drops the InternalsVisibleTo that the manager-based version needed.
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
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.
Three defects in the streamed response path, each of which turned a correct response into a 200 that carried no body — the shape that is hardest to notice, because the status and the
content-lengthare both right and nothing is logged.Found while serving static files through GenHTTP's ioxide engine, where the same file worked on HTTP/1.1 and failed on HTTP/2 or HTTP/3 depending on which module produced it.
1. HPACK put field names on the wire in the caller's case
RFC 9113 §8.2.1 makes an uppercase letter in a field name malformed. A handler adding
VaryorContent-Typeproduced a response a strict peer refuses — curl fails it withCURLE_HTTP2_STREAMand delivers nothing, while .NET's own stack tolerates it, which is why this looked like a body-size problem for a long time.QPACK has always lowercased for HTTP/3; the two encoders had simply drifted. The static table is now matched case-insensitively too, so a canonically spelled name still costs one byte instead of being written out as a literal.
2. The per-stream send window was inert for every GET
It lived on the
PendingRequest, but a request that arrives complete — every GET, so every static file — is moved out of_streamsthe moment it becomes ready, which is before its handler runs. From then on a streamed response could find no window at all: nothing was charged when sending, and a stream-levelWINDOW_UPDATEcredited nothing.Only the connection window was left, so any body past the peer's 65535-byte stream window overran it and the stream was killed with
FLOW_CONTROL_ERROR. The window belongs to the response, and now lives with it.Relatedly,
FlushCoreparked on credit without first flushing what it had already staged — waiting for aWINDOW_UPDATEthat the wait itself prevented, since a peer only credits what it has consumed.3.
Nghttp3ResponseWriter.GetMemorythrewThat made it a non-conforming
IBufferWriter<byte>. AStreamreads intoMemory<byte>and cannot read intoSpan<byte>, so every "copy this stream to the response" helper reaches forGetMemory— exactly how a framework serves a static file. The headers were already sent by then, so the throw could not even become a 500.The original reasoning (nghttp3 holds the pointer across the callback that offers it) is true and never required throwing:
UnmanagedMemoryManagerpresents the native staging block asMemory<byte>without copying or moving it, which is how the connection's own buffers are already exposed. One manager per writer, re-pointed per call rather than allocated per chunk.Tests
Assertions are on the received bytes, not on frame shape — that is the coverage gap that let all three through. A body that never leaves still produces well-formed HEADERS and END_STREAM, so a test that walks frame headers passes while the peer gets nothing.
Http2BodyTests/Http3BodyTests— real reactor, real socket, real client: small, multi-chunk, past-the-window, flush-per-chunk, staged-whole,GetMemory-copied, concurrent streams, capitalised headers. nghttp3's streamed writer had no server-side coverage at all before this.HpackEncodeTests— the case rule on encoded bytes, mirroring the existing QPACK tests.Http2StreamedBodyTests— the same body shapes through the in-process pipe, so anything failing on a socket but not here is the transport rather than the framing.Http2OutputQueueTestsintoHttp2StrictClient, and itsFramenow captures payloads.Each guard was verified to fail with its fix reverted.
Suites: Unit 42, E2E 179, Chaos 47, Http 44, Tls 156, File 4 — 0 failures.
Performance
No measurable change, three alternating 5s runs against
main:Both inside run-to-run noise.