Refactor ECN in QuicConnection.
Since ECN settings are no longer in PerPacketOptions, QuicConnection now owns the memory where settings are stored and last-minute checks against the current status are no longer necessary.
Protected by quic_restart_flag_quic_send_ect1.
PiperOrigin-RevId: 534592885
diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc
index d4e442f..b2c92db 100644
--- a/quiche/quic/core/http/end_to_end_test.cc
+++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -7113,7 +7113,7 @@
EXPECT_EQ(ecn->ect0, 0);
EXPECT_EQ(ecn->ect1, 0);
EXPECT_EQ(ecn->ce, 0);
- client_connection->set_ecn_codepoint(ECN_NOT_ECT);
+ EXPECT_TRUE(client_connection->set_ecn_codepoint(ECN_NOT_ECT));
client_->SendSynchronousRequest("/foo");
EXPECT_EQ(ecn->ect0, 0);
EXPECT_EQ(ecn->ect1, 0);
@@ -7133,7 +7133,7 @@
EXPECT_EQ(ecn->ect0, 0);
EXPECT_EQ(ecn->ect1, 0);
EXPECT_EQ(ecn->ce, 0);
- client_connection->set_ecn_codepoint(ECN_ECT0);
+ EXPECT_TRUE(client_connection->set_ecn_codepoint(ECN_ECT0));
client_->SendSynchronousRequest("/foo");
if (!GetQuicRestartFlag(quic_receive_ecn) ||
!GetQuicRestartFlag(quic_quiche_ecn_sockets) ||
@@ -7159,7 +7159,7 @@
EXPECT_EQ(ecn->ect0, 0);
EXPECT_EQ(ecn->ect1, 0);
EXPECT_EQ(ecn->ce, 0);
- client_connection->set_ecn_codepoint(ECN_ECT1);
+ EXPECT_TRUE(client_connection->set_ecn_codepoint(ECN_ECT1));
client_->SendSynchronousRequest("/foo");
if (!GetQuicRestartFlag(quic_receive_ecn) ||
!GetQuicRestartFlag(quic_quiche_ecn_sockets) ||
@@ -7185,7 +7185,7 @@
EXPECT_EQ(ecn->ect0, 0);
EXPECT_EQ(ecn->ect1, 0);
EXPECT_EQ(ecn->ce, 0);
- client_connection->set_ecn_codepoint(ECN_CE);
+ EXPECT_TRUE(client_connection->set_ecn_codepoint(ECN_CE));
client_->SendSynchronousRequest("/foo");
if (!GetQuicRestartFlag(quic_receive_ecn) ||
!GetQuicRestartFlag(quic_quiche_ecn_sockets) ||
@@ -7210,7 +7210,7 @@
QuicEcnCounts* ecn = QuicSentPacketManagerPeer::GetPeerEcnCounts(
QuicConnectionPeer::GetSentPacketManager(server_connection),
APPLICATION_DATA);
- server_connection->set_ecn_codepoint(ECN_ECT1);
+ EXPECT_TRUE(server_connection->set_ecn_codepoint(ECN_ECT1));
client_->SendSynchronousRequest("/foo");
// A second request provides a packet for the client ACKs to go with.
client_->SendSynchronousRequest("/foo");
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc
index 6fd85b7..f139005 100644
--- a/quiche/quic/core/quic_connection.cc
+++ b/quiche/quic/core/quic_connection.cc
@@ -3915,7 +3915,7 @@
// Only packets on the default path are in-flight.
if (!default_path_.ecn_marked_packet_acked) {
QUIC_DVLOG(1) << ENDPOINT << "First ECT packet acked on active path.";
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_send_ect1, 2, 2);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_send_ect1, 2, 3);
default_path_.ecn_marked_packet_acked = true;
}
}
@@ -4073,11 +4073,6 @@
QuicEcnCodepoint QuicConnection::GetEcnCodepointToSend(
const QuicSocketAddress& destination_address) const {
- const QuicEcnCodepoint original_codepoint =
- packet_writer_params_.ecn_codepoint;
- if (disable_ecn_codepoint_validation_) {
- return original_codepoint;
- }
// Don't send ECN marks on alternate paths. Sending ECN marks might
// cause the connectivity check to fail on some networks.
if (destination_address != peer_address()) {
@@ -4088,23 +4083,7 @@
if (in_probe_time_out_ && !default_path_.ecn_marked_packet_acked) {
return ECN_NOT_ECT;
}
- switch (original_codepoint) {
- case ECN_NOT_ECT:
- break;
- case ECN_ECT0:
- if (!sent_packet_manager_.GetSendAlgorithm()->SupportsECT0()) {
- return ECN_NOT_ECT;
- }
- break;
- case ECN_ECT1:
- if (!sent_packet_manager_.GetSendAlgorithm()->SupportsECT1()) {
- return ECN_NOT_ECT;
- }
- break;
- case ECN_CE:
- return ECN_NOT_ECT;
- }
- return original_codepoint;
+ return packet_writer_params_.ecn_codepoint;
}
WriteResult QuicConnection::SendPacketToWriter(
@@ -7371,5 +7350,35 @@
<< context.peer_address() << " after successful validation";
}
+bool QuicConnection::set_ecn_codepoint(QuicEcnCodepoint ecn_codepoint) {
+ if (!GetQuicReloadableFlag(quic_send_ect1)) {
+ return false;
+ }
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_send_ect1, 3, 3);
+ if (disable_ecn_codepoint_validation_ || ecn_codepoint == ECN_NOT_ECT) {
+ packet_writer_params_.ecn_codepoint = ecn_codepoint;
+ return true;
+ }
+ switch (ecn_codepoint) {
+ case ECN_NOT_ECT:
+ QUICHE_DCHECK(false);
+ break;
+ case ECN_ECT0:
+ if (!sent_packet_manager_.GetSendAlgorithm()->SupportsECT0()) {
+ return false;
+ }
+ break;
+ case ECN_ECT1:
+ if (!sent_packet_manager_.GetSendAlgorithm()->SupportsECT1()) {
+ return false;
+ }
+ break;
+ case ECN_CE:
+ return false;
+ }
+ packet_writer_params_.ecn_codepoint = ecn_codepoint;
+ return true;
+}
+
#undef ENDPOINT // undef for jumbo builds
} // namespace quic
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h
index a89e915..37e558d 100644
--- a/quiche/quic/core/quic_connection.h
+++ b/quiche/quic/core/quic_connection.h
@@ -1322,12 +1322,9 @@
// Sets the ECN marking for all outgoing packets, assuming that the congestion
// control supports that codepoint. QuicConnection will revert to sending
// ECN_NOT_ECT if there is evidence the path is dropping ECN-marked packets,
- // or if the peer provides invalid ECN feedback.
- void set_ecn_codepoint(QuicEcnCodepoint ecn_codepoint) {
- if (GetQuicReloadableFlag(quic_send_ect1)) {
- packet_writer_params_.ecn_codepoint = ecn_codepoint;
- }
- }
+ // or if the peer provides invalid ECN feedback. Returns false if the current
+ // configuration prevents setting the desired codepoint.
+ bool set_ecn_codepoint(QuicEcnCodepoint ecn_codepoint);
QuicEcnCodepoint ecn_codepoint() const {
return packet_writer_params_.ecn_codepoint;
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index 4e326fd..58bb93a 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -17111,15 +17111,19 @@
TEST_P(QuicConnectionTest, EcnCodepointsRejected) {
SetQuicReloadableFlag(quic_send_ect1, true);
for (QuicEcnCodepoint ecn : {ECN_NOT_ECT, ECN_ECT0, ECN_ECT1, ECN_CE}) {
- connection_.set_ecn_codepoint(ecn);
if (ecn == ECN_ECT0) {
EXPECT_CALL(*send_algorithm_, SupportsECT0()).WillOnce(Return(false));
} else if (ecn == ECN_ECT1) {
EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillOnce(Return(false));
}
+ if (ecn == ECN_NOT_ECT) {
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ecn));
+ } else {
+ EXPECT_FALSE(connection_.set_ecn_codepoint(ecn));
+ }
+ EXPECT_EQ(connection_.ecn_codepoint(), ECN_NOT_ECT);
EXPECT_CALL(connection_, OnSerializedPacket(_));
SendPing();
- EXPECT_EQ(connection_.ecn_codepoint(), ecn);
EXPECT_EQ(writer_->last_ecn_sent(), ECN_NOT_ECT);
}
}
@@ -17127,19 +17131,23 @@
TEST_P(QuicConnectionTest, EcnCodepointsAccepted) {
SetQuicReloadableFlag(quic_send_ect1, true);
for (QuicEcnCodepoint ecn : {ECN_NOT_ECT, ECN_ECT0, ECN_ECT1, ECN_CE}) {
- connection_.set_ecn_codepoint(ecn);
if (ecn == ECN_ECT0) {
EXPECT_CALL(*send_algorithm_, SupportsECT0()).WillOnce(Return(true));
} else if (ecn == ECN_ECT1) {
EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillOnce(Return(true));
}
+ if (ecn == ECN_CE) {
+ EXPECT_FALSE(connection_.set_ecn_codepoint(ecn));
+ } else {
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ecn));
+ }
EXPECT_CALL(connection_, OnSerializedPacket(_));
SendPing();
QuicEcnCodepoint expected_codepoint = ecn;
if (ecn == ECN_CE) {
- expected_codepoint = ECN_NOT_ECT;
+ expected_codepoint = ECN_ECT1;
}
- EXPECT_EQ(connection_.ecn_codepoint(), ecn);
+ EXPECT_EQ(connection_.ecn_codepoint(), expected_codepoint);
EXPECT_EQ(writer_->last_ecn_sent(), expected_codepoint);
}
}
@@ -17147,7 +17155,7 @@
TEST_P(QuicConnectionTest, EcnCodepointsRejectedIfFlagIsFalse) {
SetQuicReloadableFlag(quic_send_ect1, false);
for (QuicEcnCodepoint ecn : {ECN_NOT_ECT, ECN_ECT0, ECN_ECT1, ECN_CE}) {
- connection_.set_ecn_codepoint(ecn);
+ EXPECT_FALSE(connection_.set_ecn_codepoint(ecn));
EXPECT_CALL(connection_, OnSerializedPacket(_));
SendPing();
EXPECT_EQ(connection_.ecn_codepoint(), ECN_NOT_ECT);
@@ -17159,7 +17167,7 @@
SetQuicReloadableFlag(quic_send_ect1, true);
QuicConnectionPeer::DisableEcnCodepointValidation(&connection_);
for (QuicEcnCodepoint ecn : {ECN_NOT_ECT, ECN_ECT0, ECN_ECT1, ECN_CE}) {
- connection_.set_ecn_codepoint(ecn);
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ecn));
EXPECT_CALL(connection_, OnSerializedPacket(_));
SendPing();
EXPECT_EQ(connection_.ecn_codepoint(), ecn);
@@ -17169,8 +17177,8 @@
TEST_P(QuicConnectionTest, RtoDisablesEcnMarking) {
SetQuicReloadableFlag(quic_send_ect1, true);
- EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillRepeatedly(Return(true));
- connection_.set_ecn_codepoint(ECN_ECT1);
+ EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillOnce(Return(true));
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ECN_ECT1));
QuicPacketCreatorPeer::SetPacketNumber(
QuicConnectionPeer::GetPacketCreator(&connection_), 1);
SendPing();
@@ -17184,8 +17192,9 @@
}
TEST_P(QuicConnectionTest, RtoDoesntDisableEcnMarkingIfEcnAcked) {
- EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillRepeatedly(Return(true));
- connection_.set_ecn_codepoint(ECN_ECT1);
+ SetQuicReloadableFlag(quic_send_ect1, true);
+ EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillOnce(Return(true));
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ECN_ECT1));
QuicPacketCreatorPeer::SetPacketNumber(
QuicConnectionPeer::GetPacketCreator(&connection_), 1);
if (!GetQuicReloadableFlag(quic_send_ect1)) {
@@ -17209,8 +17218,8 @@
TEST_P(QuicConnectionTest, InvalidFeedbackCancelsEcn) {
SetQuicReloadableFlag(quic_send_ect1, true);
- EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillRepeatedly(Return(true));
- connection_.set_ecn_codepoint(ECN_ECT1);
+ EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillOnce(Return(true));
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ECN_ECT1));
EXPECT_EQ(connection_.ecn_codepoint(), ECN_ECT1);
if (!GetQuicReloadableFlag(quic_send_ect1)) {
EXPECT_QUIC_BUG(connection_.OnInvalidEcnFeedback(),
@@ -17224,8 +17233,8 @@
TEST_P(QuicConnectionTest, StateMatchesSentEcn) {
SetQuicReloadableFlag(quic_send_ect1, true);
- EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillRepeatedly(Return(true));
- connection_.set_ecn_codepoint(ECN_ECT1);
+ EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillOnce(Return(true));
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ECN_ECT1));
SendPing();
QuicSentPacketManager* sent_packet_manager =
QuicConnectionPeer::GetSentPacketManager(&connection_);
@@ -17240,8 +17249,8 @@
return;
}
SetQuicReloadableFlag(quic_send_ect1, true);
- EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillRepeatedly(Return(true));
- connection_.set_ecn_codepoint(ECN_ECT1);
+ EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillOnce(Return(true));
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ECN_ECT1));
// All these steps are necessary to send an INITIAL ping and save it to be
// coalesced, instead of just calling SendPing() and sending it immediately.
char buffer[1000];
@@ -17253,9 +17262,9 @@
creator_, frames, buffer, sizeof(buffer));
connection_.SendOrQueuePacket(std::move(packet1));
creator_->set_encryption_level(ENCRYPTION_FORWARD_SECURE);
- EXPECT_CALL(*send_algorithm_, SupportsECT0()).WillRepeatedly(Return(true));
+ EXPECT_CALL(*send_algorithm_, SupportsECT0()).WillOnce(Return(true));
// If not for the line below, these packets would coalesce.
- connection_.set_ecn_codepoint(ECN_ECT0);
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ECN_ECT0));
EXPECT_EQ(writer_->packets_write_attempts(), 0);
SendPing();
EXPECT_EQ(writer_->packets_write_attempts(), 2);
@@ -17264,13 +17273,13 @@
TEST_P(QuicConnectionTest, BufferedPacketRetainsOldEcn) {
SetQuicReloadableFlag(quic_send_ect1, true);
- EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillRepeatedly(Return(true));
- connection_.set_ecn_codepoint(ECN_ECT1);
+ EXPECT_CALL(*send_algorithm_, SupportsECT1()).WillOnce(Return(true));
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ECN_ECT1));
writer_->SetWriteBlocked();
EXPECT_CALL(visitor_, OnWriteBlocked()).Times(2);
SendPing();
- EXPECT_CALL(*send_algorithm_, SupportsECT0()).WillRepeatedly(Return(true));
- connection_.set_ecn_codepoint(ECN_ECT0);
+ EXPECT_CALL(*send_algorithm_, SupportsECT0()).WillOnce(Return(true));
+ EXPECT_TRUE(connection_.set_ecn_codepoint(ECN_ECT0));
writer_->SetWritable();
connection_.OnCanWrite();
EXPECT_EQ(writer_->last_ecn_sent(), ECN_ECT1);
diff --git a/quiche/quic/core/quic_sent_packet_manager.cc b/quiche/quic/core/quic_sent_packet_manager.cc
index 6dc502e..2f030ec 100644
--- a/quiche/quic/core/quic_sent_packet_manager.cc
+++ b/quiche/quic/core/quic_sent_packet_manager.cc
@@ -1421,7 +1421,7 @@
// Validate ECN feedback.
absl::optional<QuicEcnCounts> valid_ecn_counts;
if (GetQuicReloadableFlag(quic_send_ect1)) {
- QUIC_RELOADABLE_FLAG_COUNT_N(quic_send_ect1, 1, 2);
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_send_ect1, 1, 3);
if (IsEcnFeedbackValid(acked_packet_number_space, ecn_counts,
newly_acked_ect0, newly_acked_ect1)) {
valid_ecn_counts = ecn_counts;
