Copilot commented on code in PR #13783:
URL: https://github.com/apache/trafficserver/pull/13783#discussion_r4160966194
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -971,6 +987,45 @@ Http2ConnectionState::rcv_goaway_frame(const Http2Frame
&frame)
static_cast<int>(goaway.error_code));
this->rx_error_code = {ProxyErrorClass::SSN,
static_cast<uint32_t>(goaway.error_code)};
+
+ // Per RFC 9113 6.8: streams whose id is greater than `last_streamid` were
+ // not (and will not be) processed by the peer, and the requests they carry
+ // may be safely retried on a fresh connection. On an outbound H/2 session
+ // the streams in question are the ones we initiated toward the origin
+ // using odd-numbered client stream IDs. Tagging them before closing them
+ // lets HttpSM decide to retry non-idempotent requests (e.g. POST) that
+ // would otherwise surface to the client as ERR_CLIENT_ABORT. This is
+ // especially important for AWS-style origin load balancers that
+ // aggressively send GOAWAY(last_stream_id=0, NO_ERROR) when draining a
+ // connection.
+ //
+ // The origin may still complete the streams at or below `last_streamid`,
+ // so those are left to finish. Half-closing the session takes it out of
+ // the pool and stops new streams; release_stream() closes it once the
+ // remaining streams are gone.
+ if (this->session->is_outbound()) {
+ this->_peer_goaway_drain = true;
+ this->session->set_half_close_local_flag(true);
+
+ Http2Stream *s = stream_list.head;
+ while (s != nullptr) {
+ Http2Stream *next = static_cast<Http2Stream *>(s->link.next);
+ if (http2_is_client_streamid(s->get_id()) && s->get_id() >
goaway.last_streamid) {
+ Http2StreamDebug(this->session, s->get_id(), "Origin not-processed
assertion: GOAWAY last_stream_id=%u",
+ goaway.last_streamid);
+ s->set_safe_to_retry();
+ s->set_rx_error_code(this->rx_error_code);
+ s->initiating_close();
Review Comment:
The new drain tests exercise only one origin stream per session, so they do
not cover the selective behavior in this loop. Testing GOAWAY(0) and a covered
stream in separate scenarios would still pass if a regression closed every
stream or failed to close streams above the boundary. Add a multiplexed case
with concurrent streams on both sides of `last_stream_id`, asserting that the
lower stream completes without replay while the higher stream retries on a
fresh connection.
--
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]
