Do not drop too many packages in PacketDroppingTestWriter.
After each dropped package, succeed in writing at least two packages before
dropping another one (unless fake drop percentage is 100). This is to avoid
test flakiness due to too many dropped packages. For example, if both the
server and the client happen to drop every other package, no probe ack will ever
be received. See https://crbug.com/1095063 for more context.
Without this change, I can reproduce the following errors in AckNotifierWithPacketLossAndBlockedSocket tests:
- in the internal code base, test flakiness (QUIC_TOO_MANY_RTOS errors),
- in Chromium debug build, test flakiness (QUIC_TOO_MANY_RTOS errors),
- in Chromium MSAN build, both test flakiness and test timeouts.
With this change, I ran all of AckNotifierWithPacketLossAndBlockedSocket in Chromium in both setups, and all of EndToEndTests with internal tools, each of them 1000 times, and they all passed.
Test-only change
PiperOrigin-RevId: 319202623
Change-Id: I8aedcf3e790a947d3d6fa341f6064a7465156472
diff --git a/quic/test_tools/packet_dropping_test_writer.cc b/quic/test_tools/packet_dropping_test_writer.cc
index c897fdc..d840e4d 100644
--- a/quic/test_tools/packet_dropping_test_writer.cc
+++ b/quic/test_tools/packet_dropping_test_writer.cc
@@ -10,7 +10,11 @@
namespace quic {
namespace test {
-const int32_t kMaxConsecutivePacketLoss = 3;
+// Every dropped packet must be followed by this number of succesfully written
+// packets. This is to avoid flaky test failures and timeouts, for example, in
+// case both the client and the server drop every other packet (which is
+// statistically possible even if drop percentage is less than 50%).
+const int32_t kMinSuccesfulWritesAfterPacketLoss = 2;
// An alarm that is scheduled if a blocked socket is simulated to indicate
// it's writable again.
@@ -49,14 +53,16 @@
: clock_(nullptr),
cur_buffer_size_(0),
num_calls_to_write_(0),
+ // Do not require any number of successful writes before the first dropped
+ // packet.
+ num_consecutive_succesful_writes_(kMinSuccesfulWritesAfterPacketLoss),
fake_packet_loss_percentage_(0),
fake_drop_first_n_packets_(0),
fake_blocked_socket_percentage_(0),
fake_packet_reorder_percentage_(0),
fake_packet_delay_(QuicTime::Delta::Zero()),
fake_bandwidth_(QuicBandwidth::Zero()),
- buffer_size_(0),
- num_consecutive_packet_lost_(0) {
+ buffer_size_(0) {
uint64_t seed = QuicRandom::GetInstance()->RandUint64();
QUIC_LOG(INFO) << "Seeding packet loss with " << seed;
simple_random_.set_seed(seed);
@@ -90,34 +96,39 @@
static_cast<uint64_t>(fake_drop_first_n_packets_)) {
QUIC_DVLOG(1) << "Dropping first " << fake_drop_first_n_packets_
<< " packets (packet number " << num_calls_to_write_ << ")";
+ num_consecutive_succesful_writes_ = 0;
return WriteResult(WRITE_STATUS_OK, buf_len);
}
- const int32_t kMaxPacketLossPercentage =
- kMaxConsecutivePacketLoss * 100.0 / (kMaxConsecutivePacketLoss + 1);
- if (fake_packet_loss_percentage_ > 0 &&
- // Do not allow too many consecutive packet drops to avoid test flakiness.
- (num_consecutive_packet_lost_ <= kMaxConsecutivePacketLoss ||
- // Allow as many consecutive packet drops as possbile if
- // |fake_packet_lost_percentage_| is large enough. Without this exception
- // it is hard to simulate high loss rate, like 100%.
- fake_packet_loss_percentage_ > kMaxPacketLossPercentage) &&
- (simple_random_.RandUint64() % 100 <
- static_cast<uint64_t>(fake_packet_loss_percentage_))) {
- QUIC_DVLOG(1) << "Dropping packet.";
- ++num_consecutive_packet_lost_;
+
+ // Drop every packet at 100%, otherwise always succeed for at least
+ // kMinSuccesfulWritesAfterPacketLoss packets between two dropped ones.
+ if (fake_packet_loss_percentage_ == 100 ||
+ (fake_packet_loss_percentage_ > 0 &&
+ num_consecutive_succesful_writes_ >=
+ kMinSuccesfulWritesAfterPacketLoss &&
+ (simple_random_.RandUint64() % 100 <
+ static_cast<uint64_t>(fake_packet_loss_percentage_)))) {
+ QUIC_DVLOG(1) << "Dropping packet " << num_calls_to_write_;
+ num_consecutive_succesful_writes_ = 0;
return WriteResult(WRITE_STATUS_OK, buf_len);
} else {
- num_consecutive_packet_lost_ = 0;
+ ++num_consecutive_succesful_writes_;
}
+
if (fake_blocked_socket_percentage_ > 0 &&
simple_random_.RandUint64() % 100 <
static_cast<uint64_t>(fake_blocked_socket_percentage_)) {
CHECK(on_can_write_ != nullptr);
- QUIC_DVLOG(1) << "Blocking socket.";
+ QUIC_DVLOG(1) << "Blocking socket for packet " << num_calls_to_write_;
if (!write_unblocked_alarm_->IsSet()) {
// Set the alarm to fire immediately.
write_unblocked_alarm_->Set(clock_->ApproximateNow());
}
+
+ // Dropping this packet on retry could result in PTO timeout,
+ // make sure to avoid this.
+ num_consecutive_succesful_writes_ = 0;
+
return WriteResult(WRITE_STATUS_BLOCKED, EAGAIN);
}
@@ -225,13 +236,6 @@
on_can_write_->OnCanWrite();
}
-void PacketDroppingTestWriter::set_fake_packet_loss_percentage(
- int32_t fake_packet_loss_percentage) {
- QuicWriterMutexLock lock(&config_mutex_);
- fake_packet_loss_percentage_ = fake_packet_loss_percentage;
- num_consecutive_packet_lost_ = 0;
-}
-
PacketDroppingTestWriter::DelayedWrite::DelayedWrite(
const char* buffer,
size_t buf_len,
diff --git a/quic/test_tools/packet_dropping_test_writer.h b/quic/test_tools/packet_dropping_test_writer.h
index f066e91..47a3216 100644
--- a/quic/test_tools/packet_dropping_test_writer.h
+++ b/quic/test_tools/packet_dropping_test_writer.h
@@ -73,7 +73,14 @@
void OnCanWrite();
// The percent of time a packet is simulated as being lost.
- void set_fake_packet_loss_percentage(int32_t fake_packet_loss_percentage);
+ // If |fake_packet_loss_percentage| is 100, then all packages are lost.
+ // Otherwise actual percentage will be lower than
+ // |fake_packet_loss_percentage|, because every dropped package is followed by
+ // a minimum number of successfully written packets.
+ void set_fake_packet_loss_percentage(int32_t fake_packet_loss_percentage) {
+ QuicWriterMutexLock lock(&config_mutex_);
+ fake_packet_loss_percentage_ = fake_packet_loss_percentage;
+ }
// Simulate dropping the first n packets unconditionally.
// Subsequent packets will be lost at fake_packet_loss_percentage_ if set.
@@ -159,6 +166,7 @@
DelayedPacketList delayed_packets_;
QuicByteCount cur_buffer_size_;
uint64_t num_calls_to_write_;
+ int32_t num_consecutive_succesful_writes_;
QuicMutex config_mutex_;
int32_t fake_packet_loss_percentage_ QUIC_GUARDED_BY(config_mutex_);
@@ -168,7 +176,6 @@
QuicTime::Delta fake_packet_delay_ QUIC_GUARDED_BY(config_mutex_);
QuicBandwidth fake_bandwidth_ QUIC_GUARDED_BY(config_mutex_);
QuicByteCount buffer_size_ QUIC_GUARDED_BY(config_mutex_);
- int32_t num_consecutive_packet_lost_ QUIC_GUARDED_BY(config_mutex_);
};
} // namespace test
