Copilot commented on code in PR #12936:
URL: https://github.com/apache/trafficserver/pull/12936#discussion_r2908305116
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -1100,6 +1127,27 @@ Http2ConnectionState::rcv_continuation_frame(const
Http2Frame &frame)
Http2ErrorCode result =
stream->decode_header_blocks(*this->local_hpack_handle,
this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE));
+ // We just processed the heaer blocks to keep the dynamic table in
+ // sync with peer to avoid future HPACK compression errors
+ if (stream->reset_header_after_decoding) {
+ stream->reset_receive_headers();
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+ this->stream_list.remove(stream);
+ if (stream->free_stream_after_decoding) {
+ stream->initiating_close();
+ }
Review Comment:
Same issue as in `rcv_headers_frame`: `initiating_close()` may send an
RST_STREAM(NO_ERROR) if the stream is in a writeable state, and then the
`HTTP2_ERROR_CLASS_STREAM` return at line 1144 causes `rcv_frame()` to send a
second RST_STREAM(REFUSED_STREAM). This results in duplicate RST_STREAM frames
for the same stream.
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -477,13 +498,19 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame
&frame)
Http2ErrorCode result =
stream->decode_header_blocks(*this->local_hpack_handle,
this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE));
- // If this was an outbound connection and the state was already closed,
just clear the
- // headers after processing. We just processed the heaer blocks to keep
the dynamic table in
+ // We just processed the heaer blocks to keep the dynamic table in
Review Comment:
Typo: "heaer" should be "header" in the comment.
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -477,13 +498,19 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame
&frame)
Http2ErrorCode result =
stream->decode_header_blocks(*this->local_hpack_handle,
this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE));
- // If this was an outbound connection and the state was already closed,
just clear the
- // headers after processing. We just processed the heaer blocks to keep
the dynamic table in
+ // We just processed the heaer blocks to keep the dynamic table in
// sync with peer to avoid future HPACK compression errors
if (reset_header_after_decoding) {
stream->reset_receive_headers();
+ SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
+ this->stream_list.remove(stream);
if (free_stream_after_decoding) {
- THREAD_FREE(stream, http2StreamAllocator, this_ethread());
+ stream->initiating_close();
+ }
Review Comment:
When `initiating_close()` is called on the temporary stream here, the
stream's state has already transitioned from IDLE to OPEN (via `change_state`
at line 481), making `is_state_writeable()` return true. This causes
`initiating_close()` to send an RST_STREAM with `HTTP2_ERROR_NO_ERROR`. Then,
when this function returns the `HTTP2_ERROR_CLASS_STREAM` error at line 512,
`rcv_frame()` sends a second RST_STREAM with `HTTP2_ERROR_REFUSED_STREAM` (line
1556 of `rcv_frame`). This results in duplicate RST_STREAM frames for the same
stream. Consider either not calling `initiating_close()` and instead using
`THREAD_FREE` directly (as the old code did for the outbound case), or setting
the stream state to CLOSED before calling `initiating_close()` to prevent the
extra RST_STREAM.
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -1100,6 +1127,27 @@ Http2ConnectionState::rcv_continuation_frame(const
Http2Frame &frame)
Http2ErrorCode result =
stream->decode_header_blocks(*this->local_hpack_handle,
this->acknowledged_local_settings.get(HTTP2_SETTINGS_HEADER_TABLE_SIZE));
+ // We just processed the heaer blocks to keep the dynamic table in
Review Comment:
Typo: "heaer" should be "header" in the comment.
##########
src/proxy/http2/Http2CommonSession.cc:
##########
@@ -362,25 +362,30 @@ Http2CommonSession::do_process_frame_read(int /* event
ATS_UNUSED */, VIO *vio,
while (this->_read_buffer_reader->read_avail() >=
static_cast<int64_t>(HTTP2_FRAME_HEADER_LEN)) {
// Cancel reading if there was an error or connection is closed
- if (connection_state.tx_error_code.code !=
static_cast<uint32_t>(Http2ErrorCode::HTTP2_ERROR_NO_ERROR) ||
- connection_state.is_state_closed()) {
+ const auto has_fatal_error_code =
+ (connection_state.tx_error_code.code !=
static_cast<uint32_t>(Http2ErrorCode::HTTP2_ERROR_NO_ERROR) &&
+ connection_state.tx_error_code.code !=
static_cast<uint32_t>(Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM));
+ if (has_fatal_error_code || connection_state.is_state_closed()) {
Http2SsnDebug("reading a frame has been canceled (%u)",
connection_state.tx_error_code.code);
- break;
+ return 0;
}
- Http2ErrorCode err = Http2ErrorCode::HTTP2_ERROR_NO_ERROR;
- if (this->connection_state.get_stream_error_rate() > std::min(1.0,
Http2::stream_error_rate_threshold * 2.0)) {
+ if (this->connection_state.get_stream_error_rate() > std::min(1.0,
Http2::stream_error_rate_threshold * 2.0) &&
+ !this->connection_state.get_goaway_sent()) {
ip_port_text_buffer ipb;
const char *peer_ip =
ats_ip_ntop(this->get_proxy_session()->get_remote_addr(), ipb, sizeof(ipb));
SiteThrottledWarning("HTTP/2 session error peer_ip=%s session_id=%"
PRId64
" closing a connection, because its stream error
rate (%f) exceeded the threshold (%f)",
peer_ip, this->get_connection_id(),
this->connection_state.get_stream_error_rate(),
Http2::stream_error_rate_threshold);
- err = Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM;
+
this->connection_state.send_goaway_frame(this->connection_state.get_latest_stream_id_in(),
+
Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM);
+ this->set_half_close_local_flag(true);
Review Comment:
This PR fixes a significant behavioral issue (408 timeouts caused by
ignoring DATA frames after GOAWAY), but doesn't include any test coverage for
the new behavior. Given the existing test infrastructure in
`tests/gold_tests/h2/` and the complexity of this fix (continuing to process
frames after GOAWAY, minimal HPACK processing for HEADERS/CONTINUATION on
refused streams, flow-control counting for DATA), it would be valuable to add
an integration test that validates the core scenario: sending multiple POST
requests over a single HTTP/2 connection where the stream error rate threshold
is exceeded, and verifying that in-flight requests with pending DATA frames
complete successfully rather than timing out.
--
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]
