Deprecate --gfe2_reloadable_flag_quic_goaway_with_max_stream_id. Also remove QuicSpdySession::SendHttp3Shutdown(). Callers should use QuicSpdySession::SendHttp3GoAway() instead, which behaves identically. PiperOrigin-RevId: 359798040 Change-Id: I53495a19141ec02438aad92a93b2c8c205ceaf53
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index 8418bb2..81c60b2 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -508,14 +508,9 @@ ietf_server_push_enabled_( GetQuicFlag(FLAGS_quic_enable_http3_server_push)), http3_max_push_id_sent_(false), - goaway_with_max_stream_id_( - GetQuicReloadableFlag(quic_goaway_with_max_stream_id)), next_available_datagram_flow_id_(perspective() == Perspective::IS_SERVER ? kFirstDatagramFlowIdServer : kFirstDatagramFlowIdClient) { - if (goaway_with_max_stream_id_) { - QUIC_RELOADABLE_FLAG_COUNT_N(quic_goaway_with_max_stream_id, 1, 2); - } h2_deframer_.set_visitor(spdy_framer_visitor_.get()); h2_deframer_.set_debug_visitor(spdy_framer_visitor_.get()); spdy_framer_.set_debug_visitor(spdy_framer_visitor_.get()); @@ -822,40 +817,18 @@ } QuicStreamId stream_id; - if (goaway_with_max_stream_id_) { - stream_id = QuicUtils::GetMaxClientInitiatedBidirectionalStreamId( - transport_version()); - if (last_sent_http3_goaway_id_.has_value()) { - if (last_sent_http3_goaway_id_.value() == stream_id) { - // Do not send GOAWAY twice. - return; - } - if (last_sent_http3_goaway_id_.value() < stream_id) { - // A previous GOAWAY frame was sent with smaller stream ID. This is not - // possible, because the only time a GOAWAY frame with non-maximal - // stream ID is sent is right before closing connection. - QUIC_BUG << "GOAWAY frame with smaller ID already sent."; - return; - } + stream_id = QuicUtils::GetMaxClientInitiatedBidirectionalStreamId( + transport_version()); + if (last_sent_http3_goaway_id_.has_value()) { + if (last_sent_http3_goaway_id_.value() == stream_id) { + // Do not send GOAWAY twice. + return; } - } else { - stream_id = GetLargestPeerCreatedStreamId(/*unidirectional = */ false); - - 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. + if (last_sent_http3_goaway_id_.value() < stream_id) { + // A previous GOAWAY frame was sent with smaller stream ID. This is not + // possible, because the only time a GOAWAY frame with non-maximal + // stream ID is sent is right before closing connection. + QUIC_BUG << "GOAWAY frame with smaller ID already sent."; return; } } @@ -864,30 +837,6 @@ last_sent_http3_goaway_id_ = stream_id; } -void QuicSpdySession::SendHttp3Shutdown() { - if (goaway_with_max_stream_id_) { - SendHttp3GoAway(QUIC_PEER_GOING_AWAY, "Server shutdown"); - return; - } - - QUICHE_DCHECK_EQ(perspective(), Perspective::IS_SERVER); - QUICHE_DCHECK(VersionUsesHttp3(transport_version())); - QuicStreamCount advertised_max_incoming_bidirectional_streams = - GetAdvertisedMaxIncomingBidirectionalStreams(); - const QuicStreamId stream_id = - QuicUtils::GetFirstBidirectionalStreamId(transport_version(), - Perspective::IS_CLIENT) + - QuicUtils::StreamIdDelta(transport_version()) * - advertised_max_incoming_bidirectional_streams; - if (last_sent_http3_goaway_id_.has_value() && - last_sent_http3_goaway_id_.value() < stream_id) { - send_control_stream_->SendGoAway(last_sent_http3_goaway_id_.value()); - return; - } - send_control_stream_->SendGoAway(stream_id); - last_sent_http3_goaway_id_ = stream_id; -} - void QuicSpdySession::WritePushPromise(QuicStreamId original_stream_id, QuicStreamId promised_stream_id, SpdyHeaderBlock headers) { @@ -1555,13 +1504,12 @@ } if (last_sent_http3_goaway_id_.has_value() && last_sent_http3_goaway_id_.value() <= stream_id) { - if (goaway_with_max_stream_id_) { - // A previous GOAWAY frame was sent with smaller stream ID. This is not - // possible, because this is the only method sending a GOAWAY frame with - // non-maximal stream ID, and this must only be called once, right - // before closing connection. - QUIC_BUG << "GOAWAY frame with smaller ID already sent."; - } + // A previous GOAWAY frame was sent with smaller stream ID. This is not + // possible, because this is the only method sending a GOAWAY frame with + // non-maximal stream ID, and this must only be called once, right + // before closing connection. + QUIC_BUG << "GOAWAY frame with smaller ID already sent."; + // 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.
diff --git a/quic/core/http/quic_spdy_session.h b/quic/core/http/quic_spdy_session.h index 7e13afa..cff9e63 100644 --- a/quic/core/http/quic_spdy_session.h +++ b/quic/core/http/quic_spdy_session.h
@@ -243,10 +243,6 @@ // before encryption gets established. void SendHttp3GoAway(QuicErrorCode error_code, const std::string& reason); - // Same as SendHttp3GoAway(). TODO(bnc): remove when - // gfe2_reloadable_flag_quic_goaway_with_max_stream_id flag is deprecated. - void SendHttp3Shutdown(); - // Write |headers| for |promised_stream_id| on |original_stream_id| in a // PUSH_PROMISE frame to peer. virtual void WritePushPromise(QuicStreamId original_stream_id, @@ -627,9 +623,6 @@ // recent MAX_PUSH_ID frame. Once true, never goes back to false. bool http3_max_push_id_sent_; - // Latched value of reloadable flag quic_goaway_with_max_stream_id. - const bool goaway_with_max_stream_id_; - // Value of the smallest unused HTTP/3 datagram flow ID that this endpoint's // datagram flow ID allocation service will use next. QuicDatagramFlowId next_available_datagram_flow_id_;
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index 848f215..b266633 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -1132,15 +1132,8 @@ EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _)) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0))); - if (GetQuicReloadableFlag(quic_goaway_with_max_stream_id)) { - // Send max stream id (currently 32 bits). - EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(/* stream_id = */ 0xfffffffc)); - } else { - // 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)); - } + // Send max stream id (currently 32 bits). + EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(/* stream_id = */ 0xfffffffc)); session_.SendHttp3GoAway(QUIC_PEER_GOING_AWAY, "Goaway"); EXPECT_TRUE(session_.goaway_sent()); @@ -1183,15 +1176,8 @@ EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _)) .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0))); - if (GetQuicReloadableFlag(quic_goaway_with_max_stream_id)) { - // Send max stream id (currently 32 bits). - EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(/* stream_id = */ 0xfffffffc)); - } else { - // 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)); - } + // Send max stream id (currently 32 bits). + EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(/* stream_id = */ 0xfffffffc)); session_.SendHttp3GoAway(QUIC_PEER_GOING_AWAY, "Goaway"); EXPECT_TRUE(session_.goaway_sent()); @@ -1200,59 +1186,6 @@ session_.SendHttp3GoAway(QUIC_PEER_GOING_AWAY, "Goaway"); } -TEST_P(QuicSpdySessionTestServer, SendHttp3Shutdown) { - if (GetQuicReloadableFlag(quic_goaway_with_max_stream_id)) { - return; - } - - if (!VersionUsesHttp3(transport_version())) { - return; - } - - CompleteHandshake(); - StrictMock<MockHttp3DebugVisitor> debug_visitor; - session_.set_debug_visitor(&debug_visitor); - - EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _)) - .WillOnce(Return(WriteResult(WRITE_STATUS_OK, 0))); - EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(_)); - session_.SendHttp3Shutdown(); - EXPECT_TRUE(session_.goaway_sent()); - - const QuicStreamId kTestStreamId = - GetNthClientInitiatedBidirectionalStreamId(transport_version(), 0); - EXPECT_CALL(*connection_, OnStreamReset(kTestStreamId, _)).Times(0); - EXPECT_TRUE(session_.GetOrCreateStream(kTestStreamId)); -} - -TEST_P(QuicSpdySessionTestServer, SendHttp3GoAwayAfterShutdownNotice) { - if (GetQuicReloadableFlag(quic_goaway_with_max_stream_id)) { - return; - } - - if (!VersionUsesHttp3(transport_version())) { - return; - } - - CompleteHandshake(); - StrictMock<MockHttp3DebugVisitor> debug_visitor; - session_.set_debug_visitor(&debug_visitor); - - EXPECT_CALL(*writer_, WritePacket(_, _, _, _, _)) - .Times(2) - .WillRepeatedly(Return(WriteResult(WRITE_STATUS_OK, 0))); - EXPECT_CALL(debug_visitor, OnGoAwayFrameSent(_)).Times(2); - - session_.SendHttp3Shutdown(); - EXPECT_TRUE(session_.goaway_sent()); - session_.SendHttp3GoAway(QUIC_PEER_GOING_AWAY, "Goaway"); - - const QuicStreamId kTestStreamId = - GetNthClientInitiatedBidirectionalStreamId(transport_version(), 0); - EXPECT_CALL(*connection_, OnStreamReset(kTestStreamId, _)).Times(0); - EXPECT_TRUE(session_.GetOrCreateStream(kTestStreamId)); -} - TEST_P(QuicSpdySessionTestServer, DoNotSendGoAwayTwice) { CompleteHandshake(); if (VersionHasIetfQuicFrames(transport_version())) {
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 82fe74b..40f8502 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -45,7 +45,6 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_encrypted_goaway, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_on_stream_reset, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_willing_and_able_to_write2, true) -QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_goaway_with_max_stream_id, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_h3_datagram, false) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_parse_accept_ch_frame, true) QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_pass_path_response_to_validator, false)
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc index 8cd0a57..e3b0408 100644 --- a/quic/core/quic_session.cc +++ b/quic/core/quic_session.cc
@@ -911,19 +911,13 @@ return; } transport_goaway_sent_ = true; - if (GetQuicReloadableFlag(quic_goaway_with_max_stream_id)) { - QUICHE_DCHECK_EQ(perspective(), Perspective::IS_SERVER); - QUIC_RELOADABLE_FLAG_COUNT_N(quic_goaway_with_max_stream_id, 2, 2); - control_frame_manager_.WriteOrBufferGoAway( - error_code, - QuicUtils::GetMaxClientInitiatedBidirectionalStreamId( - transport_version()), - reason); - } else { - control_frame_manager_.WriteOrBufferGoAway( - error_code, stream_id_manager_.largest_peer_created_stream_id(), - reason); - } + + QUICHE_DCHECK_EQ(perspective(), Perspective::IS_SERVER); + control_frame_manager_.WriteOrBufferGoAway( + error_code, + QuicUtils::GetMaxClientInitiatedBidirectionalStreamId( + transport_version()), + reason); } void QuicSession::SendBlocked(QuicStreamId id) {