Harden the connection path and clean up review findings#6
Merged
Conversation
A non-integer or empty Content-Length value made content_length/1 call binary_to_integer/1, which threw badarg and crashed the connection worker. A negative value parsed fine but later caused a badmatch when slicing the body. Both crashes propagated to the acceptor (see the spawn_link topology) and, repeated within the supervisor restart window, took down the whole application. Parse the value defensively and return bad_request (mapped to a 400) for anything that is not a non-negative integer.
Connection workers were spawned with spawn_link to the acceptor, which does not trap exits. Any abnormal worker exit (a misbehaving handler return shape, bad iodata in a response, an unforeseen parse path) thus propagated to the acceptor and every sibling connection. Repeated within the supervisor restart window it took the whole application down. Spawn workers unlinked with proc_lib:spawn/3 and transfer socket ownership to the worker via gen_tcp:controlling_process/2 right after accept, so a crash closes only that worker's own socket and leaves the acceptor running.
The [whitecap, connections, send_error] event has been emitted since 0.1.4 but was missing from whitecap:events/0 and the README telemetry table, so consumers attaching handlers via events/0 never observed send failures. List it in both.
A header line with a colon but no following space (e.g. "Host:x")
matched none of the parse_headers/2 clauses and raised case_clause.
Add a catch-all clause so the public headers/1 API returns
{error, invalid_headers} for any value that violates the strict
single-space rule.
Two keep-alive bugs at the connection limit: - The Connection: close injection matched the literal string key "Connection", so a handler that set a binary or differently-cased Connection header was not overwritten and a duplicate header was emitted. Replace overwrite_key/4 with force_connection_close/1, which drops any existing Connection header (case-insensitive, any iodata form) before prepending the close. - The limit check fired on N == max_keepalive after the counter had already advanced, serving max_keepalive + 1 requests. Use N + 1 >= max_keepalive so exactly max_keepalive requests are served before the forced close, matching the documented semantics.
bin_patterns was the only value kept in persistent_term while the rest of the config lived in foil. foil's compiled-module lookup is faster than persistent_term:get, and foil can carry the compiled patterns (which hold a reference) through its constant-pool path, so there is no reason to keep a second mechanism. Insert bin_patterns alongside the other config at init and read it through whitecap_config:get/1.
Order the README event table by event name so it matches the sorted whitecap:events/0 list.
start.sh pointed -pa at the compile profile, which does not contain the test_handler fixture, so the demo answered 500. Build the test profile and add its paths; the node now serves 200 on 8080. Add OTP 29 to the CI matrix to match the supported toolchain.
Pin the README install example to 0.1.5 as well.
whitecap does not use metal directly; it is a non-optional dependency of foil, which pulls it in transitively. Remove it from the deps and the application list and relock so metal is recorded at dependency level 1 instead of being declared at the top level.
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
A full review of the server turned up one critical robustness defect plus a set of smaller correctness, performance, and hygiene issues. Each is a separate commit.
Content-Length→ 400. A non-integer, empty, or negative value madebinary_to_integer/1throw (and a negative length later badmatched), crashing the connection worker. Repeated, this exhausted the supervisor restart window and took the whole application down. Such values now return400 Bad Request.spawn_linked to a non-trapping acceptor, so any abnormal worker exit killed the acceptor and every sibling connection. Workers now spawn unlinked viaproc_lib:spawn/3and own their socket throughgen_tcp:controlling_process/2, so a crash closes only that worker's socket. Covered by a burst e2e test.send_errortelemetry is now listed inwhitecap:events/0and the README table (emitted since 0.1.4 but unpublished).headers/1returns{error, invalid_headers}for a value that breaks the single-space rule instead of raisingcase_clause.max_keepaliverequests (was off by one), and the forcedConnection: closeoverwrites a handler-setConnectionheader of any casing/iodata form instead of duplicating it.bin_patternsmoved frompersistent_termtofoil, matching the rest of the config — foil's compiled-module lookup is faster and carries the compiled patterns through its constant pool.metalis no longer declared directly; it is pulled transitively throughfoiland relocked at dependency level 1.start.shnow builds the test profile so thetest_handlerdemo is on the path (serves 200 on 8080); OTP 29 added to the CI matrix; telemetry table sorted; version bumped to 0.1.5 with a CHANGELOG entry.Strict compile, eunit (28 tests), xref, and dialyzer all pass on OTP 29.