bneradt commented on code in PR #13783:
URL: https://github.com/apache/trafficserver/pull/13783#discussion_r4161403461
##########
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:
Added in the force-pushed commit. `goaway-split` holds three POSTs that ATS
multiplexes onto one origin connection, then sends
`GOAWAY(last_stream_id=<lowest>, NO_ERROR)` and answers only the lowest stream.
The test asserts that the lowest stream gets the drained response with no
replay, that both higher streams are retried on a new connection, that
`retry_admitted` is 2, and that ATS closes the drained connection itself.
Without the drain, the covered stream comes back as a 502. If the
`initiating_close()` of the above-boundary streams is removed, the scenario
hangs and fails.
--
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]
