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