fasthttp: refactor toward the vanilla architecture (framing, pooling, per-worker state, append handler)#27771
Open
enghitalo wants to merge 14 commits into
Open
fasthttp: refactor toward the vanilla architecture (framing, pooling, per-worker state, append handler)#27771enghitalo wants to merge 14 commits into
enghitalo wants to merge 14 commits into
Conversation
Port vanilla's pure request-framing layer into fasthttp's parser so the read loop can learn not just *whether* a full request has arrived, but exactly *where* it ends. This lets a single recv holding several pipelined requests be split into individual messages and answered in one batched write, and lets fragmented (TCP-split) requests be reassembled deterministically. New public API in request_parser.v (all pure, allocation-free on the hot path): - frame_request_length(buf) !int - frame_request_length_lim(buf, max_header, max_body) !int (413/431/400 codes) - frame_request_length_lim_idx(buf, max_header, max_body) int (no-Result twin) - frame_expected_total(buf) int (one-alloc read-buffer sizing hint) - frame_head_len(buf) int The existing has_complete_body / decode_http_request / parse_http1_request_line functions are untouched, so the current handler contract and all existing tests keep working. framing_test.v adds split-point fuzzing (every growing prefix must report incomplete until the exact final byte) plus pipelining, chunked, and limit (413/431) coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the five parallel per-fd maps (client_fds / client_buffers / client_read_starts / closing_client_fds / client_write_states) with a single flat fd-indexed table of pooled ConnState objects, and answer HTTP/1.1 pipelined requests in one batched write. Each connection now owns a persistent read buffer and write buffer that are reused for its whole lifetime (capacity kept across requests), and retired connections return their ConnState — buffers included — to a per-worker free-list, so the steady state does zero per-request buffer allocation and no per-connection map churn. This is the server-side allocation/contention that capped throughput and core scaling. Behavioral improvements: - Exact-length framing (frame_request_length_lim_idx): a single recv holding several pipelined requests is split into individual messages and answered in ONE send; TCP-fragmented requests are reassembled deterministically. - Backpressure via EPOLLOUT is unchanged in spirit but now drains the reused write buffer (+ optional sendfile body) and re-arms EPOLLIN on completion. - Oversized request heads return 413 during framing, before the handler runs. The public handler contract (fn (HttpRequest) !HttpResponse), ServerConfig, HttpResponse, takeover modes and lifecycle API are all unchanged, so veb and other consumers are unaffected. The white-box linux regression test is rewritten to the ConnState model and now asserts pipelining, TCP-fragment reassembly, keep-alive re-arm after a consumed edge, and ConnState pooling with buffers retained. BEHAVIOR CHANGE: because request bodies are now framed by their exact declared Content-Length (RFC 9112 §6), a body longer than Content-Length is trimmed to the declared length and the surplus bytes begin the next request on the connection, instead of being absorbed into the request (which was a request-smuggling gap). veb's large_payload test is updated accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an optional ServerConfig.make_state fn () voidptr that each worker thread
calls exactly once at startup; the returned value reaches every request on that
worker as the new HttpRequest.worker_state field. Because there is one instance
per worker thread, a handler can hold per-worker resources (a DB connection, a
reused render scratch buffer) with no shared pool and no mutex —
`unsafe { &MyState(req.worker_state) }` is sound and lock-free.
Both fields are additive: existing handlers and consumers (veb) are unaffected;
worker_state is simply nil when no make_state is configured. Wired through the
Linux worker for now (BSD/Windows follow in the parity commit). A white-box test
verifies the same per-worker instance reaches every request on a worker.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ontrol)
Add an alternative handler contract inspired by vanilla: instead of building and
returning an HttpResponse (which allocates a response []u8 every request), an
append handler writes the raw HTTP response DIRECTLY into the connection's reused
write buffer and returns a Step:
fn (req HttpRequest, mut out []u8, worker_state voidptr, mut ctl ResponseControl) Step
- Step{done, close, suspend} controls the connection (suspend is reserved for a
future async watch reactor and currently drops the connection).
- ResponseControl carries takeover_mode / should_close / file_path out of band,
so takeover (SSE/WebSocket) and zero-copy sendfile still work.
- ServerConfig gains `append_handler`; `handler` is relaxed from @[required].
new_server now validates that exactly one of the two is set.
Appending into the reused, pipelining-batched write buffer removes the
per-request response allocation + copy that the return-a-response contract
requires — the throughput lever from the benchmark analysis. The reactor opens
no -prealloc scope on the append path (growing the write buffer inside a scope
would free it out from under the connection); a handler that wants request
arenas manages its own and leaves it before writing into `out`.
The legacy `handler` path is unchanged, so existing consumers keep working.
White-box tests cover the append path's pipelining/keep-alive, should_close,
and the exactly-one-handler validation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrite veb's internal fasthttp adapter from the return-a-response contract (parallel_request_handler → HttpResponse) to fasthttp's append handler (parallel_append_handler): it parses and routes the request, then serializes the http.Response head+body DIRECTLY into the connection's reused write buffer via the new http.Response.write_to(mut strings.Builder), and signals takeover / close / file streaming through ResponseControl. This removes the per-request res.bytes() allocation and the reactor-side copy into the write buffer. veb's PUBLIC API is unchanged — Context.html/json/text/file and takeover (SSE/WebSocket) all still work; only the private adapter changed. The adapter keeps veb's -prealloc per-request arena behavior by managing its own scope (begin → route → leave before writing into the reused buffer → free), mirroring the discipline the legacy reactor used. Also drops content_length_validation_response: with fasthttp's exact-length framing the parsed body length always equals Content-Length, so the check was dead (an over-long body now frames the surplus as the next request, and a too-short body is an incomplete request handled by the read timeout → 408). Adds http.Response.write_to (net/http) to serialize a response into an existing strings.Builder / []u8 without allocating a fresh buffer. All veb tests pass, including persistent_connection (keep-alive + reusable takeover), sse (manual takeover), static_handler/static_compression (file sendfile), large_payload, and memory_leak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Support the new append handler and per-worker state on the BSD (kqueue) and Windows (IOCP) backends so consumers — notably veb, which now uses the append handler — work across platforms and the tree compiles for macOS/BSD/Windows. - Both Server structs relax `request_handler` from @[required] and gain `append_handler` + `make_state`; each new_server validates that exactly one of handler/append_handler is set (matching Linux). - Both process_request paths dispatch the append handler by wrapping its appended `out` bytes + ResponseControl into the existing response-sending path (one request per read; no pipelining rewrite on these backends yet). The reactor -prealloc arena is gated to the legacy path (the append handler owns its buffer / manages its own scope). - BSD threads per-worker `worker_state` through the Conn (make_state is called once per kqueue worker). On the Windows IOCP thread pool, worker_state is left nil for now (per-thread state needs TLS, and Server.run is not implemented on Windows yet). Verified with cross-compilation (`v -os macos` / `v -os windows`) of the fasthttp example, the veb suite, and the per-platform regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the current module accurately: native I/O multiplexing, per-connection buffer reuse + free-list pooling, HTTP/1.1 pipelining and fragment reassembly, the two handler contracts (append handler + classic handler), lock-free per-worker state via make_state/worker_state, takeover, sendfile file bodies, the Step / ResponseControl reference, the platform-support matrix, and the updated -prealloc notes. Removes stale claims (e.g. "headers are not parsed", the handler returning `![]u8`). All fenced V examples compile under `v check-md`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Before / after benchmarkLocal loopback benchmark isolating the server layer. Because the pre-refactor
Environment: AMD Ryzen 7 5800H (16 cores), Linux 6.8, V 0.5.2, all built with Keep-alive (one request per round-trip, connection reused)
→ ~1.48× throughput and ~2.5× lower latency from the reactor rewrite alone HTTP pipelining (16 requests batched per connection write)
The old reactor answers one request per readiness edge and defers the rest, TakeawayThe refactor targets exactly what the HttpArena gap analysis attributed the Benchmark harness (3 tiny servers + wrk driver) available on request; happy |
…, OWS) Fixes found by an adversarial review of the framing layer: - parse_content_length: guard against integer overflow. A Content-Length beyond i32 previously wrapped to a negative int, which framed the request as body-less and let the declared body be re-parsed as a new request (request-smuggling / truncation). Now rejected as 400. - frame_chunked_total: guard chunk-size accumulation against overflow (mirrors has_complete_chunked_body); a huge hex size no longer wraps negative and drives an out-of-bounds read. - frame_chunked_total: consume optional trailer field lines after the terminating zero chunk instead of requiring an immediate CRLF, so a chunked request with trailers completes instead of hanging until the read timeout. - Framing now recognizes bare-LF line endings consistently (CRLF or LF) and measures each header line's length CR-aware, matching the module's existing bare-LF-tolerant has_complete_body — closing a mismatch where a Content-Length on a bare-LF line was measured one byte short (under-framing the body). - line_header_value trims trailing (and tab) OWS around the value, so a Content-Length padded with whitespace is no longer rejected with a spurious 400. New tests cover each case (overflow, chunked trailers, bare-LF Content-Length, trailing OWS). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Pipelined-behind-a-file stranding (HIGH): a request pipelined right after a file (sendfile) response was left in read_buf and never answered — no new EPOLLIN edge fires for bytes already buffered — so it hung until a spurious 408. drain_requests now keeps draining after an inline file/keep-alive flush, and handle_writable re-drains buffered requests once a parked file batch finishes. New regression test covers file-response + pipelined follow-up. - 413 on an oversized head no longer discards responses already buffered for earlier pipelined requests: it appends the 413 and flushes in order, then closes. - w.parked is now decremented once per armed deadline (a connection can have both a read and a write deadline armed); it no longer leaks upward and forces a full-table timeout sweep every loop. - Document that a .manual takeover must be the sole response in a read burst (holds for veb, whose takeover is always the first request). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n BSD - veb: clone the file response path off the request arena AFTER leaving the scope. Previously ctl.file_path pointed into the arena, which the handler frees before the reactor opens the file — a use-after-free under -prealloc (the default GC build was unaffected). The reactor now always sees a live path. - Document that the BSD/kqueue and Windows/IOCP append paths allocate a fresh per-request `out` buffer that is not reclaimed under -prealloc (they do not yet reuse a persistent per-connection write buffer like the Linux backend); use the classic handler with -prealloc on those backends. The default GC build is fine. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
Self-review passI ran an adversarial review over the diff (reviewers per subsystem, each finding independently verified). It surfaced 12 issues; all are now fixed or documented in follow-up commits. Summary: Framing (
|
| # | Sev | Issue | Fix |
|---|---|---|---|
| 1 | critical | Content-Length ≥ 2³¹ wrapped negative → framed body-less → smuggling/truncation |
overflow guard → 400 |
| 2 | high | chunk-size hex accumulation could overflow → negative offset → OOB read | overflow guard → 400 |
| 3 | medium | chunked trailers not modeled → request with a trailer line never completed (hang) | consume trailer lines up to the blank line |
| 4 | medium | bare-LF line endings mis-measured a Content-Length line (under-framing) |
CR-aware line length + recognize bare-LF terminator (consistent with has_complete_body) |
| 5 | low | trailing OWS in a Content-Length value → spurious 400 |
trim leading/trailing OWS |
New tests cover each (overflow, chunked trailers, bare-LF, trailing OWS).
Linux reactor (fasthttp_linux.v) — commit 3286f23
| # | Sev | Issue | Fix |
|---|---|---|---|
| 6 | high | a request pipelined behind a file (sendfile) response was stranded in the read buffer → spurious 408 | drain continues after an inline file/keep-alive flush; handle_writable re-drains after a parked file batch finishes (+ regression test) |
| 7 | medium | 413 on an oversized head discarded responses already buffered for earlier pipelined requests | append the 413 and flush in order, then close |
| 8 | low | w.parked decremented once even when both read+write deadlines were armed → leaked upward |
decrement once per armed deadline |
| 9 | low | .manual takeover pipelined behind buffered responses drops them |
documented (takeover is always the first response in practice; holds for veb) |
veb + BSD/Windows — commit 332305f
| # | Sev | Issue | Fix |
|---|---|---|---|
| 10 | high | veb: ctl.file_path pointed into the request arena freed before the reactor opened the file → use-after-free under -prealloc (default GC build unaffected) |
clone the path off the arena after leaving the scope |
| 11 | medium | BSD append path's per-request out buffer not reclaimed under -prealloc |
documented as a known limitation (kqueue does not yet reuse a persistent write buffer; use the classic handler with -prealloc) |
| 12 | medium | Windows IOCP: same | documented (also Server.run is not implemented on Windows yet) |
Verification after fixes
v test vlib/fasthttp— 4 passed / 3 skipped (incl. new pipelined-behind-file + framing tests)v test vlib/veb— 31 passed / 1 skipped- Cross-compile:
v -os macosclean.
Rewrite the example to use the zero-copy append handler: the controllers append their raw HTTP response into the connection's reused `out` buffer and the handler returns a Step, instead of building and returning an HttpResponse. This showcases the recommended contract (no per-request response allocation). The example README is updated to match. The classic `handler` contract remains fully supported and documented in the module README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ackends
The pipelining commit gave the Linux reactor exact-length framing
(frame_request_length_lim_idx → buf_view(read_buf, pos, total)), so a request
body is trimmed to its declared Content-Length and any surplus begins the next
request on the connection (RFC 9112 §6). The BSD/kqueue and Windows/IOCP
backends were never updated: they passed the whole received buffer to
decode_http_request, so decoded.body spanned all received bytes rather than
exactly Content-Length.
That made veb's large_payload test's test_smaller_content_length fail on macOS
(and every BSD/Windows target): a 9-byte body sent with `Content-Length: 5`
was echoed back whole ('123456789') instead of trimmed to '12345'.
Frame the assembled request to its exact length before decoding on both
backends, mirroring the Linux reactor. Both serve one request per read, so a
trailing pipelined request is dropped (no batching there yet); a valid total is
always <= the buffer length because has_complete_body already gated on it, and
on a framing sentinel (< 0) the buffer is left intact for decode_http_request's
error path. Chunked and no-Content-Length requests already frame to the full
buffer, so trimming is a no-op for them.
Verified on macOS: vlib/veb (32/32) and vlib/fasthttp (5 passed / 2 skipped)
now pass, including large_payload's test_smaller_content_length.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`eventually()` only waited 30ms for a cancelled merged context's done channel to fire across goroutines. On a loaded tcc Windows CI runner that window is too tight, so the cross-goroutine cancellation occasionally does not propagate in time and `v test-self vlib` flaked here (gcc/msvc-windows passed the identical run). Give the positive `assert eventually(...)` checks a generous 2s budget -- the passing path still returns as soon as the channel fires, so there is no cost on green runs -- and keep a short 30ms probe for the negative `assert !eventually(...)` checks (which must also stay well under the 10s child deadline in test_merge_deadline_context_n). Also add a `// vtest retry: 3` guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Member
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a6754f36b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two P2 issues from the automated Codex review of the branch: - Zero-length file responses leaked a descriptor. open_deferred_file leaves cs.file_fd set with file_remaining == 0 for an empty file, and flush_batch's Phase-2 close was gated on `file_remaining > 0`, so the fd was never closed. On a keep-alive connection each empty-file response opened a new fd and the previous one was orphaned (only the last was reclaimed at close_conn). Gate the deferred-file drain/close on `file_fd != -1` in flush_batch, in drain_requests' pre-append flush, and at the batch boundary, so a zero-length file is drained (drain_file returns 1 immediately) and its fd closed before the next open_deferred_file runs. - Response order could reverse before a takeover. A .manual/.reusable takeover handler writes its response straight to the socket; if an earlier pipelined request in the same read burst still had its response buffered in write_buf, that buffered response would flush AFTER the takeover write, reversing HTTP/1.1 response order. The reactor documented (but did not enforce) that a takeover must be the sole response in its burst. Enforce it on both the append and legacy paths: if bytes are still buffered when a takeover is signalled, drop the connection instead of emitting responses out of order. In practice a takeover is always the first request on its connection, so this path is not hit by veb; it only fires on a peer that violates the contract. Adds white-box regression tests for both: two pipelined empty-file responses on one connection (fd reset each time, connection kept alive), and a normal request followed by a .reusable takeover in one burst (connection closed rather than reordered). Co-Authored-By: Claude Opus 4.8 (1M context) <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
2 participants
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.
Refactor
fasthttp(andveb's backend) toward the vanilla architecturevlib/fasthttpwas originally inspired by vanilla, which has since evolved a much faster server architecture. On the HttpArenajson-4096profile (identical JSON serialization on both), the gap lives entirely in the server layer:fasthttp(before)vanilla-epollThis PR ports vanilla's throughput architecture into
fasthttpwhile keeping the public API additive — breaking changes are confined to (a) fasthttp's own white-box tests and (b)veb's internal adapter.veb's user-facing API is unchanged.What changed (each commit is independently buildable)
request_parser.v) —frame_request_length*,frame_expected_total,frame_head_len; the pure functions the read loop uses to split pipelined requests and reassemble TCP-fragmented ones. Split-point fuzz tests included. Existing parser functions untouched.ConnStateobjects with reused read/write buffers (free-list, buffers retained across connections). HTTP/1.1 pipelining: many requests in one read → one batched write. Zero per-request buffer allocation / no map churn in steady state. White-box regression test rewritten to assert pooling, fragment reassembly, keep-alive re-arm, pipelining.ServerConfig.make_state fn () voidptr+HttpRequest.worker_state(additive).append_handler fn (req, mut out []u8, worker_state, mut ctl ResponseControl) Stepwrites the raw response straight into the reused write buffer (no response object, no copy).Step{done,close,suspend}+ResponseControl{takeover_mode, should_close, file_path}.handlerrelaxed from@[required];new_servervalidates exactly one is set.http.Responsedirectly into the reused buffer via the newhttp.Response.write_to, dropping the per-requestres.bytes()allocation + reactor copy. veb's public API unchanged; SSE/WebSocket takeover, static/sendfile, keep-alive all preserved. Addshttp.Response.write_to(net/http).make_state; cross-compiled (v -os macos/v -os windows) for fasthttp, veb, and the platform regression tests.v check-md).Behavior change (documented)
Because request bodies are now framed by their exact declared
Content-Length(RFC 9112 §6), a body longer thanContent-Lengthis trimmed to the declared length and the surplus begins the next request on the connection (this also closes a request-smuggling gap).veb'slarge_payloadtest is updated accordingly.Verification
v test vlib/fasthttp— 4 passed / 3 skipped (platform).v test vlib/veb— 31 passed / 1 skipped (incl. persistent-connection keep-alive + reusable takeover, SSE, static/sendfile, chunked upload, large payload, memory-leak).v test vlib/net/http/response_test.v— passed.v -os macosandv -os windowsfor fasthttp + veb + regression tests.🤖 Generated with Claude Code