Fix stream ID sent in GOAWAY frame.
The stream ID sent in a HTTP/3 GOAWAY frame is the lowest that can be retried by
the client. (This is unlike HTTP/2 where the stream ID is the highest that
cannot be retried.) Fix QuicSpdySession::SendHttp3GoAway() to send the next
stream ID after the largest received one.
Also do not send GOAWAY frame if it could not convey new information to the
client with respect to the previously sent GOAWAY frame.
Protected by FLAGS_quic_reloadable_flag_quic_fix_http3_goaway_stream_id.
PiperOrigin-RevId: 330840957
Change-Id: I3f2a1a18e5d0fb294d39f21c315b8af7ec1a4811
diff --git a/quic/core/http/quic_send_control_stream.cc b/quic/core/http/quic_send_control_stream.cc
index a7a5349..c7f1b38 100644
--- a/quic/core/http/quic_send_control_stream.cc
+++ b/quic/core/http/quic_send_control_stream.cc
@@ -128,7 +128,8 @@
GoAwayFrame frame;
// If the peer has not created any stream yet, use stream ID 0 to indicate no
// request is accepted.
- if (id == QuicUtils::GetInvalidStreamId(session()->transport_version())) {
+ if (!GetQuicReloadableFlag(quic_fix_http3_goaway_stream_id) &&
+ id == QuicUtils::GetInvalidStreamId(session()->transport_version())) {
id = 0;
}
frame.id = id;
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc
index 753bd18..82bfab6 100644
--- a/quic/core/http/quic_spdy_session.cc
+++ b/quic/core/http/quic_spdy_session.cc
@@ -730,8 +730,29 @@
void QuicSpdySession::SendHttp3GoAway() {
DCHECK_EQ(perspective(), Perspective::IS_SERVER);
DCHECK(VersionUsesHttp3(transport_version()));
- const QuicStreamId stream_id =
+
+ QuicStreamId stream_id =
GetLargestPeerCreatedStreamId(/*unidirectional = */ false);
+
+ if (GetQuicReloadableFlag(quic_fix_http3_goaway_stream_id)) {
+ if (stream_id == QuicUtils::GetInvalidStreamId(transport_version())) {
+ // No client-initiated bidirectional streams received yet.
+ // Send 0 to let client know that all requests can be retried.
+ stream_id = 0;
+ } else {
+ // Tell client that streams starting with the next after the largest
+ // received one can be retried.
+ stream_id += QuicUtils::StreamIdDelta(transport_version());
+ }
+ if (last_sent_http3_goaway_id_.has_value() &&
+ last_sent_http3_goaway_id_.value() <= stream_id) {
+ // MUST not send GOAWAY with identifier larger than previously sent.
+ // Do not bother sending one with same identifier as before, since GOAWAY
+ // frames on the control stream are guaranteed to be processed in order.
+ return;
+ }
+ }
+
send_control_stream_->SendGoAway(stream_id);
last_sent_http3_goaway_id_ = stream_id;
}
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h
index e9143c8..0a71e48 100644
--- a/quic/core/http/quic_spdy_session.h
+++ b/quic/core/http/quic_spdy_session.h
@@ -225,7 +225,10 @@
// Send GOAWAY if the peer is blocked on the implementation max.
bool OnStreamsBlockedFrame(const QuicStreamsBlockedFrame& frame) override;
- // Write GOAWAY frame to the control stream with the last seen stream ID.
+ // Write GOAWAY frame on the control stream to notify the client that every
+ // stream that has not reached the server yet can be retried. Do not send a
+ // GOAWAY frame if it could not convey new information to the client with
+ // respect to the previous GOAWAY frame.
void SendHttp3GoAway();
// Write advisory GOAWAY frame on the control stream with the max stream ID
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index 740865d..6012638 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -1080,7 +1080,10 @@
EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _))
.WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0)));
- EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(_));
+ // No client-initiated stream has been received, therefore a GOAWAY frame with
+ // stream ID = 0 is sent to notify the client that all requests can be retried
+ // on a different connection.
+ EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(/* stream_id = */ 0));
session_.SendHttp3GoAway();
EXPECT_TRUE(session_.goaway_sent());
@@ -1090,6 +1093,37 @@
EXPECT_TRUE(session_.GetOrCreateStream(kTestStreamId));
}
+TEST_P(QuicSpdySessionTestServer, SendHttp3GoAwayAfterStreamIsCreated) {
+ if (!VersionUsesHttp3(transport_version())) {
+ return;
+ }
+
+ if (!GetQuicReloadableFlag(quic_fix_http3_goaway_stream_id)) {
+ return;
+ }
+
+ CompleteHandshake();
+ StrictMock<MockHttp3DebugVisitor> debug_visitor;
+ session_.set_debug_visitor(&debug_visitor);
+
+ const QuicStreamId kTestStreamId =
+ GetNthClientInitiatedBidirectionalStreamId(transport_version(), 0);
+ EXPECT_TRUE(session_.GetOrCreateStream(kTestStreamId));
+
+ EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _))
+ .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0)));
+ // The first stream, of kTestStreamId = 0, could already have been processed.
+ // A GOAWAY frame is sent to notify the client that requests starting with
+ // stream ID = 4 can be retried on a different connection.
+ EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(/* stream_id = */ 4));
+ session_.SendHttp3GoAway();
+ EXPECT_TRUE(session_.goaway_sent());
+
+ // No more GOAWAY frames are sent because they could not convey new
+ // information to the client.
+ session_.SendHttp3GoAway();
+}
+
TEST_P(QuicSpdySessionTestServer, SendHttp3Shutdown) {
if (!VersionUsesHttp3(transport_version())) {
return;
