-
Notifications
You must be signed in to change notification settings - Fork 852
Fix 408 timeout caused by ignoring DATA frames after GOAWAY #12936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
yknoya
wants to merge
1
commit into
apache:master
Choose a base branch
from
yknoya:fix/h2-post-timeout/read-frame-after-goaway
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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())); | ||
| ink_assert(this->session->is_outbound() == true); | ||
| 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 = 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,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(); | ||
| } | ||
|
Comment on lines
507
to
+509
|
||
|
|
||
| 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 +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(); | ||
| } | ||
|
Comment on lines
+1136
to
+1138
|
||
|
|
||
| // 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 +1485,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 (frame->header().type >= HTTP2_FRAME_TYPE_MAX) { | ||
| Http2StreamDebug(session, stream_id, "Discard a frame which has unknown type, type=%x", frame->header().type); | ||
| if (type >= HTTP2_FRAME_TYPE_MAX) { | ||
| Http2StreamDebug(session, stream_id, "Discard a frame which has unknown type, type=%x", type); | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -1457,15 +1506,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() && | ||
| (frame->header().type == HTTP2_FRAME_TYPE_DATA || frame->header().type == HTTP2_FRAME_TYPE_RST_STREAM || | ||
| frame->header().type == HTTP2_FRAME_TYPE_PUSH_PROMISE || frame->header().type == HTTP2_FRAME_TYPE_GOAWAY)) { | ||
| Http2StreamDebug(session, stream_id, "Discard a frame which is received from early data and has type=%x", frame->header().type); | ||
| 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; | ||
| } | ||
|
|
||
| if (this->_frame_handlers[frame->header().type]) { | ||
| error = (this->*_frame_handlers[frame->header().type])(*frame); | ||
| // 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 +2806,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->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; | ||
|
|
||
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.