Parse and frame the updated ACK_FREQUENCY frame and implement the logic for receiving it.
In a change from draft-iyengar-quic-delayed-ack-01, previously missing packets are always acknowledged regardless of reordering_threshold.
Protected by FLAGS_quic_reloadable_flag_quic_receive_ack_frequency.
PiperOrigin-RevId: 765276193
diff --git a/quiche/quic/core/frames/quic_ack_frequency_frame.cc b/quiche/quic/core/frames/quic_ack_frequency_frame.cc
index 7faf7f1..bcb6888 100644
--- a/quiche/quic/core/frames/quic_ack_frequency_frame.cc
+++ b/quiche/quic/core/frames/quic_ack_frequency_frame.cc
@@ -5,25 +5,27 @@
#include "quiche/quic/core/frames/quic_ack_frequency_frame.h"
#include <cstdint>
-#include <limits>
#include <ostream>
namespace quic {
QuicAckFrequencyFrame::QuicAckFrequencyFrame(
QuicControlFrameId control_frame_id, uint64_t sequence_number,
- uint64_t packet_tolerance, QuicTime::Delta max_ack_delay)
+ uint64_t ack_eliciting_threshold, QuicTime::Delta requested_max_ack_delay,
+ uint64_t reordering_threshold)
: control_frame_id(control_frame_id),
sequence_number(sequence_number),
- packet_tolerance(packet_tolerance),
- max_ack_delay(max_ack_delay) {}
+ ack_eliciting_threshold(ack_eliciting_threshold),
+ requested_max_ack_delay(requested_max_ack_delay),
+ reordering_threshold(reordering_threshold) {}
std::ostream& operator<<(std::ostream& os, const QuicAckFrequencyFrame& frame) {
os << "{ control_frame_id: " << frame.control_frame_id
<< ", sequence_number: " << frame.sequence_number
- << ", packet_tolerance: " << frame.packet_tolerance
- << ", max_ack_delay_ms: " << frame.max_ack_delay.ToMilliseconds()
- << ", ignore_order: " << frame.ignore_order << " }\n";
+ << ", ack_eliciting_threshold: " << frame.ack_eliciting_threshold
+ << ", requested_max_ack_delay_ms: "
+ << frame.requested_max_ack_delay.ToMilliseconds()
+ << ", reordering_threshold: " << frame.reordering_threshold << " }\n";
return os;
}
diff --git a/quiche/quic/core/frames/quic_ack_frequency_frame.h b/quiche/quic/core/frames/quic_ack_frequency_frame.h
index 660b1a7..22fe058 100644
--- a/quiche/quic/core/frames/quic_ack_frequency_frame.h
+++ b/quiche/quic/core/frames/quic_ack_frequency_frame.h
@@ -11,7 +11,7 @@
#include "quiche/quic/core/quic_constants.h"
#include "quiche/quic/core/quic_time.h"
#include "quiche/quic/core/quic_types.h"
-#include "quiche/quic/platform/api/quic_export.h"
+#include "quiche/common/platform/api/quiche_export.h"
namespace quic {
@@ -21,28 +21,32 @@
std::ostream& os, const QuicAckFrequencyFrame& ack_frequency_frame);
QuicAckFrequencyFrame() = default;
+ // Set a default for reordering_threshold so it does not break Envoy tests.
QuicAckFrequencyFrame(QuicControlFrameId control_frame_id,
- uint64_t sequence_number, uint64_t packet_tolerance,
- QuicTime::Delta max_ack_delay);
+ uint64_t sequence_number,
+ uint64_t ack_eliciting_threshold,
+ QuicTime::Delta requested_max_ack_delay,
+ uint64_t reordering_threshold = 1);
// A unique identifier of this control frame. 0 when this frame is
// received, and non-zero when sent.
QuicControlFrameId control_frame_id = kInvalidControlFrameId;
- // If true, do not ack immediately upon observeation of packet reordering.
- bool ignore_order = false;
-
// Sequence number assigned to the ACK_FREQUENCY frame by the sender to allow
// receivers to ignore obsolete frames.
uint64_t sequence_number = 0;
- // The maximum number of ack-eliciting packets after which the receiver sends
- // an acknowledgement. Invald if == 0.
- uint64_t packet_tolerance = 2;
+ // The maximum number of ack-eliciting packets that do not require an
+ // acknowledgement.
+ uint64_t ack_eliciting_threshold = 1;
// The maximum time that ack packets can be delayed.
- QuicTime::Delta max_ack_delay =
+ QuicTime::Delta requested_max_ack_delay =
QuicTime::Delta::FromMilliseconds(kDefaultPeerDelayedAckTimeMs);
+
+ // The number of out-of-order packets necessary to trigger an immediate
+ // acknowledgement. If zero, OOO packets are not acked immediately.
+ uint64_t reordering_threshold = 1;
};
} // namespace quic
diff --git a/quiche/quic/core/frames/quic_frames_test.cc b/quiche/quic/core/frames/quic_frames_test.cc
index e6a843d..eeed473 100644
--- a/quiche/quic/core/frames/quic_frames_test.cc
+++ b/quiche/quic/core/frames/quic_frames_test.cc
@@ -327,12 +327,13 @@
EXPECT_TRUE(IsControlFrame(frame.type));
}
-TEST_F(QuicFramesTest, QuicAckFreuqncyFrameToString) {
+TEST_F(QuicFramesTest, QuicAckFrequencyFrameToString) {
QuicAckFrequencyFrame ack_frequency_frame;
ack_frequency_frame.sequence_number = 1;
- ack_frequency_frame.packet_tolerance = 2;
- ack_frequency_frame.max_ack_delay = QuicTime::Delta::FromMilliseconds(25);
- ack_frequency_frame.ignore_order = false;
+ ack_frequency_frame.ack_eliciting_threshold = 2;
+ ack_frequency_frame.requested_max_ack_delay =
+ QuicTime::Delta::FromMilliseconds(25);
+ ack_frequency_frame.reordering_threshold = false;
QuicFrame frame(&ack_frequency_frame);
ASSERT_EQ(ACK_FREQUENCY_FRAME, frame.type);
SetControlFrameId(6, &frame);
@@ -340,8 +341,8 @@
std::ostringstream stream;
stream << *frame.ack_frequency_frame;
EXPECT_EQ(
- "{ control_frame_id: 6, sequence_number: 1, packet_tolerance: 2, "
- "max_ack_delay_ms: 25, ignore_order: 0 }\n",
+ "{ control_frame_id: 6, sequence_number: 1, ack_eliciting_threshold: 2, "
+ "requested_max_ack_delay_ms: 25, reordering_threshold: 0 }\n",
stream.str());
EXPECT_TRUE(IsControlFrame(frame.type));
}
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc
index 214414a..74ac6e4 100644
--- a/quiche/quic/core/quic_connection.cc
+++ b/quiche/quic/core/quic_connection.cc
@@ -607,6 +607,8 @@
if (config.GetMinAckDelayDraft10ToSendMs() <=
config.GetMaxAckDelayToSendMs()) { // MinAckDelay is valid.
set_can_receive_ack_frequency_immediate_ack(true);
+ local_min_ack_delay_ = QuicTime::Delta::FromMilliseconds(
+ config.GetMinAckDelayDraft10ToSendMs());
} else {
QUIC_BUG(quic_bug_min_ack_delay_too_high)
<< "MinAckDelay higher than MaxAckDelay";
@@ -2116,6 +2118,15 @@
QUIC_LOG_EVERY_N_SEC(ERROR, 120) << "Get unexpected AckFrequencyFrame.";
return false;
}
+ if (frame.requested_max_ack_delay < local_min_ack_delay_) {
+ QUIC_LOG_EVERY_N_SEC(ERROR, 120)
+ << "Received AckFrequencyFrame with requested_max_ack_delay "
+ << frame.requested_max_ack_delay
+ << " which is less than the minimum ack delay " << local_min_ack_delay_;
+ CloseConnection(IETF_QUIC_PROTOCOL_VIOLATION, "MaxAckDelay too small",
+ ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+ return false;
+ }
if (auto packet_number_space =
QuicUtils::GetPacketNumberSpace(
last_received_packet_info_.decrypted_level) == APPLICATION_DATA) {
@@ -4087,10 +4098,11 @@
QUIC_RELOADABLE_FLAG_COUNT_N(quic_can_send_ack_frequency, 2, 3);
auto ack_frequency_frame =
sent_packet_manager_.GetUpdatedAckFrequencyFrame();
- // This AckFrequencyFrame is meant to only update the max_ack_delay. Set
- // packet tolerance to the default value for now.
- ack_frequency_frame.packet_tolerance =
+ // This AckFrequencyFrame is meant to only update the max_ack_delay. All
+ // other values are set to the default.
+ ack_frequency_frame.ack_eliciting_threshold =
kDefaultRetransmittablePacketsBeforeAck;
+ ack_frequency_frame.reordering_threshold = 1;
visitor_->SendAckFrequency(ack_frequency_frame);
if (!connected_) {
return;
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h
index a2f7bfa..e2073c5 100644
--- a/quiche/quic/core/quic_connection.h
+++ b/quiche/quic/core/quic_connection.h
@@ -2530,6 +2530,9 @@
QuicTime::Delta multi_port_probing_interval_;
+ // The minimum ack delay time advertised to the peer via transport parameter.
+ QuicTime::Delta local_min_ack_delay_ = QuicTime::Delta::Zero();
+
std::unique_ptr<MultiPortStats> multi_port_stats_;
// If true, connection will migrate to multi-port path upon path degrading.
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index 1950e10..915f65d 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -4,8 +4,6 @@
#include "quiche/quic/core/quic_connection.h"
-#include <errno.h>
-
#include <algorithm>
#include <cstdint>
#include <memory>
@@ -3592,7 +3590,7 @@
EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
QuicAckFrequencyFrame ack_frequency_frame;
- ack_frequency_frame.packet_tolerance = 3;
+ ack_frequency_frame.ack_eliciting_threshold = 2;
ProcessFramePacketAtLevel(1, QuicFrame(&ack_frequency_frame),
ENCRYPTION_FORWARD_SECURE);
@@ -13299,8 +13297,8 @@
// Send packet 101.
SendStreamDataToPeer(/*id=*/1, "bar", /*offset=*/3, NO_FIN, nullptr);
- EXPECT_EQ(captured_frame.packet_tolerance, 10u);
- EXPECT_EQ(captured_frame.max_ack_delay,
+ EXPECT_EQ(captured_frame.ack_eliciting_threshold, 10u);
+ EXPECT_EQ(captured_frame.requested_max_ack_delay,
QuicTime::Delta::FromMilliseconds(GetDefaultDelayedAckTimeMs()));
// Sending packet 102 does not trigger sending another AckFrequencyFrame.
@@ -13339,8 +13337,8 @@
connection_.OnHandshakeComplete();
- EXPECT_EQ(captured_frame.packet_tolerance, 2u);
- EXPECT_EQ(captured_frame.max_ack_delay,
+ EXPECT_EQ(captured_frame.ack_eliciting_threshold, 2u);
+ EXPECT_EQ(captured_frame.requested_max_ack_delay,
QuicTime::Delta::FromMilliseconds(GetDefaultDelayedAckTimeMs()));
}
diff --git a/quiche/quic/core/quic_control_frame_manager.cc b/quiche/quic/core/quic_control_frame_manager.cc
index 9dd73a1..b5dbbba 100644
--- a/quiche/quic/core/quic_control_frame_manager.cc
+++ b/quiche/quic/core/quic_control_frame_manager.cc
@@ -137,11 +137,12 @@
QuicControlFrameId control_frame_id = ++last_control_frame_id_;
// Using the control_frame_id for sequence_number here leaves gaps in
// sequence_number.
- WriteOrBufferQuicFrame(
- QuicFrame(new QuicAckFrequencyFrame(control_frame_id,
- /*sequence_number=*/control_frame_id,
- ack_frequency_frame.packet_tolerance,
- ack_frequency_frame.max_ack_delay)));
+ WriteOrBufferQuicFrame(QuicFrame(
+ new QuicAckFrequencyFrame(control_frame_id,
+ /*sequence_number=*/control_frame_id,
+ ack_frequency_frame.ack_eliciting_threshold,
+ ack_frequency_frame.requested_max_ack_delay,
+ ack_frequency_frame.reordering_threshold)));
}
void QuicControlFrameManager::WriteOrBufferNewConnectionId(
diff --git a/quiche/quic/core/quic_control_frame_manager_test.cc b/quiche/quic/core/quic_control_frame_manager_test.cc
index a007d92..d58ffed 100644
--- a/quiche/quic/core/quic_control_frame_manager_test.cc
+++ b/quiche/quic/core/quic_control_frame_manager_test.cc
@@ -421,8 +421,9 @@
TEST_F(QuicControlFrameManagerTest, SendAndAckAckFrequencyFrame) {
// Send AckFrequencyFrame
QuicAckFrequencyFrame frame_to_send;
- frame_to_send.packet_tolerance = 10;
- frame_to_send.max_ack_delay = QuicTime::Delta::FromMilliseconds(24);
+ frame_to_send.ack_eliciting_threshold = 10;
+ frame_to_send.requested_max_ack_delay = QuicTime::Delta::FromMilliseconds(24);
+ frame_to_send.reordering_threshold = 3;
EXPECT_CALL(*session_, WriteControlFrame(_, _))
.WillOnce(Invoke(&ClearControlFrameWithTransmissionType));
manager_->WriteOrBufferAckFrequency(frame_to_send);
diff --git a/quiche/quic/core/quic_framer.cc b/quiche/quic/core/quic_framer.cc
index a506624..e01b1af 100644
--- a/quiche/quic/core/quic_framer.cc
+++ b/quiche/quic/core/quic_framer.cc
@@ -628,10 +628,10 @@
const QuicAckFrequencyFrame& frame) {
return QuicDataWriter::GetVarInt62Len(IETF_ACK_FREQUENCY) +
QuicDataWriter::GetVarInt62Len(frame.sequence_number) +
- QuicDataWriter::GetVarInt62Len(frame.packet_tolerance) +
- QuicDataWriter::GetVarInt62Len(frame.max_ack_delay.ToMicroseconds()) +
- // One byte for encoding boolean
- 1;
+ QuicDataWriter::GetVarInt62Len(frame.ack_eliciting_threshold) +
+ QuicDataWriter::GetVarInt62Len(
+ frame.requested_max_ack_delay.ToMicroseconds()) +
+ QuicDataWriter::GetVarInt62Len(frame.reordering_threshold);
}
// static
@@ -3391,14 +3391,10 @@
return false;
}
- if (!reader->ReadVarInt62(&frame->packet_tolerance)) {
+ if (!reader->ReadVarInt62(&frame->ack_eliciting_threshold)) {
set_detailed_error("Unable to read packet tolerance.");
return false;
}
- if (frame->packet_tolerance == 0) {
- set_detailed_error("Invalid packet tolerance.");
- return false;
- }
uint64_t max_ack_delay_us;
if (!reader->ReadVarInt62(&max_ack_delay_us)) {
set_detailed_error("Unable to read max_ack_delay_us.");
@@ -3409,19 +3405,13 @@
set_detailed_error("Invalid max_ack_delay_us.");
return false;
}
- frame->max_ack_delay = QuicTime::Delta::FromMicroseconds(max_ack_delay_us);
+ frame->requested_max_ack_delay =
+ QuicTime::Delta::FromMicroseconds(max_ack_delay_us);
- uint8_t ignore_order;
- if (!reader->ReadUInt8(&ignore_order)) {
- set_detailed_error("Unable to read ignore_order.");
+ if (!reader->ReadVarInt62(&frame->reordering_threshold)) {
+ set_detailed_error("Unable to read reordering_threshold.");
return false;
}
- if (ignore_order > 1) {
- set_detailed_error("Invalid ignore_order.");
- return false;
- }
- frame->ignore_order = ignore_order;
-
return true;
}
@@ -5270,17 +5260,17 @@
set_detailed_error("Writing sequence number failed.");
return false;
}
- if (!writer->WriteVarInt62(frame.packet_tolerance)) {
+ if (!writer->WriteVarInt62(frame.ack_eliciting_threshold)) {
set_detailed_error("Writing packet tolerance failed.");
return false;
}
- if (!writer->WriteVarInt62(
- static_cast<uint64_t>(frame.max_ack_delay.ToMicroseconds()))) {
+ if (!writer->WriteVarInt62(static_cast<uint64_t>(
+ frame.requested_max_ack_delay.ToMicroseconds()))) {
set_detailed_error("Writing max_ack_delay_us failed.");
return false;
}
- if (!writer->WriteUInt8(static_cast<uint8_t>(frame.ignore_order))) {
- set_detailed_error("Writing ignore_order failed.");
+ if (!writer->WriteVarInt62(frame.reordering_threshold)) {
+ set_detailed_error("Writing reordering_threshold failed.");
return false;
}
diff --git a/quiche/quic/core/quic_framer_test.cc b/quiche/quic/core/quic_framer_test.cc
index cced010..c8a79b3 100644
--- a/quiche/quic/core/quic_framer_test.cc
+++ b/quiche/quic/core/quic_framer_test.cc
@@ -4847,12 +4847,12 @@
0x40, 0xAF,
// sequence_number
0x11,
- // packet_tolerance
+ // ack_eliciting_threshold
0x02,
// max_ack_delay_us = 2'5000 us
0x80, 0x00, 0x61, 0xA8,
- // ignore_order
- 0x01
+ // reordering_threshold = 0 (ignore order)
+ 0x00
};
// clang-format on
@@ -4872,9 +4872,9 @@
ASSERT_EQ(1u, visitor_.ack_frequency_frames_.size());
const auto& frame = visitor_.ack_frequency_frames_.front();
EXPECT_EQ(17u, frame->sequence_number);
- EXPECT_EQ(2u, frame->packet_tolerance);
- EXPECT_EQ(2'5000u, frame->max_ack_delay.ToMicroseconds());
- EXPECT_EQ(true, frame->ignore_order);
+ EXPECT_EQ(2u, frame->ack_eliciting_threshold);
+ EXPECT_EQ(2'5000u, frame->requested_max_ack_delay.ToMicroseconds());
+ EXPECT_EQ(0u, frame->reordering_threshold);
}
TEST_P(QuicFramerTest, ParseImmediateAckFrame) {
@@ -8253,9 +8253,10 @@
QuicAckFrequencyFrame ack_frequency_frame;
ack_frequency_frame.sequence_number = 3;
- ack_frequency_frame.packet_tolerance = 5;
- ack_frequency_frame.max_ack_delay = QuicTime::Delta::FromMicroseconds(0x3fff);
- ack_frequency_frame.ignore_order = false;
+ ack_frequency_frame.ack_eliciting_threshold = 5;
+ ack_frequency_frame.requested_max_ack_delay =
+ QuicTime::Delta::FromMicroseconds(0x3fff);
+ ack_frequency_frame.reordering_threshold = 3;
QuicFrames frames = {QuicFrame(&ack_frequency_frame)};
// clang-format off
@@ -8271,12 +8272,12 @@
0x40, 0xaf,
// sequence number
0x03,
- // packet tolerance
+ // ack-eliciting threshold
0x05,
// max_ack_delay_us
0x7f, 0xff,
- // ignore_oder
- 0x00
+ // reordering threshold
+ 0x03
};
// clang-format on
if (!VersionHasIetfQuicFrames(framer_.transport_version())) {
diff --git a/quiche/quic/core/quic_received_packet_manager.cc b/quiche/quic/core/quic_received_packet_manager.cc
index 72a1425..5e18cab 100644
--- a/quiche/quic/core/quic_received_packet_manager.cc
+++ b/quiche/quic/core/quic_received_packet_manager.cc
@@ -12,6 +12,7 @@
#include "quiche/quic/core/crypto/crypto_protocol.h"
#include "quiche/quic/core/quic_config.h"
#include "quiche/quic/core/quic_connection_stats.h"
+#include "quiche/quic/core/quic_time.h"
#include "quiche/quic/core/quic_types.h"
#include "quiche/quic/platform/api/quic_bug_tracker.h"
#include "quiche/quic/platform/api/quic_flag_utils.h"
@@ -45,17 +46,14 @@
stats_(stats),
num_retransmittable_packets_received_since_last_ack_sent_(0),
min_received_before_ack_decimation_(kMinReceivedBeforeAckDecimation),
- ack_frequency_(kDefaultRetransmittablePacketsBeforeAck),
ack_decimation_delay_(GetQuicFlag(quic_ack_decimation_delay)),
unlimited_ack_decimation_(false),
one_immediate_ack_(false),
- ignore_order_(false),
local_max_ack_delay_(
QuicTime::Delta::FromMilliseconds(GetDefaultDelayedAckTimeMs())),
ack_timeout_(QuicTime::Zero()),
time_of_previous_received_packet_(QuicTime::Zero()),
- was_last_packet_missing_(false),
- last_ack_frequency_frame_sequence_number_(-1) {}
+ was_last_packet_missing_(false) {}
QuicReceivedPacketManager::~QuicReceivedPacketManager() {}
@@ -294,11 +292,12 @@
return;
}
- if (!ignore_order_ && was_last_packet_missing_ &&
- last_sent_largest_acked_.IsInitialized() &&
+ // TODO(martinduke): If a missing packet is received, do we send it when
+ // reordering_threshold_ == 0?
+ if (was_last_packet_missing_ && last_sent_largest_acked_.IsInitialized() &&
last_received_packet_number < last_sent_largest_acked_) {
- // Only ack immediately if an ACK frame was sent with a larger largest acked
- // than the newly received packet number.
+ // Ack immediately if an ACK frame was sent with a larger largest acked than
+ // the newly received packet number.
ack_timeout_ = now;
return;
}
@@ -322,7 +321,7 @@
return;
}
- if (!ignore_order_ && HasNewMissingPackets()) {
+ if (reordering_threshold_ == 1 && HasNewMissingPackets()) { // Fast path.
ack_timeout_ = now;
return;
}
@@ -384,15 +383,14 @@
void QuicReceivedPacketManager::OnAckFrequencyFrame(
const QuicAckFrequencyFrame& frame) {
- int64_t new_sequence_number = frame.sequence_number;
- if (new_sequence_number <= last_ack_frequency_frame_sequence_number_) {
+ if (frame.sequence_number < next_ack_frequency_frame_sequence_number_) {
// Ignore old ACK_FREQUENCY frames.
return;
}
- last_ack_frequency_frame_sequence_number_ = new_sequence_number;
- ack_frequency_ = frame.packet_tolerance;
- local_max_ack_delay_ = frame.max_ack_delay;
- ignore_order_ = frame.ignore_order;
+ next_ack_frequency_frame_sequence_number_ = frame.sequence_number + 1;
+ ack_frequency_ = frame.ack_eliciting_threshold + 1;
+ local_max_ack_delay_ = frame.requested_max_ack_delay;
+ reordering_threshold_ = frame.reordering_threshold;
}
} // namespace quic
diff --git a/quiche/quic/core/quic_received_packet_manager.h b/quiche/quic/core/quic_received_packet_manager.h
index d1feccc..404d46d 100644
--- a/quiche/quic/core/quic_received_packet_manager.h
+++ b/quiche/quic/core/quic_received_packet_manager.h
@@ -6,13 +6,15 @@
#define QUICHE_QUIC_CORE_QUIC_RECEIVED_PACKET_MANAGER_H_
#include <cstddef>
+#include <cstdint>
#include "quiche/quic/core/frames/quic_ack_frequency_frame.h"
#include "quiche/quic/core/quic_config.h"
-#include "quiche/quic/core/quic_framer.h"
+#include "quiche/quic/core/quic_constants.h"
#include "quiche/quic/core/quic_packets.h"
+#include "quiche/quic/core/quic_time.h"
#include "quiche/quic/core/quic_types.h"
-#include "quiche/quic/platform/api/quic_export.h"
+#include "quiche/common/platform/api/quiche_export.h"
namespace quic {
@@ -146,7 +148,7 @@
const RttStats& rtt_stats) const;
bool AckFrequencyFrameReceived() const {
- return last_ack_frequency_frame_sequence_number_ >= 0;
+ return next_ack_frequency_frame_sequence_number_ > 0;
}
void MaybeTrimAckRanges();
@@ -186,8 +188,8 @@
QuicPacketCount num_retransmittable_packets_received_since_last_ack_sent_;
// Ack decimation will start happening after this many packets are received.
size_t min_received_before_ack_decimation_;
- // Ack every n-th packet.
- size_t ack_frequency_;
+ // Ack every nth packet.
+ size_t ack_frequency_ = kDefaultRetransmittablePacketsBeforeAck;
// The max delay in fraction of min_rtt to use when sending decimated acks.
float ack_decimation_delay_;
// When true, removes ack decimation's max number of packets(10) before
@@ -195,12 +197,16 @@
bool unlimited_ack_decimation_;
// When true, only send 1 immediate ACK when reordering is detected.
bool one_immediate_ack_;
- // When true, do not ack immediately upon observation of packet reordering.
- bool ignore_order_;
+ // If the largest unacked packet has a packet number that is
+ // reordering_threshold_ more than the lowest missing packet number that is
+ // greater than largest_acked, then an immediate ACK will be sent. The default
+ // value is 1. A value of 0 means to ignore reordering.
+ uint64_t reordering_threshold_ = 1;
// The local node's maximum ack delay time. This is the maximum amount of
// time to wait before sending an acknowledgement.
QuicTime::Delta local_max_ack_delay_;
+
// Time that an ACK needs to be sent. 0 means no ACK is pending. Used when
// decide_when_to_send_acks_ is true.
QuicTime ack_timeout_;
@@ -222,9 +228,8 @@
// Last sent largest acked, which gets updated when ACK was successfully sent.
QuicPacketNumber last_sent_largest_acked_;
- // The sequence number of the last received AckFrequencyFrame. Negative if
- // none received.
- int64_t last_ack_frequency_frame_sequence_number_;
+ // The expected sequence number of the next received AckFrequencyFrame.
+ uint64_t next_ack_frequency_frame_sequence_number_ = 0;
};
} // namespace quic
diff --git a/quiche/quic/core/quic_received_packet_manager_test.cc b/quiche/quic/core/quic_received_packet_manager_test.cc
index 1f4aaee..97f00c5 100644
--- a/quiche/quic/core/quic_received_packet_manager_test.cc
+++ b/quiche/quic/core/quic_received_packet_manager_test.cc
@@ -543,17 +543,17 @@
EXPECT_FALSE(HasPendingAck());
QuicAckFrequencyFrame frame;
- frame.max_ack_delay = QuicTime::Delta::FromMilliseconds(10);
- frame.packet_tolerance = 5;
+ frame.requested_max_ack_delay = QuicTime::Delta::FromMilliseconds(10);
+ frame.ack_eliciting_threshold = 4;
received_manager_.OnAckFrequencyFrame(frame);
for (int i = 1; i <= 50; ++i) {
RecordPacketReceipt(i, clock_.ApproximateNow());
MaybeUpdateAckTimeout(kInstigateAck, i);
- if (i % frame.packet_tolerance == 0) {
+ if (i % (frame.ack_eliciting_threshold + 1) == 0) {
CheckAckTimeout(clock_.ApproximateNow());
} else {
- CheckAckTimeout(clock_.ApproximateNow() + frame.max_ack_delay);
+ CheckAckTimeout(clock_.ApproximateNow() + frame.requested_max_ack_delay);
}
}
}
@@ -563,9 +563,9 @@
EXPECT_FALSE(HasPendingAck());
QuicAckFrequencyFrame frame;
- frame.max_ack_delay = kDelayedAckTime;
- frame.packet_tolerance = 2;
- frame.ignore_order = true;
+ frame.requested_max_ack_delay = kDelayedAckTime;
+ frame.ack_eliciting_threshold = 1;
+ frame.reordering_threshold = 0;
received_manager_.OnAckFrequencyFrame(frame);
RecordPacketReceipt(4, clock_.ApproximateNow());
@@ -578,27 +578,27 @@
RecordPacketReceipt(3, clock_.ApproximateNow());
MaybeUpdateAckTimeout(kInstigateAck, 3);
- // Don't ack as ignore_order is set by AckFrequencyFrame.
- CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime);
-
- RecordPacketReceipt(2, clock_.ApproximateNow());
- MaybeUpdateAckTimeout(kInstigateAck, 2);
- // Immediate ack is sent as this is the 2nd packet of every two packets.
+ // Ack, since missing packets are always acknowledged.
CheckAckTimeout(clock_.ApproximateNow());
- RecordPacketReceipt(1, clock_.ApproximateNow());
- MaybeUpdateAckTimeout(kInstigateAck, 1);
- // Don't ack as ignore_order is set by AckFrequencyFrame.
+ RecordPacketReceipt(7, clock_.ApproximateNow());
+ MaybeUpdateAckTimeout(kInstigateAck, 10);
+ // No ack, as the frame says to ignore ordering.
CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime);
+
+ RecordPacketReceipt(1, clock_.ApproximateNow());
+ MaybeUpdateAckTimeout(kInstigateAck, 11);
+ // It's the second packet in a row, ack it.
+ CheckAckTimeout(clock_.ApproximateNow());
}
TEST_F(QuicReceivedPacketManagerTest,
- DisableMissingPaketsAckByIgnoreOrderFromAckFrequencyFrame) {
+ DisableMissingPacketsAckByIgnoreOrderFromAckFrequencyFrame) {
EXPECT_FALSE(HasPendingAck());
QuicAckFrequencyFrame frame;
- frame.max_ack_delay = kDelayedAckTime;
- frame.packet_tolerance = 2;
- frame.ignore_order = true;
+ frame.requested_max_ack_delay = kDelayedAckTime;
+ frame.ack_eliciting_threshold = 1;
+ frame.reordering_threshold = 0;
received_manager_.OnAckFrequencyFrame(frame);
RecordPacketReceipt(1, clock_.ApproximateNow());
@@ -622,8 +622,7 @@
RecordPacketReceipt(7, clock_.ApproximateNow());
MaybeUpdateAckTimeout(kInstigateAck, 7);
- // Don't ack even if packet 6 is newly missing as ignore_order is set by
- // AckFrequencyFrame.
+ // Don't ack as AckFrequencyFrame says to ignore ordering.
CheckAckTimeout(clock_.ApproximateNow() + kDelayedAckTime);
}
@@ -632,9 +631,9 @@
EXPECT_FALSE(HasPendingAck());
QuicAckFrequencyFrame frame;
- frame.max_ack_delay = kDelayedAckTime;
- frame.packet_tolerance = 3;
- frame.ignore_order = true;
+ frame.requested_max_ack_delay = kDelayedAckTime;
+ frame.ack_eliciting_threshold = 2;
+ frame.reordering_threshold = 0;
received_manager_.OnAckFrequencyFrame(frame);
// Process all the packets in order so there aren't missing packets.
diff --git a/quiche/quic/core/quic_sent_packet_manager.cc b/quiche/quic/core/quic_sent_packet_manager.cc
index 2e10bbc..ac1cc36 100644
--- a/quiche/quic/core/quic_sent_packet_manager.cc
+++ b/quiche/quic/core/quic_sent_packet_manager.cc
@@ -637,6 +637,7 @@
return !peer_min_ack_delay_.IsInfinite() && handshake_finished_;
}
+// TODO(martinduke): Update to include reordering threshold.
QuicAckFrequencyFrame QuicSentPacketManager::GetUpdatedAckFrequencyFrame()
const {
QuicAckFrequencyFrame frame;
@@ -647,15 +648,16 @@
}
QUIC_RELOADABLE_FLAG_COUNT_N(quic_can_send_ack_frequency, 1, 3);
- frame.packet_tolerance = kMaxRetransmittablePacketsBeforeAck;
+ frame.ack_eliciting_threshold = kMaxRetransmittablePacketsBeforeAck;
auto rtt = use_smoothed_rtt_in_ack_delay_ ? rtt_stats_.SmoothedOrInitialRtt()
: rtt_stats_.MinOrInitialRtt();
- frame.max_ack_delay = rtt * kPeerAckDecimationDelay;
- frame.max_ack_delay = std::max(frame.max_ack_delay, peer_min_ack_delay_);
+ frame.requested_max_ack_delay = rtt * kPeerAckDecimationDelay;
+ frame.requested_max_ack_delay =
+ std::max(frame.requested_max_ack_delay, peer_min_ack_delay_);
// TODO(haoyuewang) Remove this once kDefaultMinAckDelayTimeMs is updated to
// 5 ms on the client side.
- frame.max_ack_delay =
- std::max(frame.max_ack_delay,
+ frame.requested_max_ack_delay =
+ std::max(frame.requested_max_ack_delay,
QuicTime::Delta::FromMilliseconds(kDefaultMinAckDelayTimeMs));
return frame;
}
@@ -1611,10 +1613,11 @@
void QuicSentPacketManager::OnAckFrequencyFrameSent(
const QuicAckFrequencyFrame& ack_frequency_frame) {
- in_use_sent_ack_delays_.emplace_back(ack_frequency_frame.max_ack_delay,
- ack_frequency_frame.sequence_number);
- if (ack_frequency_frame.max_ack_delay > peer_max_ack_delay_) {
- peer_max_ack_delay_ = ack_frequency_frame.max_ack_delay;
+ in_use_sent_ack_delays_.emplace_back(
+ ack_frequency_frame.requested_max_ack_delay,
+ ack_frequency_frame.sequence_number);
+ if (ack_frequency_frame.requested_max_ack_delay > peer_max_ack_delay_) {
+ peer_max_ack_delay_ = ack_frequency_frame.requested_max_ack_delay;
}
}
diff --git a/quiche/quic/core/quic_sent_packet_manager_test.cc b/quiche/quic/core/quic_sent_packet_manager_test.cc
index 5e92c19..f325d58 100644
--- a/quiche/quic/core/quic_sent_packet_manager_test.cc
+++ b/quiche/quic/core/quic_sent_packet_manager_test.cc
@@ -2845,7 +2845,7 @@
int packet_number, int ack_frequency_sequence_number,
QuicTime::Delta max_ack_delay) {
auto* ack_frequency_frame = new QuicAckFrequencyFrame();
- ack_frequency_frame->max_ack_delay = max_ack_delay;
+ ack_frequency_frame->requested_max_ack_delay = max_ack_delay;
ack_frequency_frame->sequence_number = ack_frequency_sequence_number;
SerializedPacket packet(QuicPacketNumber(packet_number),
PACKET_4BYTE_PACKET_NUMBER, nullptr, kDefaultLength,
@@ -3074,10 +3074,10 @@
/*now=*/QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(24));
auto frame = manager_.GetUpdatedAckFrequencyFrame();
- EXPECT_EQ(frame.max_ack_delay,
+ EXPECT_EQ(frame.requested_max_ack_delay,
std::max(rtt_stats->min_rtt() * 0.25,
QuicTime::Delta::FromMilliseconds(1u)));
- EXPECT_EQ(frame.packet_tolerance, 10u);
+ EXPECT_EQ(frame.ack_eliciting_threshold, 10u);
}
TEST_F(QuicSentPacketManagerTest, SmoothedRttIgnoreAckDelay) {
@@ -3216,7 +3216,7 @@
/*now=*/QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(24));
auto frame = manager_.GetUpdatedAckFrequencyFrame();
- EXPECT_EQ(frame.max_ack_delay,
+ EXPECT_EQ(frame.requested_max_ack_delay,
std::max(rtt_stats->SmoothedOrInitialRtt() * 0.25,
QuicTime::Delta::FromMilliseconds(1u)));
}
diff --git a/quiche/quic/test_tools/simple_session_notifier.cc b/quiche/quic/test_tools/simple_session_notifier.cc
index d72b431..6e06b8c 100644
--- a/quiche/quic/test_tools/simple_session_notifier.cc
+++ b/quiche/quic/test_tools/simple_session_notifier.cc
@@ -4,13 +4,13 @@
#include "quiche/quic/test_tools/simple_session_notifier.h"
+#include "quiche/quic/core/frames/quic_ack_frequency_frame.h"
#include "quiche/quic/core/frames/quic_frame.h"
#include "quiche/quic/core/frames/quic_reset_stream_at_frame.h"
#include "quiche/quic/core/quic_error_codes.h"
#include "quiche/quic/core/quic_types.h"
#include "quiche/quic/core/quic_utils.h"
#include "quiche/quic/platform/api/quic_logging.h"
-#include "quiche/quic/test_tools/quic_test_utils.h"
namespace quic {
@@ -170,11 +170,12 @@
const bool had_buffered_data =
HasBufferedStreamData() || HasBufferedControlFrames();
QuicControlFrameId control_frame_id = ++last_control_frame_id_;
- control_frames_.emplace_back((
- QuicFrame(new QuicAckFrequencyFrame(control_frame_id,
- /*sequence_number=*/control_frame_id,
- ack_frequency_frame.packet_tolerance,
- ack_frequency_frame.max_ack_delay))));
+ control_frames_.emplace_back((QuicFrame(
+ new QuicAckFrequencyFrame(control_frame_id,
+ /*sequence_number=*/control_frame_id,
+ ack_frequency_frame.ack_eliciting_threshold,
+ ack_frequency_frame.requested_max_ack_delay,
+ ack_frequency_frame.reordering_threshold))));
if (had_buffered_data) {
QUIC_DLOG(WARNING) << "Connection is write blocked";
return;
diff --git a/quiche/quic/test_tools/simple_session_notifier.h b/quiche/quic/test_tools/simple_session_notifier.h
index dc3bcb2..eec1b6a 100644
--- a/quiche/quic/test_tools/simple_session_notifier.h
+++ b/quiche/quic/test_tools/simple_session_notifier.h
@@ -6,11 +6,11 @@
#define QUICHE_QUIC_TEST_TOOLS_SIMPLE_SESSION_NOTIFIER_H_
#include "absl/container/flat_hash_map.h"
+#include "quiche/quic/core/quic_connection.h"
#include "quiche/quic/core/quic_error_codes.h"
#include "quiche/quic/core/quic_interval_set.h"
#include "quiche/quic/core/quic_types.h"
#include "quiche/quic/core/session_notifier_interface.h"
-#include "quiche/quic/platform/api/quic_test.h"
#include "quiche/common/quiche_circular_deque.h"
#include "quiche/common/quiche_linked_hash_map.h"
