Skip to content

Commit

Permalink
Deliver STOP_SENDING to zombie streams.
Browse files Browse the repository at this point in the history
This is to fix a bug that a zombie stream is not closed when receiving STOP_SENDING. Credit to external report in #76

Protected by FLAGS_quic_reloadable_flag_quic_deliver_stop_sending_to_zombie_streams.

PiperOrigin-RevId: 677548901
  • Loading branch information
yangfanud authored and copybara-github committed Sep 22, 2024
1 parent 9232c78 commit 93f3b29
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 1 deletion.
1 change: 1 addition & 0 deletions quiche/common/quiche_feature_flags_list.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ QUICHE_FLAG(bool, quiche_reloadable_flag_quic_conservative_cwnd_and_pacing_gains
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_default_enable_5rto_blackhole_detection2, true, true, "If true, default-enable 5RTO blachole detection.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_default_to_bbr, true, false, "When true, defaults to BBR congestion control instead of Cubic.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_default_to_bbr_v2, false, false, "If true, use BBRv2 as the default congestion controller. Takes precedence over --quic_default_to_bbr.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_deliver_stop_sending_to_zombie_streams, false, false, "If true, deliver STOP_SENDING to zombie streams.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_disable_batch_write, false, false, "If true, round-robin stream writes instead of batching in QuicWriteBlockedList.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_disable_server_blackhole_detection, false, false, "If true, disable blackhole detection on server side.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_disable_version_draft_29, false, false, "If true, disable QUIC version h3-29.")
Expand Down
17 changes: 16 additions & 1 deletion quiche/quic/core/quic_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,22 @@ void QuicSession::OnStopSendingFrame(const QuicStopSendingFrame& frame) {
return;
}

QuicStream* stream = GetOrCreateStream(stream_id);
QuicStream* stream = nullptr;
if (enable_stop_sending_for_zombie_streams_) {
stream = GetStream(stream_id);
if (stream != nullptr) {
if (stream->IsZombie()) {
QUIC_RELOADABLE_FLAG_COUNT_N(
quic_deliver_stop_sending_to_zombie_streams, 1, 3);
} else {
QUIC_RELOADABLE_FLAG_COUNT_N(
quic_deliver_stop_sending_to_zombie_streams, 2, 3);
}
stream->OnStopSending(frame.error());
return;
}
}
stream = GetOrCreateStream(stream_id);
if (!stream) {
// Errors are handled by GetOrCreateStream.
return;
Expand Down
7 changes: 7 additions & 0 deletions quiche/quic/core/quic_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,10 @@ class QUICHE_EXPORT QuicSession
// Returns the priority type used by the streams in the session.
QuicPriorityType priority_type() const { return priority_type_; }

bool enable_stop_sending_for_zombie_streams() const {
return enable_stop_sending_for_zombie_streams_;
}

protected:
using StreamMap =
absl::flat_hash_map<QuicStreamId, std::unique_ptr<QuicStream>>;
Expand Down Expand Up @@ -1094,6 +1098,9 @@ class QUICHE_EXPORT QuicSession
std::unique_ptr<QuicAlarm> stream_count_reset_alarm_;

QuicPriorityType priority_type_;

const bool enable_stop_sending_for_zombie_streams_ =
GetQuicReloadableFlag(quic_deliver_stop_sending_to_zombie_streams);
};

} // namespace quic
Expand Down
38 changes: 38 additions & 0 deletions quiche/quic/core/quic_session_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3047,6 +3047,44 @@ TEST_P(QuicSessionTestServer, OnStopSendingForWriteClosedStream) {
session_.OnStopSendingFrame(frame);
}

// Regression test for b/368421586.
TEST_P(QuicSessionTestServer, OnStopSendingForZombieStreams) {
if (!VersionHasIetfQuicFrames(transport_version())) {
return;
}
CompleteHandshake();
session_.set_writev_consumes_all_data(true);

TestStream* stream = session_.CreateOutgoingBidirectionalStream();
std::string body(100, '.');
QuicStreamPeer::CloseReadSide(stream);
stream->WriteOrBufferData(body, true, nullptr);
EXPECT_TRUE(stream->IsWaitingForAcks());
// Verify that the stream is a zombie.
EXPECT_TRUE(stream->IsZombie());
ASSERT_EQ(0u, session_.closed_streams()->size());

QuicStopSendingFrame frame(1, stream->id(), QUIC_STREAM_CANCELLED);
EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(0);
if (GetQuicReloadableFlag(quic_deliver_stop_sending_to_zombie_streams)) {
EXPECT_CALL(*connection_, SendControlFrame(_)).Times(1);
EXPECT_CALL(*connection_, OnStreamReset(_, _)).Times(1);
} else {
EXPECT_CALL(*connection_, SendControlFrame(_)).Times(0);
EXPECT_CALL(*connection_, OnStreamReset(_, _)).Times(0);
}
session_.OnStopSendingFrame(frame);
if (GetQuicReloadableFlag(quic_deliver_stop_sending_to_zombie_streams)) {
// STOP_SENDING should cause the stream to be closed.
EXPECT_FALSE(stream->IsZombie());
EXPECT_EQ(1u, session_.closed_streams()->size());
} else {
// STOP_SENDING is not delivered to zombie streams.
EXPECT_TRUE(stream->IsZombie());
EXPECT_EQ(0u, session_.closed_streams()->size());
}
}

// If stream is closed, return true and do not close the connection.
TEST_P(QuicSessionTestServer, OnStopSendingClosedStream) {
if (!VersionHasIetfQuicFrames(transport_version())) {
Expand Down
6 changes: 6 additions & 0 deletions quiche/quic/core/quic_stream.cc
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,12 @@ bool QuicStream::OnStopSending(QuicResetStreamError error) {

stream_error_ = error;
MaybeSendRstStream(error);
if (session()->enable_stop_sending_for_zombie_streams() &&
read_side_closed_ && write_side_closed_ && !IsWaitingForAcks()) {
QUIC_RELOADABLE_FLAG_COUNT_N(quic_deliver_stop_sending_to_zombie_streams, 3,
3);
session()->MaybeCloseZombieStream(id_);
}
return true;
}

Expand Down

0 comments on commit 93f3b29

Please sign in to comment.