gfe-relnote: No longer send a RESET_STREAM in response to a STOP_SENDING if the stream is write closed. Protected by disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 293529515
Change-Id: I0a837b79a776233d7a21fb8f87dbb7e173241a26
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc
index 7621288..97623a7 100644
--- a/quic/core/http/end_to_end_test.cc
+++ b/quic/core/http/end_to_end_test.cc
@@ -4013,10 +4013,8 @@
}
}
-// Test that STOP_SENDING makes it to the other side. Set up a client & server,
-// create a stream (do not close it), and then send a STOP_SENDING from one
-// side. The other side should get a call to QuicStream::OnStopSending.
-// (aside, test cribbed from RequestAndStreamRstInOnePacket)
+// Test that STOP_SENDING makes it to the peer. Create a stream and send a
+// STOP_SENDING. The receiver should get a call to QuicStream::OnStopSending.
TEST_P(EndToEndTest, SimpleStopSendingTest) {
const uint16_t kStopSendingTestCode = 123;
ASSERT_TRUE(Initialize());
@@ -4028,9 +4026,6 @@
QuicConnection* client_connection = client_session->connection();
ASSERT_NE(nullptr, client_connection);
- // STOP_SENDING will cause the server to not to send the trailer
- // (and the FIN) after the response body. Instead, it sends a STOP_SENDING
- // frame for the stream.
std::string response_body(1305, 'a');
SpdyHeaderBlock response_headers;
response_headers[":status"] = quiche::QuicheTextUtils::Uint64ToString(200);
@@ -4057,14 +4052,13 @@
// Wait for the connection to become idle.
client_->WaitForDelayedAcks();
- // The real expectation is the test does not crash or timeout.
EXPECT_THAT(client_->connection_error(), IsQuicNoError());
- // And that the stop-sending code is received.
QuicSimpleClientStream* client_stream =
static_cast<QuicSimpleClientStream*>(client_->latest_created_stream());
ASSERT_NE(nullptr, client_stream);
- // Make sure we have the correct stream
+ // Ensure the stream has been write closed upon receiving STOP_SENDING.
EXPECT_EQ(stream_id, client_stream->id());
+ EXPECT_TRUE(client_stream->write_side_closed());
EXPECT_EQ(kStopSendingTestCode,
static_cast<uint16_t>(client_stream->stream_error()));
}
diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc
index b8de367..b63f729 100644
--- a/quic/core/quic_session.cc
+++ b/quic/core/quic_session.cc
@@ -221,9 +221,13 @@
void QuicSession::OnStopSendingFrame(const QuicStopSendingFrame& frame) {
// STOP_SENDING is in IETF QUIC only.
DCHECK(VersionHasIetfQuicFrames(transport_version()));
+ DCHECK(QuicVersionUsesCryptoFrames(transport_version()));
QuicStreamId stream_id = frame.stream_id;
// If Stream ID is invalid then close the connection.
+ // TODO(ianswett): This check is redundant to checks for IsClosedStream,
+ // but removing it requires removing multiple DCHECKs.
+ // TODO(ianswett): Multiple QUIC_DVLOGs could be QUIC_PEER_BUGs.
if (stream_id == QuicUtils::GetInvalidStreamId(transport_version())) {
QUIC_DVLOG(1) << ENDPOINT
<< "Received STOP_SENDING with invalid stream_id: "
@@ -234,6 +238,19 @@
return;
}
+ // If stream_id is READ_UNIDIRECTIONAL, close the connection.
+ if (QuicUtils::GetStreamType(stream_id, perspective(),
+ IsIncomingStream(stream_id)) ==
+ READ_UNIDIRECTIONAL) {
+ QUIC_DVLOG(1) << ENDPOINT
+ << "Received STOP_SENDING for a read-only stream_id: "
+ << stream_id << ".";
+ connection()->CloseConnection(
+ QUIC_INVALID_STREAM_ID, "Received STOP_SENDING for a read-only stream",
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+ return;
+ }
+
if (visitor_) {
visitor_->OnStopSendingReceived(frame);
}
@@ -246,7 +263,9 @@
<< stream_id << " Ignoring.";
return;
}
- // If stream is non-existent, close the connection
+ // If stream is non-existent, close the connection.
+ // TODO(b/148842616): IETF QUIC allows a STOP_SENDING to arrive before a
+ // STREAM frame for peer-intiated bidirectional steams
StreamMap::iterator it = stream_map_.find(stream_id);
if (it == stream_map_.end()) {
QUIC_DVLOG(1) << ENDPOINT
@@ -260,10 +279,22 @@
}
QuicStream* stream = it->second.get();
+ // If the stream is null, it's an implementation error.
if (stream == nullptr) {
QUIC_BUG << ENDPOINT
<< "Received STOP_SENDING for NULL QuicStream, stream_id: "
<< stream_id << ". Ignoring.";
+ connection()->CloseConnection(
+ QUIC_INTERNAL_ERROR, "Received STOP_SENDING for a null stream",
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+ return;
+ }
+
+ // Do not reset the sream if all data has been sent and acknowledged.
+ if (stream->write_side_closed() && !stream->IsWaitingForAcks()) {
+ QUIC_DVLOG(1) << ENDPOINT
+ << "Ignoring STOP_SENDING for a write closed stream, id: "
+ << stream_id;
return;
}
diff --git a/quic/core/quic_session_test.cc b/quic/core/quic_session_test.cc
index 5bfa76b..153c787 100644
--- a/quic/core/quic_session_test.cc
+++ b/quic/core/quic_session_test.cc
@@ -1623,10 +1623,8 @@
QUIC_STREAM_CANCELLED, kByteOffset);
session_.OnRstStream(rst_frame);
if (VersionHasIetfQuicFrames(transport_version())) {
- // The test is predicated on the stream being fully closed. For IETF QUIC,
- // the RST_STREAM only does one side (the read side from the perspective of
- // the node receiving the RST_STREAM). This is needed to fully close the
- // stream and therefore fulfill all of the expects.
+ // The test requires the stream to be fully closed in both directions. For
+ // IETF QUIC, the RST_STREAM only closes one side.
QuicStopSendingFrame frame(kInvalidControlFrameId, stream->id(),
QUIC_STREAM_CANCELLED);
EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
@@ -2138,10 +2136,8 @@
stream2->OnStreamReset(rst_frame);
if (VersionHasIetfQuicFrames(transport_version())) {
- // The test is predicated on the stream being fully closed. For IETF QUIC,
- // the RST_STREAM only does one side (the read side from the perspective of
- // the node receiving the RST_STREAM). This is needed to fully close the
- // stream and therefore fulfill all of the expects.
+ // The test requires the stream to be fully closed in both directions. For
+ // IETF QUIC, the RST_STREAM only closes one side.
QuicStopSendingFrame frame(kInvalidControlFrameId, stream2->id(),
QUIC_STREAM_CANCELLED);
EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
@@ -2641,15 +2637,12 @@
session_.OnStreamFrame(unidirectional_stream_frame);
}
-// Check that the OnStopSendingFrame upcall handles bad input properly
-// First test checks that invalid stream ids are handled.
-TEST_P(QuicSessionTestServer, OnStopSendingInputInvalidStreamId) {
+// Checks that invalid stream ids are handled.
+TEST_P(QuicSessionTestServer, OnStopSendingInvalidStreamId) {
if (!VersionHasIetfQuicFrames(transport_version())) {
- // Applicable only to IETF QUIC
return;
}
// Check that "invalid" stream ids are rejected.
- // Note that the notion of an invalid stream id is Google-specific.
QuicStopSendingFrame frame(1, -1, 123);
EXPECT_CALL(
*connection_,
@@ -2658,11 +2651,22 @@
session_.OnStopSendingFrame(frame);
}
-// Second test, streams in the static stream map are not subject to
-// STOP_SENDING; it's ignored.
-TEST_P(QuicSessionTestServer, OnStopSendingInputStaticStreams) {
+TEST_P(QuicSessionTestServer, OnStopSendingReadUnidirectional) {
if (!VersionHasIetfQuicFrames(transport_version())) {
- // Applicable only to IETF QUIC
+ return;
+ }
+ // It's illegal to send STOP_SENDING with a stream ID that is read-only.
+ QuicStopSendingFrame frame(1, GetNthClientInitiatedUnidirectionalId(1), 123);
+ EXPECT_CALL(
+ *connection_,
+ CloseConnection(QUIC_INVALID_STREAM_ID,
+ "Received STOP_SENDING for a read-only stream", _));
+ session_.OnStopSendingFrame(frame);
+}
+
+// Static streams ignore STOP_SENDING.
+TEST_P(QuicSessionTestServer, OnStopSendingStaticStreams) {
+ if (!VersionHasIetfQuicFrames(transport_version())) {
return;
}
QuicStreamId stream_id = 0;
@@ -2670,7 +2674,6 @@
stream_id, &session_, /*is_static*/ true, BIDIRECTIONAL);
QuicSessionPeer::ActivateStream(&session_, std::move(fake_static_stream));
// Check that a stream id in the static stream map is ignored.
- // Note that the notion of a static stream is Google-specific.
QuicStopSendingFrame frame(1, stream_id, 123);
EXPECT_CALL(*connection_,
CloseConnection(QUIC_INVALID_STREAM_ID,
@@ -2678,31 +2681,41 @@
session_.OnStopSendingFrame(frame);
}
-// Third test, if stream id specifies a closed stream:
-// return true and do not close the connection.
-TEST_P(QuicSessionTestServer, OnStopSendingInputClosedStream) {
+// If stream is write closed, do not send a RESET_STREAM frame.
+TEST_P(QuicSessionTestServer, OnStopSendingForWriteClosedStream) {
if (!VersionHasIetfQuicFrames(transport_version())) {
- // Applicable only to IETF QUIC
return;
}
TestStream* stream = session_.CreateOutgoingBidirectionalStream();
QuicStreamId stream_id = stream->id();
- // Expect these as side effect of the close operations.
- EXPECT_CALL(*connection_, SendControlFrame(_));
- EXPECT_CALL(*connection_, OnStreamReset(_, _));
stream->CloseWriteSide();
- stream->CloseReadSide();
+ EXPECT_TRUE(stream->write_side_closed());
QuicStopSendingFrame frame(1, stream_id, 123);
EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
session_.OnStopSendingFrame(frame);
}
-// Fourth test, if stream id specifies a nonexistent stream, return false and
-// close the connection
+// If stream is closed, return true and do not close the connection.
+TEST_P(QuicSessionTestServer, OnStopSendingClosedStream) {
+ if (!VersionHasIetfQuicFrames(transport_version())) {
+ return;
+ }
+
+ TestStream* stream = session_.CreateOutgoingBidirectionalStream();
+ QuicStreamId stream_id = stream->id();
+ // Expect these as side effect of closing the stream.
+ EXPECT_CALL(*connection_, SendControlFrame(_));
+ EXPECT_CALL(*connection_, OnStreamReset(_, _));
+ session_.CloseStream(stream_id);
+ QuicStopSendingFrame frame(1, stream_id, 123);
+ EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
+ session_.OnStopSendingFrame(frame);
+}
+
+// If stream id is a nonexistent stream, return false and close the connection.
TEST_P(QuicSessionTestServer, OnStopSendingInputNonExistentStream) {
if (!VersionHasIetfQuicFrames(transport_version())) {
- // Applicable only to IETF QUIC
return;
}
