Reduce H2 header allocations - #13420
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces HTTP/2 per-request overhead in ATS by avoiding avoidable allocations/copies during HPACK processing and by introducing a fast-path that hands a decoded request header directly to HttpSM (skipping serialize + parse). It also updates the HTTP/2→1.1 conversion to normalize URL components to preserve legacy cache/remap behavior, and adds a gold test to cover the new path.
Changes:
- Decode contiguous HPACK header blocks in-place (avoids per-request malloc/memcpy/free) and encode HEADERS using an on-stack
ts::LocalBufferfor typical sizes. - Add a fast-path handoff to
HttpSMusing a borrowed pre-parsedHTTPHdr, skipping HTTP/1.1 serialization andparse_req. - Normalize
:authorityand:pathin the 2→1.1 converter (host:port split; query/fragment split; leading slash normalization) and add an AuTest replay to validate cache/remap parity.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/h2/replay/h2_request_handling.replay.yaml | New replay validating URL normalization and cache-key parity for the HTTP/2 fast path. |
| tests/gold_tests/h2/h2_request_handling.test.py | New gold test invoking the replay. |
| src/proxy/http2/Http2Stream.cc | Adds fast-path pre-parsed request handoff and strict-uri compliance checks. |
| src/proxy/http2/Http2ConnectionState.cc | In-place header-block decode when contiguous; stack-buffer HEADERS encoding via templated LocalBuffer. |
| src/proxy/http/HttpSM.cc | Consumes an optional borrowed pre-parsed request header instead of parsing from an IO buffer. |
| src/proxy/hdrs/VersionConverter.cc | Normalizes :authority and :path to match legacy serialize+reparse behavior (host/port + query/fragment splits). |
| src/proxy/hdrs/URL.cc | Adds url_is_uri_compliant() helper used for strict-uri checks on the fast path. |
| include/proxy/http2/Http2Stream.h | Adds decode_header_blocks() overload taking an explicit buffer pointer/length. |
| include/proxy/http/HttpSM.h | Adds pre-parsed request setter/query API and backing member. |
| include/proxy/hdrs/URL.h | Declares url_is_uri_compliant(). |
| include/proxy/hdrs/HdrHeap.h | Notes coupling between HdrHeap::DEFAULT_SIZE and the HTTP/2 on-stack encode buffer sizing. |
|
@JosiahWI Note that most of this got rewritten again, from suggestions from Masakazu. We'll have to rerun copilot etc. |
|
[approve ci autest] |
* Document HTTP methods for apache#13420 review * Make changes requested by Brian Neradt Put brief sentence on opening line Use in/out/in,out parameter markers Clarify that `@` headers are also included in length (cherry picked from commit e241265)
Decode HPACK header blocks in place when the whole block is present and contiguous in the frame reader, avoiding a per-request malloc + memcpy + free. Encode HEADERS frames into a stack-backed LocalBuffer (up to 2 * HdrHeap::DEFAULT_SIZE) so typical response headers skip a heap allocation, with a static_assert guarding the stack budget.
The H2 read path serialized each decoded request header so HttpSM could reparse it. Instead, normalize the URL in the 2->1.1 converter (split host:port and path?query, as a reparse would) and hand the decoded header to HttpSM via a refcounted copy(), skipping serialize+reparse. Roughly doubles small-request throughput. The fast path requires a successful 2->1.1 conversion (so a malformed request still gets its 400) and re-checks strict_uri_parsing on the target; non-compliant requests fall back to the serialize path. Header size stays bounded by the aggregate limits still in force here (SETTINGS_MAX_HEADER_LIST_SIZE and request_header_max_size); parse_req's per-field and request-line sub-limits are not separately applied.
Proxy Verifier replay coverage for the HTTP/2 <-> HttpSM fast-path handoff in all three directions: inbound request URL normalization (explicit-port and IPv6 :authority) and query-string / cross-protocol cache-key parity; client response emission (bodyless 204/304/HEAD and header preservation); and outbound server-request handling to an HTTP/2 origin (GET with query, POST with body). Extend http2_txn_start_read_gate with a bodyless GET. Its only transaction was a POST, so a dropped read event still had DATA frames to recover on, leaving the END_STREAM-on-HEADERS case uncovered. Also fix http2 test case 8, which piped curl's stderr with the bash 4 "|&" operator. Autest runs commands through /bin/sh, which is bash 3.2 on macOS, so that run died with a syntax error before reaching ATS.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
tests/gold_tests/h2/http2_fc_iso.test.py:74
- Typo in docstring: "paramenter" should be "parameter".
records.yaml file. If the paramenter is None, then no window size
tests/gold_tests/h2/http2_fc_iso.test.py:79
- Typo in docstring: "paramenter" should be "parameter".
records.yaml file. If the paramenter is None, then no policy
tests/gold_tests/h2/http2_fc_iso.test.py:132
- New Python test code should prefer f-strings over str.format() (this is also more consistent with the rest of this file’s use of f'' strings).
'proxy.config.ssl.server.private_key.path': f'{ts.Variables.SSLDir}',
'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE',
'proxy.config.dns.nameservers': '127.0.0.1:{0}'.format(self._dns.Variables.Port),
'proxy.config.dns.resolv_conf': 'NULL',
'proxy.config.http.insert_age_in_response': 0,
tests/gold_tests/h2/http2_fc_iso.test.py:69
- Typo in docstring: "paramenter" should be "parameter".
This issue also appears in the following locations of the same file:
- line 74
- line 79
records.yaml file. If the paramenter is None, then no window size
|
[approve ci autest] |
Replace the HttpSM::_pre_parsed_ua_request borrow pointer with virtual supports_direct_header_passing(), is_parsed_receive_header_ready() and parsed_receive_header() on ProxyTransaction, and use the same seam for the response and outbound-request directions. All three skip the serialize+reparse round-trip between HttpSM and the HTTP/2 stream. Request in: HttpSM pulls the decoded header from the transaction in state_read_client_request_header rather than the stream pushing a raw pointer. The stream owns _receive_header and is torn down with the SM, so the copy cannot see a dangling borrow; that drops the timeout null-out and the synchronous-delivery assert. Response out / request out: write_response_header_into_buffer and setup_server_send_request hand client_response / server_request to the stream instead of serializing them. update_write_request copies the fields onto _send_header, preserving the pseudo-headers that create(HTTP_2_0) installed and the 1.1->2 conversion fills -- a plain copy() would wipe them. A bodyless message (204/304/HEAD, or a GET to an H2 origin) has no body bytes to drive the write, so it is flushed via has_pending_send_header(). The ready flag re-arms per header so 1xx interim responses and retried requests each deliver. Because the header no longer passes through a buffer, everything that inferred it from one had to be corrected. client_response_hdr_bytes stays 0 on this path, so reported_client_response_hdr_bytes() serves the readers that mean "bytes the client received": logging, TSHttpTxnClientRespHdrBytesGet(), the size stats, and the tunnel_handler_post_ua guard that decides whether a response header has already gone out -- left raw, an early origin response followed by a post-body timeout would synthesize an error over it. Readers doing tunnel byte arithmetic keep the raw counter. The tunnel also identifies the UA consumer by comparing against get_ua_txn() rather than casting on vc_type, which response plugin agents share while carrying an INKVConnInternal. The interface design is adopted from Masakazu Kitajo's no-header-marshaling patch. The VersionConverter URL parity work and the H2 allocation reductions on this branch are unchanged.
Restore the inbound parse_resp() fallback in update_write_request(): the direct-header fast path only covers final responses, so serialized 1xx responses (100-continue, 103 early-hints) written by setup_100_continue_transfer() were dropped instead of being emitted as HEADERS frames. Gate the request fast path on the parse_req checks it would otherwise skip, falling back to serialize+parse when any of them would reject: the method must be all-token; a Content-Length must be a single 1*DIGIT with no differing duplicate and no Transfer-Encoding; and the Host built from :authority must satisfy http_parse_host_header(), the same test validate_hdr_host() applies. url_parse_internet() accepts the userinfo that RFC 9113 8.3.1 bans, so ":authority: user@host" would otherwise reach the origin instead of being rejected. Keep the fast path from mis-mapping an oversized header set to 414 (REQUEST_URI_TOO_LONG); it now returns a generic 400.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/proxy/hdrs/URL.cc:1220
url_is_uri_compliant()can be called with empty URL components (e.g. empty query/fragment), which yieldsvalue.data() == nullptr. In strict modes, this passes null pointers intourl_is_strictly_compliant()/url_is_mostly_compliant(), which currently iterate usingi < end; relational pointer comparisons on null pointers are undefined behavior. Add an early return for empty values (or otherwise avoid invoking the strict checks whenvalue.empty()).
url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
{
const char *start = value.data();
const char *end = start + value.length();
tests/gold_tests/h2/http2_fc_iso.test.py:80
- Typo in the docstring: "paramenter" should be "parameter" (appears multiple times in this block).
records.yaml file. If the paramenter is None, then no window size
bryancall
left a comment
bryancall
left a comment
There was a problem hiding this comment.
I read the whole diff and spent most of the time on the parts that could go wrong quietly. The core of this is sound, and a couple of pieces are better than I expected:
- The riskiest change, decoding HPACK straight out of the frame reader's IOBuffer instead of a stream-owned allocation that lived until stream teardown, is actually safe.
MIMEFieldWrapper::value_set/name_setcopy into the HdrHeap andXpackDynamicTable::insert_entrycopies via_storage.write(), so nothing retains a pointer into the frame buffer. The in-place branch is correctly restricted toend_headers, CONTINUATION still takes the realloc path, andheader_blocksis explicitly nulled on the trailer path so the destructor'sats_freestays correct. - The URL normalization parity work is careful and I could not find a divergence. The
#split before the?split reproducesurl_parse_http's scan for inputs like/p#f?q, andwhile (path.starts_with("/"))matches the remove-preceding-slashes loop, where a singleremove_prefixwould have changed the cache key for//foo. - Replacing
set_host(authority)withurl_parse_internet()plus a full-consumption check closes a real hole. Previously:authority: example.com/foowas stored whole as the host and reparsed into host plus path. It now returnsParseResult::ERRORand gets a 400, and it brings H2 CONNECT in line with how HTTP/1.1 parses authority-form targets. - The byte-accounting split is thought through rather than a blanket rename, with readers that mean "bytes the client received" using the reported accessor and readers doing tunnel arithmetic correctly keeping the raw counter.
- The unrelated
http2.test.pyfix is worth having on its own.|&is bash 4 syntax and autest runs through/bin/sh, which is bash 3.2 on macOS, so that case was dying on a syntax error before ever reaching ATS.
Requesting changes on two things, neither of which is a defect in the merged code path.
1. Leftover debugging file
tests/gold_tests/h2/http2_fc_iso.test.py
This is a 348-line scratch copy. I reconstructed it from the patch and diffed it against tests/gold_tests/h2/http2_flow_control.test.py: only 85 lines differ out of 348, and every difference is a deletion. The docstring is "Isolation repro: HTTP/2 flow control, policy 0, 500-byte window", the whole Http2FlowControlTest class is byte-identical including the "paramenter" typos, and the single surviving invocation is character-for-character the same one that already exists at the tail of the original file. So it is pure duplication with zero new coverage, it tests flow control rather than header handling, and it carries proxy.config.diags.debug.enabled: 3 with the http tag, which will produce large traffic.out files on every CI run and for every developer running the h2 gold tests. It is mentioned in neither the PR body nor any of the five commits.
It should just be deleted.
2. The fast-path rejection gates have no negative test
tests/gold_tests/h2/replay/h2_request_handling.replay.yaml
The gates added in 86afd77 are the entire safety argument for skipping parse_req: the token method check, http_parse_host_header on the synthesized Host, the single 1*DIGIT Content-Length with no Transfer-Encoding, and strict_uri_parsing. The Host gate in particular is load-bearing, since http_parse_host_header calls validate_host_name, which rejects the @ in user@host, which is exactly the case your commit message says it exists for.
Every proxy-response in the added replay is 200, 204 or 304. There is no expected 400 anywhere in the PR's test additions, and grepping the diff for user@, userinfo or Transfer-Encoding in test files finds nothing but Content-Length values on happy-path bodies. So if someone later reorders the && chain or hoists parse_req_would_accept() behind a flag, a request with :authority: attacker@origin.example.com reaches the origin unvalidated and CI stays fully green. That is the bug Copilot found and you fixed, silently reintroducible.
One negative scenario per gate would close it.
Also worth a note
src/proxy/http/HttpSM.cc:724 proxy.config.http.request_line_max_size silently stops applying to HTTP/2 on the fast path, since parse_req never runs and its limit argument is never consulted. An operator who sets it to 1024 gets a 4KB path rejected over HTTP/1.1 and proxied over HTTP/2, same URL and same ATS, with no log line explaining the difference. records.yaml.en.rst:1465 still states the limit without qualification. Documentation or a release note rather than a code change, I think, but the scope change should be written down somewhere.
The branch is also conflicting with master and will need a rebase.
Smaller observations, not blocking
include/proxy/http/HttpSM.h:646clear_pending_send_header()zeroes both the client-response and server-request ready flags, and the inbound and outbound streams share one HttpSM. Nothing in the current sequencing lets both be set at once, so this is latent, but it is a sharp edge for whoever next adds an internal retry or redirect after the response header is written.include/proxy/http/HttpSM.h:640reported_client_response_hdr_bytes()uses "raw counter is nonzero" as its sentinel for "the header went through the tunnel", whichsetup_blind_tunnelviolates by adding prewarm body bytes to that same counter. Withproxy.config.tunnel.prewarmon,pshlandTSHttpTxnClientRespHdrBytesGetcan report origin body bytes as response header size.src/proxy/hdrs/VersionConverter.cc:207url_parse_internet()is called with source pointers intoheader.m_heapwhile it writes host and port back into that same heap.set_host(heap, host, true)runs beforeset_port(heap, port, true), andHdrHeap::allocate_strcan reachcoalesce_str_heaps(), which drops the last reference to the old storage. I could not construct a reachable case, since it needs more thanMAX_LOST_STR_SPACEof freed string space accumulated during HPACK decode, but the aliasing shape is there and worth a second opinion from someone who knows the heap better than I do.
This reduces per-request HTTP/2 overhead in two independent steps. First, the HPACK header-block decode happens in place when the whole block is contiguous in the frame reader (avoiding a per-request malloc+memcpy+free), and HEADERS-frame encoding uses a stack buffer for typical header sizes. Second — the larger win — the decoded request header is handed directly to HttpSM instead of being serialized and reparsed: the 2→1.1 converter now normalizes the URL (splitting host:port and path?query the way the reparse did), so the pre-parsed header can be copy()'d into the state machine, skipping a full parse_req per request. In local benchmarking this roughly doubles small-request throughput (~800K → ~1.68M req/s).
Co-Author: Masakazu Kitajo