fix: WebSocket and HTTP/2 streaming performance, benign WINDOW_UPDATE budget, unbounded pendingBody - #341
Merged
Conversation
Every ws.send pushed the connection's write buffer to the socket through the flush hook, even when the send came from an onMessage handler inside the read batch whose caller (handleRead) already flushes once after processInput. One recv() carrying N frames therefore cost N send() syscalls, plus an armWrite/disarmWrite kevent pair on every EAGAIN; applyOutboxConn had the same shape, flushing per omWs message instead of once per outbox batch. Add Connection.flushHold, a loop-thread counter held while a batch of output is being produced on a connection. The flush hook skips the socket write while it is held (below respHighWater, so a handler that emits megabytes inside one dispatch still meets the socket and its backpressure/onDrain signal), and the batch owner flushes exactly once as the last hold drops: the WebSocket read batch in processInput, and processOutbox's tail for the frames/resumes applied from the outbox. Off-loop sends keep their outbox round trip and are flushed by that tail. Nested holds collapse to one flush, and the tail runs from a finally so a raising message can never leak a hold onto a live connection.
) Every outbound HTTP/1 frame was serialized into WsConn.outBuf and then copied wholesale into c.wbuf by the very next wsFlushH1, an extra memcpy (and a possible realloc) per message that existed only so the h1 path could share the outBuf staging the h2/h3 transports need for per-stream DATA framing. Route every frame producer through wsOut, which hands back c.wbuf when the transport is HTTP/1 (w.flush == wsFlushH1 doubles as the transport tag) and w.outBuf otherwise. wsFlushH1 keeps its drain of a non-empty outBuf as a backstop and still applies wantClose. Backpressure accounting is unchanged: bufferedAmount already read c.pendingOut for h1 and h2Pending for h2/h3.
Per inbound frame the pump allocated a fresh payload string (a new WsFrame per loop iteration), unmasked it byte by byte with a per-byte mask[i and 3] lookup, copied it again in dispatchMessage, and the async adapter copied it a third time into its queue. * parseFrame unmasks 8 bytes per step by XORing with a word holding two copies of the 4-byte key, tail byte-wise. The word is built with copyMem from the key bytes, so the block XOR is byte-exact on either endianness; the block loop always consumes a multiple of 4, so `i and 3` resumes correctly in the tail. * wsPump parses into one thread-local frame whose payload buffer is reused (one buffer per loop thread, not per connection; dropped after a batch that carried a payload over 64 KiB so a one-off huge message does not pin its buffer). A thread-local is sound because dispatch never re-enters the pump. * dispatchMessage no longer copies its argument: it hands the buffer (the frame payload, or the reassembly buffer) to onMessage as a read-only view and only permessage-deflate produces a second buffer, which zlib allocates anyway. Fragment reassembly keeps `frag`'s buffer instead of moving it out on every completed message. * The adapter's AwaitableReader.feed takes its item as `sink`, so the message a WebSocket reader must copy out of the pump buffer is copied exactly once instead of twice. tests: RFC 6455 5.7 masked vectors, unmasking at every length 0..40 from every start offset 0..7, and a reuse check that a shorter payload leaves no tail of the previous one.
…338) push triggered the SelectEvent (a write() on the wakeup fd) for every message, so an off-loop ws.send or worker response cost a lock acquisition plus a wakeup syscall each, and the loop woke for one-message batches: a worker pushing 100k frames/s paid 100k wakeup writes. Record the queue length under the lock and trigger only when it was empty. The loop side drains with a swap under the same lock, which is what makes the coalescing safe; the argument (a push that finds the queue non-empty is always covered by an as-yet-unconsumed drain, and the loop never sleeps holding messages) is written out at the call site.
#331) H2Stream.pendingBody was only reclaimed when the backlog reached exactly zero (pendingPos == pendingBody.len). A streaming producer keeps the backlog non-zero by construction -- the scheduler drains all but a few bytes, h2ResumeProducers wakes the producer and it appends the next chunk behind the already-sent prefix -- so that case never fired and the buffer grew by the whole response (tens of MB per stream, ~768 MB RSS across 32 concurrent downloads). Backpressure could not see it: bufferedAmount and h2Writable measure `len - pendingPos`, which excludes the dead prefix. compactPendingBody moves the live remainder down with moveMem once the dead prefix is both at least respHighWater and at least as large as that remainder. The absolute floor keeps small responses from memmoving on every frame; the "prefix >= remainder" half keeps the copying O(1) amortised per byte sent, so a handler that ignores backpressure and queues one huge chunk cannot turn compaction into an O(n^2) memmove storm. A well-behaved producer's buffer now settles near 2 x respHighWater instead of the whole response. tests/test_http2_download.nim drives the frame level directly (curl hides flow control), credits the windows as a real client does so the backlog never reaches zero, and samples the raw pendingBody length from the handler. Before this change the peak was 4136960 bytes for a 4 MiB response; it is now bounded well under 512 KiB.
… chunk per loop pass (#332) flushOut used to treat a drained c.wbuf as the end of the work: it called h2DrainResume (which tops the buffer up to respHighWater = 64 KiB), armed write interest and returned. An h2 connection could therefore push at most ~64 KiB per event-loop iteration, each costing an arm / wait / disarm round trip, so download throughput was bounded by loop iterations x 64 KiB rather than by the socket. The write path is now an outer loop: after a refill it writes the fresh bytes immediately while the socket still accepts them, and arms write interest only on EAGAIN / tlsWantWrite (those branches, including their h2DrainResume calls, are unchanged). h2MaxRefillRounds = 16 bounds one connection at ~1 MiB of h2 output per flushOut call so a producer that refills forever cannot starve the rest of the loop; past the bound it arms write and yields as before. The non-h2 tails (closeAfterFlush, WebSocket wsDrained, HTTP/1 streamed onRespDrain) return explicitly, so their behaviour and the single-shot onRespDrain contract are untouched. disarmWrite is a no-op once disarmed, so running it per round costs nothing. flushOut is called only from the event loop (never re-entered from a codec callback), so the loop adds no re-entrancy the resuming / scheduling guards did not already cover.
…get (#335) A connection-level WINDOW_UPDATE that "unblocked nothing" (nothing queued and connSendWindow already positive) was charged against maxControlFrames, as was any update for a stream that had just closed. That is the normal shape of a legitimate client's flow control on a long download: with a wide connection window the per-stream window is the only thing that ever blocks, so conn updates routinely arrive while every stream is window-blocked or has backlog 0 with its producer parked. The budget only decayed on newly accepted requests, so a single-stream 1 GiB download tore itself down with GOAWAY(ENHANCE_YOUR_CALM) after ~1000 updates. Two changes, both aimed at the ratio the budget is actually about (overhead frames per unit of useful work): * noteDataProgress decays the budget per respHighWater of response DATA emitted, exactly as an accepted request does. A flood that produces no response bytes still decays nothing. * a connection-level update that unblocked nothing, and an update for a closed stream, are charged only when no stream is open at all. With a stream open such a frame costs O(1) (no scheduler pass, no response), the increment must be non-zero (RFC 9113 6.9 already makes zero a PROTOCOL_ERROR) and must not overflow the window, and RFC 9113 5.1 requires tolerating post-close updates anyway. PING, SETTINGS, PRIORITY, CONTINUATION, GOAWAY, unknown frame types and refused-stream RSTs are all still charged unconditionally, so every flood test_security_dos covers behaves exactly as before. tests/test_http2_download.nim adds the regression: a 4 MiB download whose client returns its connection credit in 512-byte updates against a server configured with maxControlFrames = 100. Before this change it died after 90111 bytes and 176 updates; it now completes with no GOAWAY. THREAT_MODEL and HARDENING record the new decay and the narrowed charge.
…ning the stream table (#339) h2Input recomputes the connection's timeout policy on every input event, and h2Deadline answered it by calling h2AwaitingClient and h2BlockedOnPeerWindow, each of which walked the whole stream table with mpairs. Every inbound frame batch was therefore O(open streams) even when a single WINDOW_UPDATE arrived: with 256 concurrent downloads sending per-stream WINDOW_UPDATEs, every read batch scanned 256 entries twice. H2Conn now carries three counters maintained at the state transitions: awaitingClientStreams (no END_STREAM seen), backlogStreams (unsent pendingBody bytes) and windowBlockedStreams (of those, sendWindow <= 0). The connection send window is a single value, so h2BlockedOnPeerWindow is `windowBlockedStreams > 0 or (connSendWindow <= 0 and backlogStreams > 0)` -- exactly the old predicate, in O(1). Three small helpers keep them exact, all idempotent so a call from a site that changed nothing is free: syncSendState after any mutation of pendingBody / pendingPos / sendWindow, noteEndStream where the client half-closes, and dropStreamCounters just before a stream leaves the table (teardownStream, and h2WsFinalize which deletes directly). Each stream caches its own contribution (hadBacklog, wasWindowBlocked) so the helpers compare rather than assume. h2CheckCounters re-derives all three with a full scan and asserts they match; h2Deadline calls it under `when not defined(release)`, so the whole test suite audits every codec path on every input event while release builds compile it out. Verified it bites: dropping the counter maintenance at stream creation fails test_http2 immediately.
…per loop turn (#334) A streamed h2 write paid two full memcpy passes (into pendingBody, then out of it again in emitOneFrame) and one send() per producer chunk. h2WriteDirect emits as many DATA frames as flow control allows straight from the caller's buffer into c.wbuf and parks only the remainder, so a write that the windows can absorb costs one copy instead of two. It is taken only when the result is byte-identical to what the scheduler would have emitted next: the stream's own backlog must be empty (queued bytes keep their place) and the ready-queue must be empty (no other stream's turn to overtake), which leaves h2Schedule's RFC 9218 fairness contract untouched. Frame shape, max frame size, padding, END_STREAM and trailer semantics stay with emitOneFrame. The per-write socket flush is deferred with the existing flush-hold mechanism (#333) rather than a second one: * processInput holds the flush across an h2 frame batch, as the WebSocket read batch already does, so N responses/chunks produced by the handlers dispatched from one recv() cost one send(); * the sendFile chunk-apply path takes an outbox batch hold, so several chunks landing in one batch are pushed once (its own tail flush is now that batch flush); * flushOut holds the flush across h2DrainResume, so a producer resumed there does not re-enter flushOut through the hook when the enclosing call is already about to write the bytes (or has just armed write interest). A write that fills the buffer past respHighWater still meets the socket at once, so a producer emitting megabytes inside one dispatch is unchanged.
…e read-ahead budget (#340) applyFileChunk wrote the arrived chunk and only then pulled the next read. res.write is synchronous down to the socket (on a fast peer flushOut drains the whole chunk, refilling from the write scheduler, before it returns), so the disk read never overlapped anything: the socket sat idle for the read latency on every chunk, which is the stall half of #340. Dispatch the read first, then write, so the worker reads while the loop thread is still pushing bytes. applyFileStart does the same after the head. The gate now runs before the chunk is written, so the read-ahead budget drops from 2 x fileChunkCap to one chunk to keep the same dispatch instant and the same ceiling: with two chunks the peak per-stream backlog measured ~960 KiB against ~736 KiB here. The ceiling is the budget plus two chunks -- the chunk that passes the gate, and the one already in flight, which is always written when it lands -- and the budget must stay >= respHighWater so parking always follows a write that reported backpressure and therefore armed the drain that resumes the read. Test: a 3 MiB sendFile over h2 against a 32 KiB stream window, probing the download stream's backlog from a second stream on the same connection (a pkFileChunk pin does not pause h2 input, so those probes are served mid-transfer). It asserts the backlog never exceeds the budget plus two chunks and that the body still arrives byte-exact.
…ATA sent (#335) The previous #335 change stopped charging a connection-level or closed-stream WINDOW_UPDATE whenever any stream was open. That fixed the false GOAWAY on long downloads but reopened the framing-flood vector THREAT_MODEL documents: a peer that parks one stream (zero window) can then send unlimited 13-byte updates that unblock nothing, each parsed and never counted. A client can only return credit for DATA it received, so the benign rate is bounded by the bytes we sent. Model exactly that: every 256 bytes of response DATA emitted earns one uncharged no-progress update (capped at 4x maxControlFrames so a quiet download cannot bank an unbounded allowance), a benign update spends one, and an update with no credit behind it is charged as before. The DATA-progress decay of the counter is kept. Regression: a stream held open with a zero window (no DATA ever sent) plus a burst of conn updates must still GOAWAY(ENHANCE_YOUR_CALM); the benign download test from the first change still passes with a 512-byte update granularity, which is 100x finer than any real client uses.
The cross-language bench reused the stress soak Dockerfile, which compiles -d:release with --stackTrace/lineTrace/stackTraceMsgs/panics/debugger so a soak failure gets a native traceback. Those tracing flags add a bookkeeping step on every call, so every vortex row in the comparison table was a debug-build number measured against Go's and Rust's release builds. Add a PROFILE build arg to the Dockerfile: soak (the default, unchanged, still used by the stress soaks and the proxy suite) and bench (-d:danger --passC:-flto, the same build the perf/benchServer nimble tasks use). The bench runner passes PROFILE=bench for the vortex image.
The reusable inbound frame buffer from #336 is a thread-local. Nothing destroys a thread-local when its thread exits, so the last payload buffer outlived the loop thread as an unreachable heap block, and CI's valgrind memcheck ws scenario reported it definitely lost (23 bytes) on every target. Release it from runLoopThread on both the clean and error paths.
cryo2010
added a commit
that referenced
this pull request
Sep 28, 2026
…ept races, body-pause leak, writeTimeout default) (#349) * fix(loop): route every body-pause clear through one helper A streaming HTTP/1 read that hits the read-ahead high-water sets c.bodyReadPaused and takes a slot in the loop's bodyPausedConns count, which caps the selector wait at 2 ms so the async consumer keeps draining. Only closeConn and resumeBodyImpl released that slot: startStreamingDispatch and resetRequest (via resetRequestState) cleared the flag on their own, so the count stayed high forever and pinned the loop thread's selector wait at the paused-body cadence for the rest of its life. The reset path is reachable: a worker's response is applied from the outbox, and resumeAfterRespond resets the request without looking at whether the socket read is still paused. A streaming route with a manualAck sink that answers from a `blocking:` worker without draining its read-ahead debt leaks a slot per request. Clear the flag only through clearBodyPause, which also gives the slot back, and audit the count against a full scan of the connection table once a tick in a debug build (in the spirit of h2CheckCounters), so a future clear that bypasses the helper trips an assertion in the test suite instead of quietly spinning the loop in production. The new test in the HTTP/1 streaming-request suite drives that worker path and then serves a plain request: before the fix the audit reports "bodyPausedConns drift: 1 vs 0" and takes the loop thread down with it. Closes #344 * fix(loop): count and log the accept dropped by a pinned table When an accepted fd lands beyond the connection table and any slot is pinned, handleAccept refuses the connection: growing the seq would move every Connection and dangle the `addr conns[fd]` a blocking: worker holds. That refusal was a bare posix.close, so the client saw an empty connect error (indistinguishable from a network fault or a stalled loop) and the server said nothing at all. Count the drop per loop thread and log it through the same stderr channel the other accept-time backoffs use, rate-limited to one line a second and carrying the running count so a burst stays visible without flooding. The connection cap above keeps its silent drop: that one is a configured policy, while this is an internal limit the operator has no other way to see. This is the minimal half of the fix: the connection is still dropped. A real fix needs a growth-stable table (segmented blocks, so `addr conns[i]` survives growth), which touches every `conns[...]` walk in the loop, request lookup and shutdown paths and is too large to carry here. No regression test: reaching the branch needs an accepted fd past the 1024-slot table (so over a thousand live sockets from the test process) coinciding with a running blocking: worker, which is neither cheap nor deterministic in the suite. Refs #343 * fix(h2): send GOAWAY before a timeout, cap or force close An HTTP/2 connection dropped by the timeout sweep, by the receive-buffer cap, or by forceCloseAll at the end of the shutdown grace went straight to closeConn, so the peer only ever saw a bare TCP close. That is indistinguishable from a network fault (httpx reports RemoteProtocolError: Server disconnected), which leaves even an idempotent in-flight request unsafe to retry, because the client cannot know whether the server processed it. Route those three paths through closeWithGoaway, which emits the same GOAWAY the graceful drain already uses (the last stream id this server processed, so everything above it is known-unhandled) and then closes. NO_ERROR for an expired timeout and for the forced shutdown close, ENHANCE_YOUR_CALM for the receive-buffer cap, where it is the peer's own volume that forced the drop. h2Goaway grew an error-code parameter defaulting to NO_ERROR, so the drain call sites are unchanged. The write is best effort and never blocks: the frame rides whatever output is pending if the socket takes it, and the connection closes either way, so a peer that has stopped reading cannot hold the close. A connection pinned by a running blocking: worker is closed without a GOAWAY, as in markDrain: touching its h2 ref here would race the worker under ORC's non-atomic refcounts. tests/test_http2_goaway.nim covers the idle and body timeouts with a short-timeout server, asserting a GOAWAY(NO_ERROR) naming the last processed stream arrives as the final frame before EOF. The cap path has no test: it needs a megabyte of frames that cannot be compacted while a worker holds the connection, which the suite cannot stage reliably. Closes #342 * test(h1): cover the streaming read-ahead backpressure bound An HTTP/1 streaming upload consumed asynchronously (await req.read, i.e. an onBody sink registered with manualAck) was read off the socket as fast as the kernel could deliver it and copied into the adapter's unbounded reader queue, so a single slow consumer pinned the whole body in RAM and many concurrent uploads OOM'd the server (#271). The bound that fixes it shipped with #325 (7fc9122): feedBody counts delivered-but-unacked bytes per connection, handleRead stops pulling once that debt reaches the high-water (the tail waits in the kernel as real TCP backpressure), and req.ackBody repays the debt and lets the recv loop resume below the low-water. It shipped with no regression test, so nothing pinned the behavior. Add one: a streaming route whose consumer pulls a chunk, parks, then drains, driven by a non-blocking client pushing 32 MiB at it. The test records how much the server had accepted when the socket went unwritable and asserts that it is bounded (about 1.5 MiB measured here, cap 8 MiB to cover both kernel socket buffers) and that the upload still completes byte-exactly once the consumer resumes. Both framings are covered, Content-Length and chunked. Raising the high-water out of the way makes both cases accept all 32 MiB with no stall at all, so the test genuinely pins the bound rather than the timing. Also document the HTTP/1 read-ahead bound in HARDENING.md, which described the HTTP/2 and HTTP/3 upload windows but had nothing for HTTP/1. Closes #271 * test(h2): lock stream-level error scope and racing-frame tolerance The three conditions in #239 (a trailer section without END_STREAM, a HEADERS on a half-closed(remote) stream still in the table, and a HEADERS/DATA racing the server's own early final response) are already handled as RFC 9113 5.1 / 8.1 require: the first two are stream errors, and the third is answered from the bounded earlyClosed set with RST_STREAM(STREAM_CLOSED). That landed in 24d7fd7 (PR #242) without any regression coverage, so nothing pinned the *scope* of those errors, which is the whole point of the issue: escalating them to a connection error aborts every other in-flight request. Add tests/test_http2_stream_errors.nim, a frame-level suite that drives each condition on a connection that also carries an unrelated concurrent request: * trailers without END_STREAM: RST_STREAM(PROTOCOL_ERROR) on that stream, no GOAWAY, the concurrent request still answered. * HEADERS after END_STREAM on a stream still in the table (kept there by a zero SETTINGS_INITIAL_WINDOW_SIZE parking the response body): RST_STREAM(STREAM_CLOSED), no GOAWAY. * DATA plus a trailer section arriving after a streaming route answered 403 without reading the body (exactly the deletion a correct client's frames race): RST_STREAM(STREAM_CLOSED), no GOAWAY. Reverting each of the three code sites to connError makes the matching test fail with a GOAWAY and a lost concurrent response, so the suite pins the fix rather than just the current behaviour. tests/h2client.nim gains addSettingFrame (a one-entry SETTINGS frame), hasResponse and headerPayload. Closes #239 * test(h2): cover the conformance follow-ups from #240 All ten follow-ups are already implemented (24d7fd7, PR #242) but that commit landed without tests, so nothing pinned them. Each sub-item below is now driven at the frame level (or, for the shared classifier, at the unit level) and was confirmed to fail when its fix is reverted: 1. even-id idle streams: DATA, WINDOW_UPDATE and RST_STREAM on stream 2 must be a connection PROTOCOL_ERROR. Each case opens stream 3 first, so the high-water mark is past 2 and only the parity check can catch it. 2. omBlockingDone flush: the flush is in place in applyBlockingDone. It cannot be isolated by a test any more, because #341's batch-tail releaseBatchFlushes also pushes the connection when the worker's response and its done message share an outbox batch; the existing "pipelined: blocking then fast" case in test_blocking.nim exercises the path. 3. connection-specific response fields: a handler that sets connection, keep-alive, transfer-encoding, upgrade and proxy-connection gets none of them on the wire, while an ordinary field still rides. 4. unknown :method: PURGE and a value carrying a space are rejected instead of running the GET handler (h2 and the h3 classifier). 5. CONNECT rules: a CONNECT carrying :scheme and :path (forbidden by RFC 9113 8.5) is malformed instead of being dispatched as a normal request; the Extended CONNECT cases in test_http3_connect.nim stay green. 6. content-length grammar: "+5" with a 5-byte body and "1_0" with a 10-byte body are rejected. The bodies matter: a loose parse reads both as the body length, so the reconciliation check alone would let them through. 7. field-value whitespace: a leading SP or trailing HTAB is malformed (h2 and the h3 classifier). 8. HPACK: after the peer sets SETTINGS_HEADER_TABLE_SIZE=0, the next response header block starts with a 0x20 dynamic-table-size update and still decodes. 9. WS over h2: with the stream send window exhausted and a close queued behind the backlog, no RST_STREAM(CANCEL) is sent; a partial then a full WINDOW_UPDATE drain the queued echo, the close frame and END_STREAM. Restoring the old cancel branch fails the test with CANCEL and no frames. 10. GOAWAY shorter than 8 octets is answered with FRAME_SIZE_ERROR. Note for 4 and 5: an unknown-but-valid-token method and a spec-legal plain CONNECT are answered with a stream PROTOCOL_ERROR, not the 501 the issue suggested. That is the shape the codebase settled on (#245 later aligned the h1 parser's method matching with h2/h3's isKnownMethod rather than the other way round), and the tests lock it in. Closes #240 * fix(h1): ship writeTimeout on by default; document the 431 header cap Two HTTP/1 correctness follow-ups from #249. maxHeaderCount rejects with 431 (Request Header Fields Too Large), which the parser has always returned, but HARDENING.md and the README both documented it as 400. Correct both, and note that the count is an HTTP/1 limit: HTTP/2 and HTTP/3 bound the decoded header list by size (maxHeaderSize) rather than by field count, so there is no equivalent status there to align. writeTimeout shipped at 0 (disabled), so out of the box nothing reaped a slow-reading or zero-window client that stops draining a large buffered or streamed response: the write-stall machinery (armWrite arming writeDeadline on every EAGAIN, disarmWrite clearing it on a full flush, sweepTimeouts reaping it independently of c.deadline) was correct but inert. Default it to 30 s, the same as bodyTimeout, and document it in the setting's doc comment, HARDENING.md and THREAT_MODEL.md. responseTimeout stays at 0: it bounds handler time, which is an application decision. The deadline is idle, not total (every partial write re-arms it), so a response that keeps making progress is never cut off; the streaming and SSE suites are unaffected. Cover both halves in test_security_dos: a client that stops reading a 4 MiB response is reaped and gets a truncated body, while a reader that takes a quarter megabyte every 250 ms receives all of it although the server holds pending output for several times writeTimeout. Arming the deadline once instead of on every partial write fails the second test, and writeTimeout = 0 fails the first, so both pin real behavior. Closes #249 * fix(h3): keep WebSocket frames coalesced with the CONNECT handshake An HTTP/3 Extended CONNECT stream (RFC 9220) dispatches on headers, so the handler runs (and calls h3WsAccept) after the client may already have sent DATA in the same packet burst. Those bytes land in cbBody while st.ws is still nil and accumulate into st.body via the buffered-request-body path. h3WsAccept attached the WsConn but never moved st.body into w.inBuf, so the client's first WebSocket message(s) were silently lost whenever they coalesced with the handshake (the h2 path has done this since h2WsAccept). Seed w.inBuf from st.body in h3WsAccept (clearing st.body) as h2WsAccept does, and pump the seeded frames in h3Drive right after the handler ran, so they are dispatched only once onMessage is installed. The pump mirrors the h2 post-accept pump in dispatchH2 and resolves the WsConn ref before feeding, since wsFeed can tear the stream down. The local suite cannot drive WebSockets over QUIC (no h3 ws client exists for curl), so the regression case goes in the Docker conformance harness (conformance/h3websocket): the aioquic client now opens a stream with a text frame pipelined into the handshake burst and requires the echo back. Closes #259 * fix(ws): guard subprotocol() with the loop-thread check WebSocket.subprotocol() resolved the WsConn ref through wsConnOf from any thread, unlike every sibling accessor (onMessage=, onClose=, onDrain=, bufferedAmount, isAlive), which all go through withWsConn and bail off the loop thread. Those guards exist because resolving and copying the WsConn ref off-loop races the loop thread's non-atomic ORC refcount, so a worker that branched on ws.subprotocol (a natural value to read) could bump or drop the refcount while the loop dispatched frames or tore the stream down: a use-after-free or a leaked WsConn. Route it through withWsConn like the others, so off-loop it reports "" and never touches the ref. The negotiated value is not captured into the WebSocket handle instead: the handle is four plain words deliberately copyable across threads, and a string field is exactly the cross-thread copy the guard prevents. The thread rule is now documented on the accessor and on acceptWebSocket, where the value is first mentioned. The regression test spawns a thread from inside onMessage, reads ws.subprotocol there and reports only its length back (so the probe itself copies no string across threads): 4 ("chat") before the fix, 0 after. Closes #260 * fix(h3): fire onClose when a ws client half-closes before accept On HTTP/3 a client can FIN an Extended CONNECT stream before the handler accepts it (the stream dispatches on headers). cbStreamEnd only delivers wsPeerClosed when st.ws is set, and h3WsAccept never looked at the FIN it recorded, so nothing replayed it after acceptance: the application's onClose never fired and the WsConn lingered until the per-stream idle sweep timed it out. The h2 path has handled this via WsConn.preAcceptFin since h2WsAccept. Carry the recorded FIN into w.preAcceptFin in h3WsAccept, and replay it as wsPeerClosed in the h3 post-accept pump (re-checking that the stream survived the frame pump first), exactly as the h2 dispatch does. Regression case in the Docker conformance harness (conformance/h3websocket), since the local suite has no HTTP/3 WebSocket client: the aioquic client now half-closes the stream in the same burst as the handshake and requires the server's close frame, which only the replayed wsPeerClosed (the same call that delivers onClose) can produce. Closes #261 * fix(h3): drive QUIC egress for a loop-thread ws send or close WsConn sends and closes on the loop thread end with flushHook, whose implementation resolved a ptr Connection from the fd. An h3 handle has fd < 0 (a slot encoding, never a socket), so conn() returned nil and the hook did nothing: the frame sat in the QUIC stream's send buffer until some unrelated event happened to drive the connection. Sends from inside an inbound onMessage were saved by the h3Drive that follows input processing, and off-loop sends ride the outbox (which drives h3 itself), but a loop-thread send from any other source (a chronos or asyncdispatch continuation resumed by the pump, a timeout sweep) could stall on the wire for a full idle wait. flushImpl now flags the loop for fd < 0 instead of resolving a connection, and the loop drives h3 once more after the pump and sweeps if the flag is set. A flag, not a direct h3Drive: the hook can run inside a nghttp3 callback (inbound frame dispatching onMessage, which sends) and re-entering the QUIC stack from there is not safe. h3Drive clears the flag just before ngPump, so the common in-dispatch send costs no extra pass. The local suite has no HTTP/3 WebSocket client, so the case lives in the Docker conformance harness (conformance/h3websocket): the echo server answers "later" from a 200 ms timer continuation and the aioquic client requires that reply within 700 ms, well under the loop's idle wait. Closes #262 * fix(h3): bound pre-accept ws bytes as WebSocket data, not a body Before an Extended CONNECT stream is accepted, cbBody treated inbound WebSocket framing as a buffered request body: it accumulated into st.body under the maxBodySize guard and added the bytes to the per-connection bufferedBytes aggregate. So pre-accept framing was bounded by maxBodySize (vs maxMessage + 1024 once wsFeed takes over), and a client flooding unaccepted CONNECT streams could pin the aggregate that exists to cap request bodies, tripping its cap and resetting an unrelated ws-connect stream with H3_MESSAGE_ERROR instead of the WebSocket 1009. Give a known ws-connect stream (the classifier decides that at headers time) its own branch in cbBody: bound the held bytes by maxWsMessage + 1024, the same ceiling wsFeed applies after acceptance, and keep them out of bufferedBytes. Credit them to QUIC flow control as tunnel bytes, exactly as the accepted path does, so the handover into the WsConn's inBuf does not double count. On overflow before acceptance there is no WsConn to carry a 1009, so the stream is reset with H3_REQUEST_REJECTED (the h3 twin of the h2 REFUSED_STREAM on the same bound) rather than the request-body error, after returning the connection-window credit for the discarded bytes. The overflow path needs more than maxWsMessage of framing to arrive before the handler runs, which the Docker harness's well-behaved client cannot produce, so there is no new conformance case; the accounting and handover this rewrites are exercised by the coalesced-handshake case added with #259, which passes (conformance/h3websocket, aioquic). Closes #263 * fix(ws): make the permessage-deflate teardown idempotent wsStreamClosed freed the zlib contexts under `if w.pmd`, but `pmd` records that the extension was negotiated and is never cleared, so the guard was true on every later call. Nothing in the WsConn stopped a second teardown from handing the same z_stream to deflateEnd/inflateEnd again: only external discipline did, each caller nil-ing its reference (h2WsFinalize and h2WsTeardownAll clear st.ws, wsClosed clears c.ws, wsFlushH3ng clears conn.streams[sid].ws) after a call that has several reachable paths per WsConn (an h2/h3 stream reset, then the connection dying). The second deflateEnd was in the end absorbed by the `inited` flag inside Deflator.close/Inflator.close, one layer below where the invariant was written down, so the codec was relying on a defence it did not state. Move the free into wsClosePmd and gate it on a new `pmdClosed` flag, so the idempotency lives with the state it protects and a second wsStreamClosed is a no-op in both halves (notifyClose was already idempotent via closeNotified). wsStreamClosed is the single teardown path that reaches the contexts: h1 close, the h2 stream reset and finalize, the h2 connection teardown, the h3 stream and connection teardown, and the h3 flush finalize all go through it, and the idle sweep only closes frames. The one other close of a zlib context is wsSetup's negotiation-failure cleanup, which tears down a half-initialized pair before `pmd` is ever set, so it stays where it is. A true double-free is not assertable from a test (the private fields are not visible and the layer below already swallows it), so the regression test is the unit-level one the issue allows: it builds a deflate-enabled WsConn via wsSetup, calls wsStreamClosed twice and checks the ws stays closed without crashing, under `nimble testdeflate`. Closes #264 * docs: changelog for the round-2 protocol hardening fixes Record the fixes for #342, #343, #344, #259 to #264 and the writeTimeout default change from #249, plus the regression coverage added for #239, #240 and #271.
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.
Summary
Fixes the ten findings from a performance review of the WebSocket and HTTP/2 streaming-download paths, including two correctness bugs, plus a bench-harness fix so vortex is compared as a release build.
Closes #331, closes #332, closes #333, closes #334, closes #335, closes #336, closes #337, closes #338, closes #339, closes #340.
Correctness
pendingBodycompacts once the sent prefix passesrespHighWaterand exceeds the live remainder, instead of only on a full drain. A long download whose client kept the backlog non-zero grew the buffer without bound (4 MiB peak for a 4 MiB response in the regression test; tens of MB per stream on the cross-language bench).maxControlFrames. A WINDOW_UPDATE that unblocks nothing spends credit earned by response DATA sent (one per 256 bytes, capped at 4x the budget) and is charged only past that; the budget also decays per 64 KiB of DATA sent. A peer we send nothing to still trips GOAWAY(ENHANCE_YOUR_CALM) after the budget. Both directions have regression tests; THREAT_MODEL and HARDENING updated.WebSocket
Connection.flushHold) defers the socket write to the end of a read batch or outbox batch, so N frames dispatched from one recv() cost one send() instead of N. The gate sits in the flush hook, so h2 writes get the same deferral.onMessagewith no extra copy.outBufstaging remains for h2/h3.HTTP/2 streaming
flushOutrefills from the write scheduler and keeps writing (up to 16 refills per pass) instead of returning to the selector after every 64 KiB.res.writeon a stream with an empty backlog emits DATA straight from the caller's buffer and parks only what flow control refuses; frame shape matches the scheduler and the fast path yields whenever another stream is queued. Writes in one frame batch or outbox batch flush once.sendFiledispatches the next disk read before writing the arrived chunk so the read overlaps the write. The read-ahead budget is one chunk measured before the write, which keeps the same per-stream ceiling.Bench harness
PROFILEbuild arg (soakdefault,bench=-d:danger --passC:-flto) and the bench runner selectsbench.Bench (release build, 20 s, 64 MiB streams, same machine)
The ws bench client sends one message and waits for its echo, so the batching fix (#333) does not engage there; a local micro-benchmark with 32 messages in flight went from ~465k to ~4.2M msg/s.
Test plan
nimble test: 561 checks pass (550 on main); new suitestests/test_http2_download.nimand additions totests/test_websocket_frames.nim/tests/test_websocket_server.nim.nimble testdeflate,nimble testrace(ThreadSanitizer): pass.-d:plainHttpbuild compiles.nimble benchWs/nimble benchStreamDownload: tables above.