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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
