Fix 408 timeout caused by ignoring DATA frames after GOAWAY #12936
+121
−26
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,6 +118,14 @@ Http2ConnectionState::rcv_data_frame(const Http2Frame &frame) | |
| "recv data bad frame client id"); | ||
| } | ||
|
|
||
| // After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with | ||
| // identifiers higher than the identified last stream. However, DATA frames MUST be counted toward | ||
| // the connection flow-control window. (Details in [RFC 9113] 6.8.) | ||
| if (this->goaway_sent && id > this->last_stream_id_tx) { | ||
| return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM, | ||
| "recv data with id higher than last stream id"); | ||
| } | ||
|
|
||
| Http2Stream *stream = this->find_stream(id); | ||
| if (stream == nullptr) { | ||
| if (this->is_valid_streamid(id)) { | ||
|
|
@@ -330,15 +338,25 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame) | |
| "recv headers bad client id"); | ||
| } | ||
|
|
||
| // After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with | ||
| // identifiers higher than the identified last stream. However, HEADERS, PUSH_PROMISE, and CONTINUATION | ||
| // frames MUST be minimally processed to ensure that the state maintained for field section compression is | ||
| // consistent (Details in [RFC 9113] 6.8.) | ||
| if (this->goaway_sent && stream_id > this->last_stream_id_tx) { | ||
| reset_header_after_decoding = true; | ||
| } | ||
|
|
||
| if (!stream) { | ||
| if (reset_header_after_decoding) { | ||
| free_stream_after_decoding = true; | ||
| uint32_t const initial_local_stream_window = this->acknowledged_local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE); | ||
| ink_assert(dynamic_cast<Http2CommonSession *>(this->session->get_proxy_session())); | ||
| stream = THREAD_ALLOC_INIT(http2StreamAllocator, this_ethread(), this->session->get_proxy_session(), stream_id, | ||
| this->peer_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE), initial_local_stream_window, | ||
| !STREAM_IS_REGISTERED); | ||
| stream->mutex = new_ProxyMutex(); | ||
| SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread()); | ||
| this->stream_list.enqueue(stream); | ||
| } else { | ||
| // Create new stream | ||
| Http2Error error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE); | ||
|
|
@@ -370,6 +388,9 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame &frame) | |
| } | ||
| } | ||
|
|
||
| stream->reset_header_after_decoding = reset_header_after_decoding; | ||
| stream->free_stream_after_decoding = free_stream_after_decoding; | ||
|
|
||
| Http2HeadersParameter params; | ||
| uint32_t header_block_fragment_offset = 0; | ||
| uint32_t header_block_fragment_length = payload_length; | ||
|
|
@@ -477,13 +498,21 @@ 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)); | ||
|
|
||
| // We just processed the header 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) { | ||
| // Suppress RST_STREAM(NO_ERROR): rcv_frame() will send RST_STREAM(REFUSED_STREAM) | ||
| // for this stream, so sending NO_ERROR here would produce duplicate frames. | ||
| stream->initiating_close(/* suppress_rst= */ true); | ||
| } | ||
|
Comment on lines
507
to
+511
There was a problem hiding this comment. When
Contributor
Author
There was a problem hiding this comment. As a conclusion, I added a Calling trafficserver/src/proxy/http2/Http2Stream.cc Line 101 in 8d5a71f Regarding the alternative approach of modifying the stream state before calling |
||
|
|
||
| if (this->goaway_sent && stream_id > this->last_stream_id_tx) { | ||
| return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM, | ||
| "recv headers with id higher than last stream id"); | ||
| } | ||
| return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE); | ||
| } | ||
|
|
@@ -1100,6 +1129,29 @@ 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 header 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) { | ||
| // Suppress RST_STREAM(NO_ERROR): rcv_frame() will send RST_STREAM(REFUSED_STREAM) | ||
| // for this stream, so sending NO_ERROR here would produce duplicate frames. | ||
| stream->initiating_close(/* suppress_rst= */ true); | ||
| } | ||
|
Comment on lines
+1138
to
+1142
There was a problem hiding this comment. Same issue as in |
||
|
|
||
| // After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with | ||
| // identifiers higher than the identified last stream. However, HEADERS, PUSH_PROMISE, and CONTINUATION | ||
| // frames MUST be minimally processed to ensure that the state maintained for field section compression is | ||
| // consistent (Details in [RFC 9113] 6.8.) | ||
| if (this->goaway_sent && stream_id > this->last_stream_id_tx) { | ||
| return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_STREAM, Http2ErrorCode::HTTP2_ERROR_REFUSED_STREAM, | ||
| "recv continuation with id higher than last stream id"); | ||
| } | ||
| return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_NONE); | ||
| } | ||
|
|
||
| if (result != Http2ErrorCode::HTTP2_ERROR_NO_ERROR) { | ||
| if (result == Http2ErrorCode::HTTP2_ERROR_COMPRESSION_ERROR) { | ||
| return Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_COMPRESSION_ERROR, | ||
|
|
@@ -1437,12 +1489,13 @@ Http2ConnectionState::rcv_frame(const Http2Frame *frame) | |
| { | ||
| REMEMBER(NO_EVENT, this->recursion); | ||
| const Http2StreamId stream_id = frame->header().streamid; | ||
| const auto type = frame->header().type; | ||
| Http2Error error; | ||
|
|
||
| // [RFC 7540] 5.5. Extending HTTP/2 | ||
| // Implementations MUST discard frames that have unknown or unsupported types. | ||
| if (type >= HTTP2_FRAME_TYPE_MAX) { | ||
| Http2StreamDebug(session, stream_id, "Discard a frame which has unknown type, type=%x", type); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -1457,15 +1510,28 @@ Http2ConnectionState::rcv_frame(const Http2Frame *frame) | |
| // GOAWAY: NO | ||
| // WINDOW_UPDATE: YES | ||
| // CONTINUATION: YES (safe http methods only, same as HEADERS frame). | ||
| if (frame->is_from_early_data() && (type == HTTP2_FRAME_TYPE_DATA || type == HTTP2_FRAME_TYPE_RST_STREAM || | ||
| type == HTTP2_FRAME_TYPE_PUSH_PROMISE || type == HTTP2_FRAME_TYPE_GOAWAY)) { | ||
| Http2StreamDebug(session, stream_id, "Discard a frame which is received from early data and has type=%x", type); | ||
| return; | ||
| } | ||
|
|
||
| // After sending a GOAWAY frame, the sender can discard frames for streams initiated by the receiver with | ||
| // identifiers higher than the identified last stream. However, HEADERS, PUSH_PROMISE, and CONTINUATION | ||
| // frames MUST be minimally processed to ensure that the state maintained for field section compression is | ||
| // consistent; similarly, DATA frames MUST be counted toward the connection flow-control window. | ||
| // (Details in [RFC 9113] 6.8.) | ||
| if (this->goaway_sent && stream_id > this->last_stream_id_tx) { | ||
| const auto is_discardable = (type != HTTP2_FRAME_TYPE_HEADERS && type != HTTP2_FRAME_TYPE_PUSH_PROMISE && | ||
| type != HTTP2_FRAME_TYPE_CONTINUATION && type != HTTP2_FRAME_TYPE_DATA); | ||
| if (is_discardable) { | ||
| Http2StreamDebug(session, stream_id, "Discard a frame which is received after sending a GOAWAY and has type=%x", type); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (this->_frame_handlers[type]) { | ||
| error = (this->*_frame_handlers[type])(*frame); | ||
| } else { | ||
| error = Http2Error(Http2ErrorClass::HTTP2_ERROR_CLASS_CONNECTION, Http2ErrorCode::HTTP2_ERROR_INTERNAL_ERROR, "no handler"); | ||
| } | ||
|
|
@@ -2744,7 +2810,9 @@ Http2ConnectionState::send_goaway_frame(Http2StreamId id, Http2ErrorCode ec) | |
| Metrics::Counter::increment(http2_rsb.connection_errors_count); | ||
| } | ||
|
|
||
| this->tx_error_code = {ProxyErrorClass::SSN, static_cast<uint32_t>(ec)}; | ||
| this->last_stream_id_tx = id; | ||
| this->goaway_sent = true; | ||
|
|
||
| Http2Goaway goaway; | ||
| goaway.last_streamid = id; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.