You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2026/07/23 02:36:47 UTC
[PR] Reduce H2 header allocations [trafficserver]
zwoop opened a new pull request, #13420:
URL: https://github.com/apache/trafficserver/pull/13420
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3648230387
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -2472,9 +2487,11 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream)
http2_convert_header_from_1_1_to_2(send_hdr);
}
- uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case
- ts::LocalBuffer local_buffer(buf_len);
- uint8_t *buf = local_buffer.data();
+ uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case
+ // Keep typical header sets off the heap; oversized ones spill to it.
+ static_assert(HdrHeap::DEFAULT_SIZE * 2 <= 8192, "keep HEADERS encode stack buffer within the event-thread stack budget");
+ ts::LocalBuffer<uint8_t, HdrHeap::DEFAULT_SIZE * 2> local_buffer(buf_len);
+ uint8_t *buf = local_buffer.data();
Review Comment:
no, it's setting the local (stack) buffer to 2x the default header heap size. So, for the "common" case, where things (hopefully) would have fit in a freelist header_heap, it now stays on the stack. The assert it to assure that future changes to the header_heap default size doesn't blow the stack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3686922668
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1188,6 +1188,22 @@ url_is_mostly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
+
+ switch (strict_uri_parsing) {
+ case 1:
+ return url_is_strictly_compliant(start, end);
+ case 2:
+ return url_is_mostly_compliant(start, end);
+ default:
+ return true;
+ }
+}
Review Comment:
You're right, and I was wrong to act on this. `nullptr + 0` is well-defined — [expr.add]/4 says adding 0 to a null pointer yields a null pointer, so `start + value.length()` with an empty view was never UB. The guard is reverted; the empty case already fell out of the `i < end` loop conditions in `url_is_strictly_compliant()` / `url_is_mostly_compliant()`, so behavior is identical either way and the branch was pure noise.
My mistake was taking the Copilot finding at face value instead of checking the standard. Thanks for the citation.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +365,50 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces a token method and Content-Length framing (RFC 9110 8.6),
+ // which the fast path would otherwise skip.
+ auto parse_req_would_accept = [&]() {
+ auto method{_receive_header.method_get()};
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ if (MIMEField *cl = _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_CONTENT_LENGTH)); cl != nullptr) {
+ auto value{cl->value_get()};
+ if (cl->has_dups() || value.empty() || std::any_of(value.begin(), value.end(), [](char c) { return c < '0' || c > '9'; }) ||
+ _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_TRANSFER_ENCODING)) != nullptr) {
+ return false;
+ }
+ }
+ return true;
+ };
Review Comment:
Confirmed and fixed. `url_parse_internet()` splits `user@host` into userinfo + host and validates only the host part, so the conversion succeeded and the Host header kept the raw `:authority` value — which then never reached `validate_hdr_host()`. The legacy path 400s it because `http_parse_host_header()` → `validate_host_name()` rejects '@' (not in `is_host_char`).
Rather than hand-roll the character check, `parse_req_would_accept()` now calls `http_parse_host_header()` on the Host value and rejects duplicates, which is exactly what `validate_hdr_host()` does — so parity is by construction rather than by a second implementation that can drift. Such requests fall back to serialize+parse and get the same 400 as before.
Worth noting the userinfo is itself an RFC 9113 8.3.1 violation (`:authority` MUST NOT include it), so rejecting is correct on two counts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3657151924
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1188,6 +1188,22 @@ url_is_mostly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
+
+ switch (strict_uri_parsing) {
+ case 1:
+ return url_is_strictly_compliant(start, end);
+ case 2:
+ return url_is_mostly_compliant(start, end);
+ default:
+ return true;
+ }
+}
Review Comment:
Dowvoting Copilot - Copilot is wrong. Adding a 0 offset to `nullptr` is defined behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#issuecomment-5139158838
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3674277938
##########
include/proxy/http/HttpSM.h:
##########
@@ -707,13 +730,30 @@ HttpSM::get_cache_sm()
inline int
HttpSM::write_response_header_into_buffer(HTTPHdr *h, MIOBuffer *b)
{
- if (t_state.client_info.http_version == HTTPVersion(0, 9)) {
+ if (_ua.get_txn()->supports_direct_header_passing()) {
+ // Nothing lands in the buffer, so 0 keeps the tunnel's byte math honest.
+ _client_response_header_is_ready = true;
+ _direct_response_hdr_bytes = h->length_get();
+ return 0;
+ } else if (t_state.client_info.http_version == HTTPVersion(0, 9)) {
return 0;
Review Comment:
Is this a dead code path now? Can we remove it?
##########
include/proxy/http2/Http2Stream.h:
##########
@@ -81,7 +81,14 @@ class Http2Stream : public ProxyTransaction
bool expect_receive_trailer() const override;
void set_expect_receive_trailer() override;
+ bool supports_direct_header_passing() const override;
+ bool is_parsed_receive_header_ready() const override;
+ const HTTPHdr *parsed_receive_header() const override;
+ bool has_pending_send_header() const override;
+
Http2ErrorCode decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size);
+ Http2ErrorCode decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size, const uint8_t *block,
Review Comment:
Nitpick: you could consider using `std::span` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4128115270
##########
tests/gold_tests/h2/h2_outbound_request_handling.test.py:
##########
@@ -0,0 +1,27 @@
+'''
+Verify HTTP/2 outbound (SM -> origin) server-request handling: the request
+header is handed to the outbound stream without a serialize+reparse.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
Review Comment:
This follows the existing layout in tests/gold_tests/h2, where 22 tests (including recent additions) put the module docstring first and the license block right after it. A module docstring has to be the first statement.
##########
tests/gold_tests/h2/h2_request_handling.test.py:
##########
@@ -0,0 +1,28 @@
+'''
+Verify HTTP/2 client request handling through the fast-path handoff to HttpSM.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
Review Comment:
This follows the existing layout in tests/gold_tests/h2, where 22 tests (including recent additions) put the module docstring first and the license block right after it. A module docstring has to be the first statement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3637820702
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+
+ if (_pre_parsed_ua_request != nullptr) {
+ // The UA_FIRST_READ gate above never fires on this path: the read buffer stays empty.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(_pre_parsed_ua_request);
+ _pre_parsed_ua_request = nullptr;
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
+ _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size,
+ t_state.http_config_param->http_hdr_field_max_size);
+ }
Review Comment:
Validation should be applied at parse-time, not on the use of the cached value. The cached value should invariantly already be validated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#issuecomment-5746879974
Checking in on this one. It has been a month since I requested changes, and the branch has gone to conflicting with master in the meantime, so it needs a rebase before it can be evaluated again.
Both things I asked for are small:
1. Delete `tests/gold_tests/h2/http2_fc_iso.test.py`. It is a 348-line scratch copy of `http2_flow_control.test.py` with no new coverage, and it carries `proxy.config.diags.debug.enabled: 3`, so it grows `traffic.out` on every CI run and for everyone running the h2 gold tests locally.
2. One negative scenario per fast-path gate in `h2_request_handling.replay.yaml`. Every proxy-response in the added replay is 200, 204 or 304, so there is no test that fails if the `&&` chain is later reordered and `:authority: attacker@origin.example.com` reaches the origin unvalidated.
None of that is a defect in the merged code path, and I want to be clear the review was positive on the substance: I read the whole diff, the in-place HPACK decode out of the frame reader is genuinely safe, and the authority parsing change closes a real hole.
I would rather this land than rot. If you are short of time, say so and I will look at whether the test gap can reasonably move to a follow-up issue so the rest is not held up. Are you still planning to take this one forward?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4147829841
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1222,14 +1222,27 @@ url_is_mostly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
+
+ switch (strict_uri_parsing) {
+ case 1:
+ return url_is_strictly_compliant(start, end);
+ case 2:
+ return url_is_mostly_compliant(start, end);
+ default:
+ return true;
+ }
+}
Review Comment:
std::string_view::data()` may be `nullptr` for an empty view (e.g., default-constructed), and the subsequent pointer arithmetic / relational comparisons inside the compliance helpers can become undefined behavior. This is particularly relevant because callers can legitimately pass empty path/query/fragment views. Fix by handling `value.empty()` early (e.g., return `true` for strictness levels that treat empty as acceptable, or pass a non-null stable pointer for the empty case) so `start/end` are always derived from a valid buffer.
##########
include/proxy/http/HttpSM.h:
##########
@@ -636,6 +636,12 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
IOBufferReader *_netvc_reader = nullptr;
MIOBuffer *_netvc_read_buffer = nullptr;
+ bool _client_response_header_is_ready = false;
+ bool _server_request_header_is_ready = false;
+
+ // Direct-passed headers bypass the tunnel, so client_response_hdr_bytes stays 0.
+ int _direct_response_hdr_bytes = 0;
+
Review Comment:
reported_client_response_hdr_bytes()` and `_direct_response_hdr_bytes` are `int`, but the surrounding counters/APIs are `int64_t` (e.g., `TSHttpTxnClientRespHdrBytesGet`, transaction logging). This can truncate large values and breaks type consistency. Use `int64_t` (or the same typedef/type as `client_response_hdr_bytes`) for `_direct_response_hdr_bytes` and make `reported_client_response_hdr_bytes()` return `int64_t` to avoid overflow/truncation.
##########
include/proxy/http/HttpSM.h:
##########
@@ -652,6 +658,25 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
int client_transaction_priority_weight() const;
int client_transaction_priority_dependence() const;
+ HTTPHdr *get_client_response_header();
+ HTTPHdr *get_server_request_header();
+
+ int
+ reported_client_response_hdr_bytes() const
+ {
+ return client_response_hdr_bytes + _direct_response_hdr_bytes;
+ }
Review Comment:
reported_client_response_hdr_bytes()` and `_direct_response_hdr_bytes` are `int`, but the surrounding counters/APIs are `int64_t` (e.g., `TSHttpTxnClientRespHdrBytesGet`, transaction logging). This can truncate large values and breaks type consistency. Use `int64_t` (or the same typedef/type as `client_response_hdr_bytes`) for `_direct_response_hdr_bytes` and make `reported_client_response_hdr_bytes()` return `int64_t` to avoid overflow/truncation.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,53 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped on this path, so re-apply its checks. Call it only after the REQUEST
+ // type check below: the accessors assert it.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const level = config->strict_uri_parsing;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || !std::ranges::all_of(method, &ParseRules::is_token)) {
+ return false;
+ }
+ if (!url_is_uri_compliant(level, _receive_header.path_get()) || !url_is_uri_compliant(level, _receive_header.query_get()) ||
+ !url_is_uri_compliant(level, _receive_header.fragment_get())) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops or rejects a name with any non-field-name character, and trims values.
+ if (name.empty() || name.size() + value.size() > field_max || !std::ranges::all_of(name, &ParseRules::is_http_field_name) ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) || ParseRules::is_ws(value.back())))) {
+ return false;
+ }
+ }
+ return validate_hdr_host(_receive_header.m_http) == ParseResult::DONE &&
+ validate_hdr_content_length(_receive_header.m_heap, _receive_header.m_http) == ParseResult::DONE;
+ };
Review Comment:
This embeds a sizable re-implementation of `parse_req`-adjacent validation in a lambda inside `send_headers()`, which will be hard to keep in sync with future parser/security changes. Consider extracting this into a shared helper (e.g., a utility function near `HTTPHdr::parse_req`/validation code or the version-converter layer) so it can be reused and updated centrally, and unit-tested directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4149553638
##########
include/proxy/http/HttpSM.h:
##########
@@ -652,6 +658,25 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
int client_transaction_priority_weight() const;
int client_transaction_priority_dependence() const;
+ HTTPHdr *get_client_response_header();
+ HTTPHdr *get_server_request_header();
+
+ int
+ reported_client_response_hdr_bytes() const
+ {
+ return client_response_hdr_bytes + _direct_response_hdr_bytes;
+ }
Review Comment:
Hmmm, e.g.
int
TSHttpTxnClientRespHdrBytesGet(TSHttpTxn txnp)
is int.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4151864170
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -200,8 +201,27 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr &header) const
// :authority
if (MIMEField *field = header.field_find(PSEUDO_HEADER_AUTHORITY);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
- auto authority{field->value_get()};
- header.m_http->u.req.m_url_impl->set_host(header.m_heap, authority, true);
+ auto value{field->value_get()};
+
+ // No host to parse, and url_parse_internet() reads *start before checking the length.
+ if (value.empty()) {
+ return ParseResult::ERROR;
+ }
+
+ // Copy out first: allocating from header.m_heap may coalesce it and free the field's storage.
+ ts::LocalBuffer<char> buf(value.length());
+ std::string_view authority{buf.data(), value.length()};
+
+ std::copy(value.begin(), value.end(), buf.data());
+
+ // Require full consumption: url_parse_internet() stops at a '/', '?' or '#'.
+ const char *astart = authority.data();
+ const char *aend = authority.data() + authority.length();
+
+ if (url_parse_internet(header.m_heap, header.m_http->u.req.m_url_impl, &astart, aend, true, true) != ParseResult::DONE ||
+ astart != aend) {
+ return ParseResult::ERROR;
+ }
Review Comment:
The parsed authority can still contain userinfo. For a CONNECT request there is no synthesized `Host` field, so `parse_req_would_accept()` never rejects it; with `strict_uri_parsing=1`, an authority such as `a"@example.com:443` now takes the direct path even though the former serialized request target was rejected by `parse_req()`. RFC 9113 also forbids userinfo in `:authority`. Reject a parsed user/password here so CONNECT requests fall back to the existing 400 path too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#issuecomment-5074512528
@JosiahWI Note that most of this got rewritten again, from suggestions from Masakazu. We'll have to rerun copilot etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3649600319
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1188,6 +1188,22 @@ url_is_mostly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
+
+ switch (strict_uri_parsing) {
+ case 1:
+ return url_is_strictly_compliant(start, end);
+ case 2:
+ return url_is_mostly_compliant(start, end);
+ default:
+ return true;
+ }
+}
Review Comment:
url_is_uri_compliant() does pointer arithmetic on value.data() even when the string_view is empty. In this codebase, URLImpl::get_query()/get_fragment() can yield std::string_view{nullptr, 0}, so start==nullptr and `start + value.length()` is undefined behavior. This is exercised by the new Http2Stream fast-path check, which calls url_is_uri_compliant() on empty query/fragment for typical requests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3657151924
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1188,6 +1188,22 @@ url_is_mostly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
+
+ switch (strict_uri_parsing) {
+ case 1:
+ return url_is_strictly_compliant(start, end);
+ case 2:
+ return url_is_mostly_compliant(start, end);
+ default:
+ return true;
+ }
+}
Review Comment:
Dowvoting Copilot - Copilot is wrong. Adding a 0 offset to `nullptr` is defined behavior. See [this SO answer](https://stackoverflow.com/a/59409094) which references the standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3638558481
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+
+ if (_pre_parsed_ua_request != nullptr) {
+ // The UA_FIRST_READ gate above never fires on this path: the read buffer stays empty.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(_pre_parsed_ua_request);
+ _pre_parsed_ua_request = nullptr;
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
Review Comment:
This computation of `bytes_used` is good. It is more accurate than the `parse_req` path, even.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#issuecomment-5074558005
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3684015957
##########
include/proxy/http2/Http2Stream.h:
##########
@@ -81,7 +81,14 @@ class Http2Stream : public ProxyTransaction
bool expect_receive_trailer() const override;
void set_expect_receive_trailer() override;
+ bool supports_direct_header_passing() const override;
+ bool is_parsed_receive_header_ready() const override;
+ const HTTPHdr *parsed_receive_header() const override;
+ bool has_pending_send_header() const override;
+
Http2ErrorCode decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size);
+ Http2ErrorCode decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size, const uint8_t *block,
Review Comment:
I think I did, and then it didn't compile on some of our older platforms. But I'll try again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4135425835
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +954,35 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_sm == nullptr ? nullptr :
+ this->is_outbound_connection() ? this->_sm->get_server_request_header() :
+ this->_sm->get_client_response_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
+ }
Review Comment:
This copies fields into `_send_header` without clearing any existing fields first. If the same `Http2Stream` sends multiple headers over its lifetime (e.g., retries, multiple responses on the outbound side, or any path that re-arms “pending send header”), this can accumulate/duplicate header fields in `_send_header`. Clear/reset `_send_header`’s existing field set (while preserving/re-creating required HTTP/2 pseudo headers) before attaching the new fields.
##########
include/proxy/http/HttpSM.h:
##########
@@ -636,6 +636,12 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
IOBufferReader *_netvc_reader = nullptr;
MIOBuffer *_netvc_read_buffer = nullptr;
+ bool _client_response_header_is_ready = false;
+ bool _server_request_header_is_ready = false;
+
+ // Direct-passed headers bypass the tunnel, so client_response_hdr_bytes stays 0.
+ int _direct_response_hdr_bytes = 0;
Review Comment:
Header byte counts can exceed `int` (e.g., very large response headers), and `TSHttpTxnClientRespHdrBytesGet()` returns `int64_t` while this helper returns `int`, risking truncation/overflow and inconsistent accounting. Use `int64_t` (or `uint64_t`) for `_direct_response_hdr_bytes` and for `reported_client_response_hdr_bytes()`’s return type to keep header byte accounting lossless.
##########
tests/gold_tests/h2/replay/http2_txn_start_read_gate.replay.yaml:
##########
@@ -60,3 +60,35 @@ sessions:
encoding: plain
data: response-body
verify: {as: equal}
+
+ # A bodyless GET arrives with END_STREAM on the HEADERS frame, so there are no
+ # DATA frames to fall back on if the read event is dropped while TXN_START is
+ # gated. Regression coverage for the direct-header-passing path.
+ - client-request:
+ frames:
+ - HEADERS:
+ headers:
+ fields:
+ - [:method, GET]
+ - [:scheme, https]
+ - [:authority, delay-txn-start.test]
+ - [:path, /read-gate-no-body]
Review Comment:
`:method`, `:scheme`, `:authority`, and `:path` are unquoted here, unlike the other replays in this PR (which use `":method"` etc). YAML plain scalars starting with `:` can be parsed unexpectedly or rejected depending on the YAML loader; quote these pseudo-header names to keep the replay format consistent and robust.
##########
include/proxy/http/HttpSM.h:
##########
@@ -652,6 +658,26 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
int client_transaction_priority_weight() const;
int client_transaction_priority_dependence() const;
+ HTTPHdr *get_client_response_header();
+ HTTPHdr *get_server_request_header();
+
+ // For logging/SDK: client_response_hdr_bytes counts only what the tunnel wrote.
+ int
+ reported_client_response_hdr_bytes() const
+ {
+ return client_response_hdr_bytes + _direct_response_hdr_bytes;
+ }
Review Comment:
Header byte counts can exceed `int` (e.g., very large response headers), and `TSHttpTxnClientRespHdrBytesGet()` returns `int64_t` while this helper returns `int`, risking truncation/overflow and inconsistent accounting. Use `int64_t` (or `uint64_t`) for `_direct_response_hdr_bytes` and for `reported_client_response_hdr_bytes()`’s return type to keep header byte accounting lossless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4137244520
##########
include/proxy/http/HttpSM.h:
##########
@@ -739,13 +765,30 @@ HttpSM::get_cache_sm()
inline int
HttpSM::write_response_header_into_buffer(HTTPHdr *h, MIOBuffer *b)
{
- if (t_state.client_info.http_version == HTTPVersion(0, 9)) {
+ if (_ua.get_txn()->supports_direct_header_passing()) {
Review Comment:
`setup_internal_transfer()` calls this helper before checking whether a client transaction exists. Scheduled-update transactions can have no UA, so dereferencing `_ua.get_txn()` here crashes instead of reaching the existing no-UA branch in `setup_internal_transfer()`. Use the buffered path when the transaction is null.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +964,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
Review Comment:
The direct-send branch skips the former `parse_req()`/`parse_resp()` of the finalized server request or client response. Those parsers checked `Content-Length` after header hooks; the request parser also checked `Host`. A hook can now add conflicting or malformed length fields (or an invalid Host) and have them encoded to an HTTP/2 peer, changing framing and request handling. Validate the finalized header by direction before taking this branch, and preserve the old parser/error handling when it is invalid.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,73 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing (RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops a line whose name starts with a non-token and trims whitespace around values.
+ if (name.empty() || name.size() + value.size() > field_max || (!ParseRules::is_token(name.front()) && name.front() != '@') ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) || ParseRules::is_ws(value.back())))) {
Review Comment:
An ordinary HTTP/2 field name can contain `:` after its first character; neither the header validator nor this gate rejects it. For example, `content-length:extra: 0` was split at the first colon by the old `parse_req()` path, but is copied as a distinct field name by this path. That changes request interpretation and can forward an invalid name to an HTTP/2 origin. Fall back to serialization and parsing for ordinary names containing `:` (or reject them at HTTP/2 validation).
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,73 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing (RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops a line whose name starts with a non-token and trims whitespace around values.
+ if (name.empty() || name.size() + value.size() > field_max || (!ParseRules::is_token(name.front()) && name.front() != '@') ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) || ParseRules::is_ws(value.back())))) {
+ return false;
+ }
+ }
+ // url_parse_internet() accepts the userinfo that RFC 9113 8.3.1 bans from :authority,
+ // and the Host built from it never sees validate_hdr_host(). Mirror that check here.
+ if (MIMEField *host = _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_HOST)); host != nullptr) {
+ std::string_view parsed_host;
+ int port = 0;
+ bool has_port = false;
+
+ if (host->has_dups() || !http_parse_host_header(host->value_get(), parsed_host, port, has_port)) {
+ return false;
+ }
+ }
+ return validate_hdr_content_length(_receive_header.m_heap, _receive_header.m_http) == ParseResult::DONE;
+ };
+
+ // A failed conversion leaves a \xffVOID method that only parse_req can turn into a 400.
+ if (conversion_ok && !this->trailing_header_is_possible() && !this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr && this->read_vio.nbytes > 0 && uri_ok() &&
+ parse_req_would_accept()) {
+ // The stream owns _receive_header and outlives the handoff, so the pulled pointer cannot dangle.
+ this->_is_parsed_receive_header_ready = true;
Review Comment:
If `signal_read_event()` cannot lock the HttpSM, the ready flag stays set until its deferred callback runs. Meanwhile the session can resume reading frames: a DATA frame marks later HEADERS as trailers, and `rcv_headers_frame()` calls `reset_receive_headers()` on the same `_receive_header`. HttpSM then copies the trailer (or an empty RESPONSE header) as its request. Preserve the initial request in a separate header until HttpSM has copied it, or prevent trailer decoding from replacing it while the handoff is pending.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +964,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
+ }
Review Comment:
Request or response hooks can add fields after the inbound request gate runs. The old `parse_req()`/`parse_resp()` trimmed leading and trailing spaces or tabs from their values; this direct copy keeps those bytes, so the emitted HTTP/2 header can be rejected by a peer. Trim surrounding SP/HT when copying each finalized field, as the old parser did.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3637818211
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +335,37 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // The conversion_ok gate keeps a \xffVOID method on the serialize path, where parse_req 400s it.
+ if (conversion_ok && !this->trailing_header_is_possible() && !this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr && this->read_vio.nbytes > 0 && uri_ok()) {
+ this->_sm->set_pre_parsed_ua_request(&_receive_header);
+ if (this->receive_end_stream) {
+ this->read_vio.nbytes = this->data_length;
+ this->read_vio.ndone = this->read_vio.nbytes;
+ this->signal_read_event(VC_EVENT_READ_COMPLETE);
+ } else {
+ this->has_body = true;
+ this->signal_read_event(VC_EVENT_READ_READY);
+ }
+ // Delivery is synchronous (stream mutex): the borrow is copied, or the txn
+ // finished and _sm is null. A still-pending borrow is unreachable; recover anyway.
+ if (this->_sm == nullptr || !this->_sm->has_pending_pre_parsed_ua_request()) {
+ return;
+ }
+ ink_assert(!"pre-parsed handoff was deferred");
+ this->_sm->set_pre_parsed_ua_request(nullptr);
+ }
Review Comment:
This one needs to be checked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3649548979
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -830,15 +896,36 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ // The SM's direct-passed header (request for an H2 origin, else response); not
+ // serialized, so there is nothing to parse.
+ HTTPHdr *send_hdr = this->_sm == nullptr ? nullptr :
+ this->is_outbound_connection() ? this->_sm->get_server_request_header() :
+ this->_sm->get_client_response_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
+ }
+ this->_sm->clear_pending_send_header();
+ state = ParseResult::DONE;
+ } else if (this->is_outbound_connection()) {
state = this->_send_header.parse_req(&http_parser, this->_send_reader, &bytes_used, false);
} else {
- state = this->_send_header.parse_resp(&http_parser, this->_send_reader, &bytes_used, false);
+ state = ParseResult::CONT;
}
Review Comment:
Reopening — confirmed regression, good catch. On the inbound path I dropped the `parse_resp()` fallback in favor of the new direct-passing setter (`get_client_response_header()`), but `HttpSM::setup_100_continue_transfer()` still serializes the interim response via `write_header_into_buffer()` instead of `write_response_header_into_buffer()`. So `_client_response_header_is_ready` is never set, `send_hdr == nullptr` in `update_write_request()`, and the serialized 100/103 sitting in `_send_reader` falls into the new `else { state = ParseResult::CONT; }` branch and is never emitted as a HEADERS frame.
Since 103 Early Hints and Expect:100-continue both route through `handle_100_continue_response` -> `INTERNAL_100_RESPONSE` -> `setup_100_continue_transfer`, this is the root cause of the `post-continue`, `early_hints`, and `httpbin` (Expect:100-continue) autest failures. Fix: restore the `parse_resp()` fallback for serialized interim responses (or route `setup_100_continue_transfer` through the direct-passing setter).
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+
+ if (_pre_parsed_ua_request != nullptr) {
+ // The UA_FIRST_READ gate above never fires on this path: the read buffer stays empty.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(_pre_parsed_ua_request);
+ _pre_parsed_ua_request = nullptr;
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
+ _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size,
+ t_state.http_config_param->http_hdr_field_max_size);
+ }
Review Comment:
Reopening to close the loop on validation parity. The fast path does re-apply the checks that matter most: `strict_uri_parsing` via `url_is_uri_compliant()` on path/query/fragment, host-character verification via `url_parse_internet(..., verify_host_characters=true)`, and a failed 2->1.1 conversion falls back to the serialize+parse path (invalid method -> 400).
Two `parse_req` checks are *not* reproduced on the fast path: the full `ParseRules::is_token` method check (we only reject control/WS chars via `value_is_valid(is_control_BIT | is_ws_BIT)`, not separators) and `validate_hdr_content_length` (duplicate/conflicting Content-Length). CR/LF are control chars and are rejected, so this is not a request-smuggling vector, but the method-token and Content-Length parity gaps are real. Need to confirm the H2 layer already rejects conflicting Content-Length before this point; will tighten the method check to `is_token` otherwise.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +627,23 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+ ProxyTransaction *ua_txn = _ua.get_txn();
+
+ if (ua_txn->supports_direct_header_passing() && ua_txn->is_parsed_receive_header_ready()) {
+ // UA_FIRST_READ never fires on this path (the read buffer stays empty), so stamp it here.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(ua_txn->parsed_receive_header());
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(
+ &http_parser, ua_txn->get_remote_reader(), &bytes_used, _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size, t_state.http_config_param->http_hdr_field_max_size);
+ }
client_request_hdr_bytes += bytes_used;
Review Comment:
Confirmed. On the fast path `bytes_used = client_request.length_get()` is the full header-set size, and the ParseResult::ERROR handler then uses `bytes_used` as a request-line-length surrogate, so an oversized H2 header set gets mapped to 414 (URI Too Long) instead of a generic 400. Fix: keep the `REQUEST_URI_TOO_LONG` heuristic off the fast path — track request-line length separately, or gate that branch on the serialize path.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +335,37 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // The conversion_ok gate keeps a \xffVOID method on the serialize path, where parse_req 400s it.
+ if (conversion_ok && !this->trailing_header_is_possible() && !this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr && this->read_vio.nbytes > 0 && uri_ok()) {
+ this->_sm->set_pre_parsed_ua_request(&_receive_header);
+ if (this->receive_end_stream) {
+ this->read_vio.nbytes = this->data_length;
+ this->read_vio.ndone = this->read_vio.nbytes;
+ this->signal_read_event(VC_EVENT_READ_COMPLETE);
+ } else {
+ this->has_body = true;
+ this->signal_read_event(VC_EVENT_READ_READY);
+ }
+ // Delivery is synchronous (stream mutex): the borrow is copied, or the txn
+ // finished and _sm is null. A still-pending borrow is unreachable; recover anyway.
+ if (this->_sm == nullptr || !this->_sm->has_pending_pre_parsed_ua_request()) {
+ return;
+ }
+ ink_assert(!"pre-parsed handoff was deferred");
+ this->_sm->set_pre_parsed_ua_request(nullptr);
+ }
Review Comment:
This no longer applies after the rewrite. The `set_pre_parsed_ua_request()` mechanism and its `ink_assert(!"pre-parsed handoff was deferred")` are gone; handoff is now a flag on the stream (`_is_parsed_receive_header_ready`) that the SM pulls via `parsed_receive_header()`. A deferred `signal_read_event()` (MUTEX_TRY_LOCK miss -> reschedule via `_read_vio_event`) just leaves the flag set for the next read — no assert, no crash. Keeping resolved.
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -201,7 +201,16 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr &header) const
if (MIMEField *field = header.field_find(PSEUDO_HEADER_AUTHORITY);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
auto authority{field->value_get()};
- header.m_http->u.req.m_url_impl->set_host(header.m_heap, authority, true);
+
+ // Match the reparse so url_print() stays byte-identical (legacy path
+ // unchanged); require full consumption, else a stray '/', '?' or '#' is dropped.
+ const char *astart = authority.data();
Review Comment:
The fast path skips the serialize+reparse round-trip, and that reparse used to normalize the URL a particular way. `url_print()` output — and therefore cache keys and remap matching — has to stay byte-identical, so the 2->1.1 converter now reproduces that normalization directly: full `:authority` consumption via `url_parse_internet` (erroring on a stray `/ ? #`), splitting `:path` into path/query/fragment, and stripping the leading `/` that `url_print` re-adds. And yes to your follow-up: these are 2->1.1 conversion fixes required to make the optimization behavior-preserving, not the optimization itself.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -283,10 +283,16 @@ Http2Stream::main_event_handler(int event, void *edata)
Http2ErrorCode
Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size)
+{
+ return this->decode_header_blocks(hpack_handle, maximum_table_size, header_blocks, header_blocks_length);
+}
+
+Http2ErrorCode
+Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size, const uint8_t *block, uint32_t block_len)
{
Http2ErrorCode error =
- http2_decode_header_blocks(&_receive_header, (const uint8_t *)header_blocks, header_blocks_length, nullptr, hpack_handle,
- _trailing_header_is_possible, maximum_table_size, this->is_outbound_connection());
+ http2_decode_header_blocks(&_receive_header, block, block_len, nullptr, hpack_handle, _trailing_header_is_possible,
+ maximum_table_size, this->is_outbound_connection());
Review Comment:
The explicit `(block, block_len)` overload lets us decode the header block in place from the contiguous HPACK buffer, avoiding the per-request malloc/memcpy/free into `header_blocks` that the member-based signature forced. That in-place decode is one of the main allocation savings in this PR.
##########
include/proxy/http/HttpSM.h:
##########
@@ -199,6 +199,20 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
void attach_client_session(ProxyTransaction *txn);
+ // Borrowed: must outlive the copy in state_read_client_request_header. Lets HTTP/2 skip serialize+reparse.
+ void
+ set_pre_parsed_ua_request(HTTPHdr *hdr)
Review Comment:
Moot after the rewrite — `set_pre_parsed_ua_request(HTTPHdr*)` is gone. The handoff is now via the `ProxyTransaction` interface, and the stream owns `_receive_header` for the lifetime of the borrow, so there is no ownership question left to solve with `shared_ptr`. Keeping resolved.
##########
include/proxy/hdrs/URL.h:
##########
@@ -215,6 +215,9 @@ ParseResult url_parse_http(HdrHeap *heap, URLImpl *url, const char **start, cons
bool verify_host_characters);
ParseResult url_parse_http_regex(HdrHeap *heap, URLImpl *url, const char **start, const char *end, bool copy_strings);
+// strict_uri_parsing 0 = no check; 1/2 apply the same test url_parse() does.
+bool url_is_uri_compliant(int strict_uri_parsing, std::string_view value);
Review Comment:
Agreed — moving `url_parse` to `std::string_view` (so it can reuse `url_is_uri_compliant`) is a good cleanup, but out of scope for this PR. Good follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3650485422
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -830,15 +896,36 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ // The SM's direct-passed header (request for an H2 origin, else response); not
+ // serialized, so there is nothing to parse.
+ HTTPHdr *send_hdr = this->_sm == nullptr ? nullptr :
+ this->is_outbound_connection() ? this->_sm->get_server_request_header() :
+ this->_sm->get_client_response_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
+ }
+ this->_sm->clear_pending_send_header();
+ state = ParseResult::DONE;
+ } else if (this->is_outbound_connection()) {
state = this->_send_header.parse_req(&http_parser, this->_send_reader, &bytes_used, false);
} else {
- state = this->_send_header.parse_resp(&http_parser, this->_send_reader, &bytes_used, false);
+ state = ParseResult::CONT;
}
Review Comment:
Fixed in the amended commit — the inbound `else` branch calls `parse_resp()` again instead of `state = ParseResult::CONT`, so serialized interim/legacy responses are emitted as HEADERS frames. Final responses still take the direct-passing `send_hdr` path.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +627,23 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+ ProxyTransaction *ua_txn = _ua.get_txn();
+
+ if (ua_txn->supports_direct_header_passing() && ua_txn->is_parsed_receive_header_ready()) {
+ // UA_FIRST_READ never fires on this path (the read buffer stays empty), so stamp it here.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(ua_txn->parsed_receive_header());
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(
+ &http_parser, ua_txn->get_remote_reader(), &bytes_used, _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size, t_state.http_config_param->http_hdr_field_max_size);
+ }
client_request_hdr_bytes += bytes_used;
Review Comment:
Fixed. The fast path now sets a `direct_header_passed` flag, and the `REQUEST_URI_TOO_LONG` branch in the `ParseResult::ERROR` handler is gated on `!direct_header_passed` — an oversized H2 header set now returns a generic 400 rather than 414.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+
+ if (_pre_parsed_ua_request != nullptr) {
+ // The UA_FIRST_READ gate above never fires on this path: the read buffer stays empty.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(_pre_parsed_ua_request);
+ _pre_parsed_ua_request = nullptr;
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
+ _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size,
+ t_state.http_config_param->http_hdr_field_max_size);
+ }
Review Comment:
Addressed. The fast-path gate in `Http2Stream::send_headers()` now also requires `parse_req_would_accept()`: the method must be all-token (`ParseRules::is_token`), and any `Content-Length` must be a single `1*DIGIT` value with no differing duplicate and no accompanying `Transfer-Encoding`. Anything else falls back to serialize+parse, where `parse_req`/`validate_hdr_content_length` adjudicate it exactly as before. Request-target and host-character checks were already re-applied via `url_is_uri_compliant()` and `url_parse_internet(..., verify_host_characters=true)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3686924535
##########
include/proxy/http/HttpSM.h:
##########
@@ -707,13 +730,30 @@ HttpSM::get_cache_sm()
inline int
HttpSM::write_response_header_into_buffer(HTTPHdr *h, MIOBuffer *b)
{
- if (t_state.client_info.http_version == HTTPVersion(0, 9)) {
+ if (_ua.get_txn()->supports_direct_header_passing()) {
+ // Nothing lands in the buffer, so 0 keeps the tunnel's byte math honest.
+ _client_response_header_is_ready = true;
+ _direct_response_hdr_bytes = h->length_get();
+ return 0;
+ } else if (t_state.client_info.http_version == HTTPVersion(0, 9)) {
return 0;
Review Comment:
Which of the two branches do you mean? Neither looks dead to me, so I'd rather ask than guess:
- The `supports_direct_header_passing()` branch is the new H2 path — it's what replaces the serialize for HTTP/2 responses.
- The `HTTPVersion(0, 9)` branch below is still reachable: `HttpSM.cc:7545` branches on `t_state.client_info.http_version == HTTP_0_9`, and `:7208` asserts it is *not* 0.9, so the value does occur for inbound clients.
If you meant something else — e.g. that the `MIOBuffer *b` parameter is now unused on the direct path — say so and I'll take another look.
##########
include/proxy/http2/Http2Stream.h:
##########
@@ -81,7 +81,14 @@ class Http2Stream : public ProxyTransaction
bool expect_receive_trailer() const override;
void set_expect_receive_trailer() override;
+ bool supports_direct_header_passing() const override;
+ bool is_parsed_receive_header_ready() const override;
+ const HTTPHdr *parsed_receive_header() const override;
+ bool has_pending_send_header() const override;
+
Http2ErrorCode decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size);
+ Http2ErrorCode decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size, const uint8_t *block,
Review Comment:
I tried again, and I'd rather not in this PR. `std::span<const uint8_t>` does compile locally (clang, C++20), but there is currently **no** `std::span` anywhere in `src/`, `include/`, or `plugins/` — this would be the first use in the tree, which matches my recollection of it not surviving the older platforms in CI.
Introducing it here means a portability question decided by a perf PR rather than on its own merits, so I've left the `(block, block_len)` pair. Happy to see it done tree-wide as its own change — the two-arg overload is a mechanical conversion at that point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4135752506
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -200,8 +201,21 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr &header) const
// :authority
if (MIMEField *field = header.field_find(PSEUDO_HEADER_AUTHORITY);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
- auto authority{field->value_get()};
- header.m_http->u.req.m_url_impl->set_host(header.m_heap, authority, true);
+ // Copy out first: allocating from header.m_heap may coalesce it and free the field's storage.
+ auto value{field->value_get()};
+ ts::LocalBuffer<char> buf(value.length());
Review Comment:
An empty `:authority` passes the HTTP/2 presence check and `value_is_valid()`. `LocalBuffer(0).data()` is null, but `url_parse_internet()` dereferences the start pointer before checking its length. An HTTP/2 client can therefore crash a worker with an empty authority. Return `ParseResult::ERROR` before constructing or parsing the buffer.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,68 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing (RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ if (field.name_get().size() + field.value_get().size() > field_max) {
+ return false;
+ }
+ }
Review Comment:
The HTTP/2 decoder checks name casing, and the converter rejects controls and whitespace, but neither checks ordinary field-name syntax. A name such as `x(foo` therefore reaches HttpSM on this direct path even though the former `parse_req()` path rejects it. Similarly, leading or trailing SP/HT in a value is preserved here but trimmed by the parser. Extend this gate to fall back to serialization and parsing for either case, so forwarded headers keep the former validation and normalization.
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -229,14 +245,26 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr &header) const
// :path
if (MIMEField *field = header.field_find(PSEUDO_HEADER_PATH);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
- auto path{field->value_get()};
+ auto path{field->value_get()};
+ auto *url = header.m_http->u.req.m_url_impl;
Review Comment:
`path` still views the `:path` field's storage when `set_fragment()` or `set_query()` allocates from the same `HdrHeap`. Those allocations can coalesce the string heaps and invalidate `path` before the next `find()` or `set_path()`, potentially corrupting the URL used for routing and caching. Copy the full field into a local buffer before splitting it, as the authority conversion above already does.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +959,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
Review Comment:
On the outbound direct path, a plugin can change `server_request` after the client header was parsed. `TSMimeHdrFieldNameSet` accepts a name such as `x(foo`, and this loop copies it without the old `parse_req()` validation. The HTTP/2 converter checks only controls and whitespace, so the encoder can send an invalid field name to the origin, which may reject the stream. Validate the finalized outbound fields with the parser's field-name rules, or serialize and reparse this header before sending it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4137599886
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,73 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing (RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops a line whose name starts with a non-token and trims whitespace around values.
+ if (name.empty() || name.size() + value.size() > field_max || (!ParseRules::is_token(name.front()) && name.front() != '@') ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) || ParseRules::is_ws(value.back())))) {
+ return false;
+ }
+ }
+ // url_parse_internet() accepts the userinfo that RFC 9113 8.3.1 bans from :authority,
+ // and the Host built from it never sees validate_hdr_host(). Mirror that check here.
+ if (MIMEField *host = _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_HOST)); host != nullptr) {
+ std::string_view parsed_host;
+ int port = 0;
+ bool has_port = false;
+
+ if (host->has_dups() || !http_parse_host_header(host->value_get(), parsed_host, port, has_port)) {
+ return false;
+ }
+ }
+ return validate_hdr_content_length(_receive_header.m_heap, _receive_header.m_http) == ParseResult::DONE;
+ };
+
+ // A failed conversion leaves a \xffVOID method that only parse_req can turn into a 400.
+ if (conversion_ok && !this->trailing_header_is_possible() && !this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr && this->read_vio.nbytes > 0 && uri_ok() &&
+ parse_req_would_accept()) {
+ // The stream owns _receive_header and outlives the handoff, so the pulled pointer cannot dangle.
+ this->_is_parsed_receive_header_ready = true;
Review Comment:
Fixed by taking the fast path only when HEADERS carries END_STREAM, so no trailer can follow and reset _receive_header before HttpSM copies it. Requests with a body use the serialize path.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +964,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
Review Comment:
Not changing this. Hook-set headers come from admin-controlled plugins. A conflicting Content-Length or invalid Host from a hook also failed on the previous path, and HTTP/1.1 forwards hook-set headers without validating them. For HTTP/2 the DATA frames define framing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4127925109
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7196,7 +7214,13 @@ HttpSM::setup_server_send_request()
// We need a reader so bytes don't fall off the end of
// the buffer
IOBufferReader *buf_start = server_entry->write_buffer->alloc_reader();
- server_request_hdr_bytes = hdr_length = write_header_into_buffer(&t_state.hdr_info.server_request, server_entry->write_buffer);
+
+ if (server_txn->supports_direct_header_passing()) {
+ _server_request_header_is_ready = true;
+ server_request_hdr_bytes = hdr_length = 0;
Review Comment:
Keeping `server_request_hdr_bytes` at zero makes an outbound HTTP/2 request look unsent to `HttpTransact::is_request_retryable()` (`HttpTransact.cc:6947-6949`). A failed POST/other unsafe request can therefore be retried after its HEADERS were sent, potentially duplicating side effects; it also makes `TSHttpTxnServerReqHdrBytesGet` and size stats report zero. Keep only the tunnel `hdr_length` at zero and record the logical header length separately.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +367,75 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing (RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ if (field.name_get().size() + field.value_get().size() > field_max) {
+ return false;
+ }
+ }
+ // url_parse_internet() accepts the userinfo that RFC 9113 8.3.1 bans from :authority,
+ // and the Host built from it never sees validate_hdr_host(). Mirror that check here.
+ if (MIMEField *host = _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_HOST)); host != nullptr) {
+ std::string_view parsed_host;
+ int port = 0;
+ bool has_port = false;
+
+ if (host->has_dups() || !http_parse_host_header(host->value_get(), parsed_host, port, has_port)) {
+ return false;
+ }
+ }
+ if (MIMEField *cl = _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_CONTENT_LENGTH)); cl != nullptr) {
+ auto value{cl->value_get()};
+ if (cl->has_dups() || value.empty() || std::any_of(value.begin(), value.end(), [](char c) { return c < '0' || c > '9'; }) ||
+ _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_TRANSFER_ENCODING)) != nullptr) {
+ return false;
+ }
+ }
Review Comment:
This digit-only check still fast-passes a `Content-Length` above `INT64_MAX`, whereas `validate_hdr_content_length()` rejects overflow. Such a request can reach HttpSM/origin before the eventual HTTP/2 payload-length check, and `get_content_length()` then saturates to `INT64_MAX`, changing the legacy immediate-400 behavior. Reuse the existing validator so overflow follows the serialize/reparse fallback too.
##########
tests/gold_tests/h2/h2_outbound_request_handling.test.py:
##########
@@ -0,0 +1,27 @@
+'''
+Verify HTTP/2 outbound (SM -> origin) server-request handling: the request
+header is handed to the outbound stream without a serialize+reparse.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
Review Comment:
New test files must place the Apache License header at the top (`AGENTS.md:293-296`); the module docstring currently precedes it. Move the docstring below the license block.
##########
tests/gold_tests/h2/h2_request_handling.test.py:
##########
@@ -0,0 +1,28 @@
+'''
+Verify HTTP/2 client request handling through the fast-path handoff to HttpSM.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
Review Comment:
New test files must place the Apache License header at the top (`AGENTS.md:293-296`); the module docstring currently precedes it. Move the docstring below the license block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#issuecomment-5880382679
@bryancall Thanks for the review. Rebased on master and addressed your comments in df68c52d66:
- Removed `http2_fc_iso.test.py`.
- Added a negative test for each fast-path check (userinfo in `:authority`, non-token method, bad Content-Length, non-printable path, oversized header field, and a request line over `request_line_max_size`).
- Rather than documenting the `request_line_max_size` gap, the fast path now falls back to `parse_req` near the request line and header field limits, so HTTP/2 matches HTTP/1.1.
- Fixed the three smaller items: `clear_pending_send_header()` is per direction, header byte counts are right with prewarm, and `:authority` is copied out before `url_parse_internet()` allocates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3638705277
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -201,7 +201,16 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr &header) const
if (MIMEField *field = header.field_find(PSEUDO_HEADER_AUTHORITY);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
auto authority{field->value_get()};
- header.m_http->u.req.m_url_impl->set_host(header.m_heap, authority, true);
+
+ // Match the reparse so url_print() stays byte-identical (legacy path
+ // unchanged); require full consumption, else a stray '/', '?' or '#' is dropped.
+ const char *astart = authority.data();
Review Comment:
It seems to me like these are fixes to the HTTP/2 to HTTP/1.1 header conversion, which aren't directly part of the optimization itself. Is that right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3637665969
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1188,6 +1188,22 @@ url_is_mostly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
Review Comment:
You could alternatively use `value.begin` and `value.end`, but what you did is correct.
##########
src/proxy/hdrs/VersionConverter.cc:
##########
@@ -201,7 +201,16 @@ VersionConverter::_convert_req_from_2_to_1(HTTPHdr &header) const
if (MIMEField *field = header.field_find(PSEUDO_HEADER_AUTHORITY);
field != nullptr && field->value_is_valid(is_control_BIT | is_ws_BIT)) {
auto authority{field->value_get()};
- header.m_http->u.req.m_url_impl->set_host(header.m_heap, authority, true);
+
+ // Match the reparse so url_print() stays byte-identical (legacy path
+ // unchanged); require full consumption, else a stray '/', '?' or '#' is dropped.
+ const char *astart = authority.data();
Review Comment:
It's still not quite clear to me why the `url_print` output would have changed.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +335,37 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // The conversion_ok gate keeps a \xffVOID method on the serialize path, where parse_req 400s it.
+ if (conversion_ok && !this->trailing_header_is_possible() && !this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr && this->read_vio.nbytes > 0 && uri_ok()) {
+ this->_sm->set_pre_parsed_ua_request(&_receive_header);
+ if (this->receive_end_stream) {
+ this->read_vio.nbytes = this->data_length;
+ this->read_vio.ndone = this->read_vio.nbytes;
+ this->signal_read_event(VC_EVENT_READ_COMPLETE);
+ } else {
+ this->has_body = true;
+ this->signal_read_event(VC_EVENT_READ_READY);
+ }
+ // Delivery is synchronous (stream mutex): the borrow is copied, or the txn
+ // finished and _sm is null. A still-pending borrow is unreachable; recover anyway.
+ if (this->_sm == nullptr || !this->_sm->has_pending_pre_parsed_ua_request()) {
+ return;
+ }
+ ink_assert(!"pre-parsed handoff was deferred");
+ this->_sm->set_pre_parsed_ua_request(nullptr);
+ }
Review Comment:
This one needs to be checked.
##########
include/proxy/hdrs/URL.h:
##########
@@ -215,6 +215,9 @@ ParseResult url_parse_http(HdrHeap *heap, URLImpl *url, const char **start, cons
bool verify_host_characters);
ParseResult url_parse_http_regex(HdrHeap *heap, URLImpl *url, const char **start, const char *end, bool copy_strings);
+// strict_uri_parsing 0 = no check; 1/2 apply the same test url_parse() does.
+bool url_is_uri_compliant(int strict_uri_parsing, std::string_view value);
Review Comment:
It's out of scope for this PR, but follow-up work should update the `url_parse` API to take a `std::string_view` for consistency (and then it can call this helper).
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+
+ if (_pre_parsed_ua_request != nullptr) {
+ // The UA_FIRST_READ gate above never fires on this path: the read buffer stays empty.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(_pre_parsed_ua_request);
+ _pre_parsed_ua_request = nullptr;
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
+ _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size,
+ t_state.http_config_param->http_hdr_field_max_size);
+ }
Review Comment:
Validation should be applied at parse-time, not on the use of the cached value. The cached value should invariantly already be validated.
##########
include/proxy/http/HttpSM.h:
##########
@@ -199,6 +199,20 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
void attach_client_session(ProxyTransaction *txn);
+ // Borrowed: must outlive the copy in state_read_client_request_header. Lets HTTP/2 skip serialize+reparse.
+ void
+ set_pre_parsed_ua_request(HTTPHdr *hdr)
Review Comment:
It's out of scope for this PR, but if performance allows, it would be nice to take a `std::shared_ptr` here. The comment is good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3637788614
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +335,37 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // The conversion_ok gate keeps a \xffVOID method on the serialize path, where parse_req 400s it.
+ if (conversion_ok && !this->trailing_header_is_possible() && !this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr && this->read_vio.nbytes > 0 && uri_ok()) {
+ this->_sm->set_pre_parsed_ua_request(&_receive_header);
+ if (this->receive_end_stream) {
+ this->read_vio.nbytes = this->data_length;
+ this->read_vio.ndone = this->read_vio.nbytes;
+ this->signal_read_event(VC_EVENT_READ_COMPLETE);
+ } else {
+ this->has_body = true;
+ this->signal_read_event(VC_EVENT_READ_READY);
+ }
+ // Delivery is synchronous (stream mutex): the borrow is copied, or the txn
+ // finished and _sm is null. A still-pending borrow is unreachable; recover anyway.
+ if (this->_sm == nullptr || !this->_sm->has_pending_pre_parsed_ua_request()) {
+ return;
+ }
+ ink_assert(!"pre-parsed handoff was deferred");
+ this->_sm->set_pre_parsed_ua_request(nullptr);
+ }
Review Comment:
This one needs to be checked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3637769101
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+
+ if (_pre_parsed_ua_request != nullptr) {
+ // The UA_FIRST_READ gate above never fires on this path: the read buffer stays empty.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(_pre_parsed_ua_request);
+ _pre_parsed_ua_request = nullptr;
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
+ _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size,
+ t_state.http_config_param->http_hdr_field_max_size);
+ }
Review Comment:
Validation should be applied at parse-time, not on the use of the cached value. The cached value should invariantly already be validated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3637884499
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -452,11 +452,22 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame)
if (stream->trailing_header_is_possible()) {
// Don't leak the header_blocks from the initial, non-trailing headers.
ats_free(stream->header_blocks);
+ stream->header_blocks = nullptr;
Review Comment:
Setting `nullptr` is necessary semantically, because `header_blocks` might not be touched after this if the header block can be read in-place.
##########
include/proxy/hdrs/HdrHeap.h:
##########
@@ -175,6 +175,7 @@ struct StrHeapDesc {
class HdrHeap
{
public:
+ // NOTE: also sizes an on-stack HEADERS encode buffer (2*this) in HTTP/2; a static_assert bounds the growth.
Review Comment:
Good comment.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -283,10 +283,16 @@ Http2Stream::main_event_handler(int event, void *edata)
Http2ErrorCode
Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size)
+{
+ return this->decode_header_blocks(hpack_handle, maximum_table_size, header_blocks, header_blocks_length);
+}
+
+Http2ErrorCode
+Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t maximum_table_size, const uint8_t *block, uint32_t block_len)
{
Http2ErrorCode error =
- http2_decode_header_blocks(&_receive_header, (const uint8_t *)header_blocks, header_blocks_length, nullptr, hpack_handle,
- _trailing_header_is_possible, maximum_table_size, this->is_outbound_connection());
+ http2_decode_header_blocks(&_receive_header, block, block_len, nullptr, hpack_handle, _trailing_header_is_possible,
+ maximum_table_size, this->is_outbound_connection());
Review Comment:
What's the reason for adding the 4-argument overload?
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -2472,9 +2487,11 @@ Http2ConnectionState::send_headers_frame(Http2Stream *stream)
http2_convert_header_from_1_1_to_2(send_hdr);
}
- uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case
- ts::LocalBuffer local_buffer(buf_len);
- uint8_t *buf = local_buffer.data();
+ uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case
+ // Keep typical header sets off the heap; oversized ones spill to it.
+ static_assert(HdrHeap::DEFAULT_SIZE * 2 <= 8192, "keep HEADERS encode stack buffer within the event-thread stack budget");
+ ts::LocalBuffer<uint8_t, HdrHeap::DEFAULT_SIZE * 2> local_buffer(buf_len);
+ uint8_t *buf = local_buffer.data();
Review Comment:
Do I understand correctly that you are increasing the buffer capacity here to increase the chance that typical response headers will fit in the buffer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3648274202
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +627,23 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+ ProxyTransaction *ua_txn = _ua.get_txn();
+
+ if (ua_txn->supports_direct_header_passing() && ua_txn->is_parsed_receive_header_ready()) {
+ // UA_FIRST_READ never fires on this path (the read buffer stays empty), so stamp it here.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(ua_txn->parsed_receive_header());
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(
+ &http_parser, ua_txn->get_remote_reader(), &bytes_used, _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size, t_state.http_config_param->http_hdr_field_max_size);
+ }
client_request_hdr_bytes += bytes_used;
Review Comment:
On the direct-header-passing path, `bytes_used` is set to `client_request.length_get()`. That value is later interpreted as a parse-consumption/request-line length surrogate (e.g., to decide whether to use `REQUEST_URI_TOO_LONG` in the ParseResult::ERROR path). If the request is rejected due to `request_hdr_max_size`, this can incorrectly map an oversized header set to a request-line-too-long error. Consider tracking header bytes separately and leaving `bytes_used` as the parser-produced value (or 0 on the fast path).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3635219921
##########
src/proxy/http/HttpSM.cc:
##########
@@ -627,10 +628,24 @@ HttpSM::state_read_client_request_header(int event, void *data)
// tokenize header //
/////////////////////
- ParseResult state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
- _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
- t_state.http_config_param->http_request_line_max_size,
- t_state.http_config_param->http_hdr_field_max_size);
+ ParseResult state;
+
+ if (_pre_parsed_ua_request != nullptr) {
+ // The UA_FIRST_READ gate above never fires on this path: the read buffer stays empty.
+ if (milestones[TS_MILESTONE_UA_FIRST_READ] == 0) {
+ ATS_PROBE1(milestone_ua_first_read, sm_id);
+ milestones[TS_MILESTONE_UA_FIRST_READ] = ink_get_hrtime();
+ }
+ t_state.hdr_info.client_request.copy(_pre_parsed_ua_request);
+ _pre_parsed_ua_request = nullptr;
+ bytes_used = t_state.hdr_info.client_request.length_get();
+ state = ParseResult::DONE;
+ } else {
+ state = t_state.hdr_info.client_request.parse_req(&http_parser, _ua.get_txn()->get_remote_reader(), &bytes_used,
+ _ua.get_entry()->eos, t_state.http_config_param->strict_uri_parsing,
+ t_state.http_config_param->http_request_line_max_size,
+ t_state.http_config_param->http_hdr_field_max_size);
+ }
Review Comment:
Fast-path pre-parsed request copy bypasses the validations that http_parser_parse_req() normally applies (method token character check via ParseRules::is_token, plus validate_hdr_request_target/validate_hdr_host/validate_hdr_content_length). This changes behavior relative to the serialize+reparse path and can allow malformed requests (e.g., non-token :method bytes or invalid/mismatched Content-Length) to be accepted on the HTTP/2 fast path.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +335,37 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // The conversion_ok gate keeps a \xffVOID method on the serialize path, where parse_req 400s it.
+ if (conversion_ok && !this->trailing_header_is_possible() && !this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr && this->read_vio.nbytes > 0 && uri_ok()) {
+ this->_sm->set_pre_parsed_ua_request(&_receive_header);
+ if (this->receive_end_stream) {
+ this->read_vio.nbytes = this->data_length;
+ this->read_vio.ndone = this->read_vio.nbytes;
+ this->signal_read_event(VC_EVENT_READ_COMPLETE);
+ } else {
+ this->has_body = true;
+ this->signal_read_event(VC_EVENT_READ_READY);
+ }
+ // Delivery is synchronous (stream mutex): the borrow is copied, or the txn
+ // finished and _sm is null. A still-pending borrow is unreachable; recover anyway.
+ if (this->_sm == nullptr || !this->_sm->has_pending_pre_parsed_ua_request()) {
+ return;
+ }
+ ink_assert(!"pre-parsed handoff was deferred");
+ this->_sm->set_pre_parsed_ua_request(nullptr);
+ }
Review Comment:
The fast-path assumes synchronous delivery of the pre-parsed header, but Http2Stream::signal_read_event() uses MUTEX_TRY_LOCK and can defer delivery via _read_vio_event. In that case this ink_assert("pre-parsed handoff was deferred") is reachable under normal mutex contention and can crash debug builds. Treat deferral as a normal fallback (clear the borrow and continue with serialize+reparse) without asserting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3650484437
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1188,6 +1188,22 @@ url_is_mostly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
+
+ switch (strict_uri_parsing) {
+ case 1:
+ return url_is_strictly_compliant(start, end);
+ case 2:
+ return url_is_mostly_compliant(start, end);
+ default:
+ return true;
+ }
+}
Review Comment:
Fixed. `url_is_uri_compliant()` now returns early on an empty view before touching `value.data()`, so the `{nullptr, 0}` that `URLImpl::get_query()`/`get_fragment()` return for an unset component no longer reaches the pointer arithmetic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3652966153
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +365,50 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a REQUEST.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces a token method and Content-Length framing (RFC 9110 8.6),
+ // which the fast path would otherwise skip.
+ auto parse_req_would_accept = [&]() {
+ auto method{_receive_header.method_get()};
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ if (MIMEField *cl = _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_CONTENT_LENGTH)); cl != nullptr) {
+ auto value{cl->value_get()};
+ if (cl->has_dups() || value.empty() || std::any_of(value.begin(), value.end(), [](char c) { return c < '0' || c > '9'; }) ||
+ _receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_TRANSFER_ENCODING)) != nullptr) {
+ return false;
+ }
+ }
+ return true;
+ };
Review Comment:
The HTTP/2 fast path currently bypasses `parse_req`/`validate_hdr_host`, but the fast-path gate (`parse_req_would_accept`) doesn’t validate the synthesized `Host` header at all. Because `:authority` is parsed with `url_parse_internet()` (which allows RFC3986 userinfo), a request like `:authority: user@host` can pass the fast-path checks even though `validate_host_name()` (used by `validate_hdr_host`) would reject the literal '@' in `Host` and the legacy path would 400 it. Add a Host-header parity check (at least reject duplicates/empty and any '@' in the Host value) so such requests fall back to the serialize+parse path and keep behavior consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3647772045
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -830,15 +896,36 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ // The SM's direct-passed header (request for an H2 origin, else response); not
+ // serialized, so there is nothing to parse.
+ HTTPHdr *send_hdr = this->_sm == nullptr ? nullptr :
+ this->is_outbound_connection() ? this->_sm->get_server_request_header() :
+ this->_sm->get_client_response_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
+ }
+ this->_sm->clear_pending_send_header();
+ state = ParseResult::DONE;
+ } else if (this->is_outbound_connection()) {
state = this->_send_header.parse_req(&http_parser, this->_send_reader, &bytes_used, false);
} else {
- state = this->_send_header.parse_resp(&http_parser, this->_send_reader, &bytes_used, false);
+ state = ParseResult::CONT;
}
Review Comment:
When there is no direct-passed SM header (send_hdr == nullptr) on an inbound stream, update_write_request() now sets state=ParseResult::CONT and never attempts to parse a serialized HTTP/1.1 response header from _send_reader. This breaks existing paths that still serialize response headers into the tunnel buffer (e.g., 100 Continue via HttpSM::setup_100_continue_transfer() writes into a buffer and expects Http2Stream to parse+convert it). Restore the parse_resp() fallback so interim/legacy serialized responses can still be emitted as HTTP/2 HEADERS frames.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4137145989
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +954,35 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_sm == nullptr ? nullptr :
+ this->is_outbound_connection() ? this->_sm->get_server_request_header() :
+ this->_sm->get_client_response_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
+ }
Review Comment:
_send_header can't accumulate: update_write_request() destroys and recreates it after every outbound send and every 1xx response, and a final inbound response sets parsing_header_done, so no second header follows it.
##########
include/proxy/http/HttpSM.h:
##########
@@ -636,6 +636,12 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
IOBufferReader *_netvc_reader = nullptr;
MIOBuffer *_netvc_read_buffer = nullptr;
+ bool _client_response_header_is_ready = false;
+ bool _server_request_header_is_ready = false;
+
+ // Direct-passed headers bypass the tunnel, so client_response_hdr_bytes stays 0.
+ int _direct_response_hdr_bytes = 0;
Review Comment:
This matches the counter it complements: every header byte counter in HttpSM (client_request_hdr_bytes, server_request_hdr_bytes, client_response_hdr_bytes) is an int, and header sizes are capped far below 2 GiB.
##########
include/proxy/http/HttpSM.h:
##########
@@ -652,6 +658,26 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
int client_transaction_priority_weight() const;
int client_transaction_priority_dependence() const;
+ HTTPHdr *get_client_response_header();
+ HTTPHdr *get_server_request_header();
+
+ // For logging/SDK: client_response_hdr_bytes counts only what the tunnel wrote.
+ int
+ reported_client_response_hdr_bytes() const
+ {
+ return client_response_hdr_bytes + _direct_response_hdr_bytes;
+ }
Review Comment:
Same as the other thread: all of HttpSM's header byte counters are int, and header sizes are capped far below 2 GiB.
##########
tests/gold_tests/h2/replay/http2_txn_start_read_gate.replay.yaml:
##########
@@ -60,3 +60,35 @@ sessions:
encoding: plain
data: response-body
verify: {as: equal}
+
+ # A bodyless GET arrives with END_STREAM on the HEADERS frame, so there are no
+ # DATA frames to fall back on if the read event is dropped while TXN_START is
+ # gated. Regression coverage for the direct-header-passing path.
+ - client-request:
+ frames:
+ - HEADERS:
+ headers:
+ fields:
+ - [:method, GET]
+ - [:scheme, https]
+ - [:authority, delay-txn-start.test]
+ - [:path, /read-gate-no-body]
Review Comment:
Unquoted pseudo-header names are used by many existing replays in tests/gold_tests/h2, both the YAML loader and Proxy Verifier accept them, and this test passes.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,68 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing (RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ if (field.name_get().size() + field.value_get().size() > field_max) {
+ return false;
+ }
+ }
Review Comment:
parse_req() accepts x(foo too, since it only checks a field name's first character. The gate now falls back to parse_req for a name whose first character isn't a token character, and for a value with leading or trailing whitespace, with tests for both.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +959,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
Review Comment:
The previous outbound path forwarded x(foo as well, since parse_req only checks a name's first character, and the converter still rejects control characters and whitespace. A plugin-set field name is admin-controlled input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4137534868
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7196,7 +7213,15 @@ HttpSM::setup_server_send_request()
// We need a reader so bytes don't fall off the end of
// the buffer
IOBufferReader *buf_start = server_entry->write_buffer->alloc_reader();
- server_request_hdr_bytes = hdr_length = write_header_into_buffer(&t_state.hdr_info.server_request, server_entry->write_buffer);
+
+ if (server_txn->supports_direct_header_passing()) {
+ _server_request_header_is_ready = true;
+ // Only the write buffer is empty; retry decisions and stats need the real size.
+ server_request_hdr_bytes = t_state.hdr_info.server_request.length_get();
+ hdr_length = 0;
Review Comment:
For an outbound HTTP/2 request without a buffered body, `hdr_length = 0` makes the write VIO's `nbytes` zero even while its HEADERS frame is still pending. If inactivity times out before the stream emits HEADERS, `handle_server_setup_error()` checks `nbytes > 0` and labels it `INACTIVE_TIMEOUT`; `is_request_retryable()` then refuses to retry a POST because `server_request_hdr_bytes` is nonzero, although nothing reached the origin. Track whether the outbound header was actually emitted and use that state for timeout classification, while retaining the no-retry behavior once it has been sent.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,70 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here, so re-apply its checks. Both run after the REQUEST type check
+ // below, since the accessors assert that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops a line whose name starts with a non-token and trims whitespace around values.
+ if (name.empty() || name.size() + value.size() > field_max || (!ParseRules::is_token(name.front()) && name.front() != '@') ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) || ParseRules::is_ws(value.back())))) {
Review Comment:
Checking only the first character lets an HTTP/2 field such as `x(foo` take the direct path. The old `parse_req()` path calls `mime_parser_parse()`, which rejects `(` in a field name via `ParseRules::is_http_field_name()` (`src/proxy/hdrs/MIME.cc:2602-2607`); the new `(foo` test covers only invalid *first* characters. Validate every character before passing the decoded header to HttpSM so this malformed field falls back to the former parser/rejection path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4138603214
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,70 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here, so re-apply its checks. Both run after the REQUEST type check
+ // below, since the accessors assert that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops a line whose name starts with a non-token and trims whitespace around values.
+ if (name.empty() || name.size() + value.size() > field_max || (!ParseRules::is_token(name.front()) && name.front() != '@') ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) || ParseRules::is_ws(value.back())))) {
Review Comment:
Right, thanks. mime_parser_parse() checks every character of a name that isn't a well-known header. The gate now falls back to parse_req for any name with a non-field-name character, which also covers ':'. Added an x(foo case that expects a 400 and checks the origin never sees it.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7196,7 +7213,15 @@ HttpSM::setup_server_send_request()
// We need a reader so bytes don't fall off the end of
// the buffer
IOBufferReader *buf_start = server_entry->write_buffer->alloc_reader();
- server_request_hdr_bytes = hdr_length = write_header_into_buffer(&t_state.hdr_info.server_request, server_entry->write_buffer);
+
+ if (server_txn->supports_direct_header_passing()) {
+ _server_request_header_is_ready = true;
+ // Only the write buffer is empty; retry decisions and stats need the real size.
+ server_request_hdr_bytes = t_state.hdr_info.server_request.length_get();
+ hdr_length = 0;
Review Comment:
Not changing this. On the previous HTTP/2 path, do_io_write() reparsed the header synchronously and advanced ndone before any timeout could fire, so a stalled stream was already classified as INACTIVE_TIMEOUT. The direct path gives the same classification. The only difference is a cross-thread deferral window of microseconds, and the effect there is not retrying, which can't duplicate a request.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,68 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) && url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing (RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c) { return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ if (field.name_get().size() + field.value_get().size() > field_max) {
+ return false;
+ }
+ }
Review Comment:
Correction to my earlier reply: parse_req() does reject x(foo. For names that aren't well-known headers, mime_parser_parse() checks every character with is_http_field_name(). The gate now falls back to parse_req for any such name.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +959,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0) pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime, field.value_get());
+ this->_send_header.field_attach(f);
Review Comment:
Correction to my earlier reply: the previous outbound reparse would have rejected x(foo, not forwarded it. The request fails either way (the HTTP/2 origin rejects the invalid name), and the name comes from plugin code, so I'm still leaving the outbound copy as it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce H2 header allocations [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4149703355
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1222,14 +1222,27 @@ url_is_mostly_compliant(const char *start, const char *end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
+
+ switch (strict_uri_parsing) {
+ case 1:
+ return url_is_strictly_compliant(start, end);
+ case 2:
+ return url_is_mostly_compliant(start, end);
+ default:
+ return true;
+ }
+}
Review Comment:
The current code is correct: C++ defines `nullptr + 0` as a null pointer ([expr.add]/4.1), two null pointers compare equal so `i < end` is false, and neither compliance loop dereferences outside its body. We added the `value.empty()` early return anyway, since it makes that explicit.
##########
include/proxy/http/HttpSM.h:
##########
@@ -636,6 +636,12 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
IOBufferReader *_netvc_reader = nullptr;
MIOBuffer *_netvc_read_buffer = nullptr;
+ bool _client_response_header_is_ready = false;
+ bool _server_request_header_is_ready = false;
+
+ // Direct-passed headers bypass the tunnel, so client_response_hdr_bytes stays 0.
+ int _direct_response_hdr_bytes = 0;
+
Review Comment:
Same as the earlier threads on this: every HttpSM header byte counter is an `int`, and so are `TSHttpTxnClientRespHdrBytesGet()` and `HTTPHdr::length_get()`, which `_direct_response_hdr_bytes` is set from. An `int64_t` here couldn't hold a larger value than its `int` source, so we're keeping `int`.
##########
include/proxy/http/HttpSM.h:
##########
@@ -652,6 +658,25 @@ class HttpSM : public Continuation, public PluginUserArgs<TS_USER_ARGS_TXN>
int client_transaction_priority_weight() const;
int client_transaction_priority_dependence() const;
+ HTTPHdr *get_client_response_header();
+ HTTPHdr *get_server_request_header();
+
+ int
+ reported_client_response_hdr_bytes() const
+ {
+ return client_response_hdr_bytes + _direct_response_hdr_bytes;
+ }
Review Comment:
Same as the earlier threads on this: every HttpSM header byte counter is an `int`, and so are `TSHttpTxnClientRespHdrBytesGet()` and `HTTPHdr::length_get()`, which `_direct_response_hdr_bytes` is set from. An `int64_t` here couldn't hold a larger value than its `int` source, so we're keeping `int`.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,53 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped on this path, so re-apply its checks. Call it only after the REQUEST
+ // type check below: the accessors assert it.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const level = config->strict_uri_parsing;
+ auto const line_max = static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max = static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || !std::ranges::all_of(method, &ParseRules::is_token)) {
+ return false;
+ }
+ if (!url_is_uri_compliant(level, _receive_header.path_get()) || !url_is_uri_compliant(level, _receive_header.query_get()) ||
+ !url_is_uri_compliant(level, _receive_header.fragment_get())) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req makes the exact call.
+ if (method.size() + static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops or rejects a name with any non-field-name character, and trims values.
+ if (name.empty() || name.size() + value.size() > field_max || !std::ranges::all_of(name, &ParseRules::is_http_field_name) ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) || ParseRules::is_ws(value.back())))) {
+ return false;
+ }
+ }
+ return validate_hdr_host(_receive_header.m_http) == ParseResult::DONE &&
+ validate_hdr_content_length(_receive_header.m_heap, _receive_header.m_http) == ParseResult::DONE;
+ };
Review Comment:
Done: the checks now live in `HTTPHdr::parse_req_would_accept()` in HTTP.cc, next to the `validate_hdr_host()` and `validate_hdr_content_length()` it calls. `HdrParseReqWouldAccept` in test_Hdrs.cc covers each rejection and the field-size boundary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org