gr1ffon/quic-go
1
0
Fork 0
forked from quic-go/quic-go
Code Pull Requests Activity

ackhandler: avoid calling time.Now() when generating ACK frame (#4886)

Browse Source
This commit is contained in:
Marten Seemann
2025-01-18 01:16:30 -08:00
committed by GitHub
parent 29f98a296c
commit 92dc1970ec
9 changed files with 125 additions and 93 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
internal/ackhandler/interfaces.go
View File

@@ -49,5 +49,5 @@ type ReceivedPacketHandler interface {
DropPackets(protocol.EncryptionLevel)
GetAlarmTimeout() time.Time
GetAckFrame(encLevel protocol.EncryptionLevel, onlyIfQueued bool) *wire.AckFrame
GetAckFrame(_ protocol.EncryptionLevel, now time.Time, onlyIfQueued bool) *wire.AckFrame
}

4
internal/ackhandler/received_packet_handler.go
View File

@@ -87,7 +87,7 @@ func (h *receivedPacketHandler) GetAlarmTimeout() time.Time {
return h.appDataPackets.GetAlarmTimeout()
}
func (h *receivedPacketHandler) GetAckFrame(encLevel protocol.EncryptionLevel, onlyIfQueued bool) *wire.AckFrame {
func (h *receivedPacketHandler) GetAckFrame(encLevel protocol.EncryptionLevel, now time.Time, onlyIfQueued bool) *wire.AckFrame {
//nolint:exhaustive // 0-RTT packets can't contain ACK frames.
switch encLevel {
case protocol.EncryptionInitial:
@@ -101,7 +101,7 @@ func (h *receivedPacketHandler) GetAckFrame(encLevel protocol.EncryptionLevel, o
}
return nil
case protocol.Encryption1RTT:
return h.appDataPackets.GetAckFrame(onlyIfQueued)
return h.appDataPackets.GetAckFrame(now, onlyIfQueued)
default:
// 0-RTT packets can't contain ACK frames
return nil

27
internal/ackhandler/received_packet_handler_test.go
View File

@@ -17,7 +17,8 @@ func TestGenerateACKsForPacketNumberSpaces(t *testing.T) {
sentPackets := NewMockSentPacketTracker(ctrl)
handler := newReceivedPacketHandler(sentPackets, utils.DefaultLogger)
sendTime := time.Now().Add(-time.Second)
now := time.Now()
sendTime := now.Add(-time.Second)
sentPackets.EXPECT().GetLowestPacketNotConfirmedAcked().AnyTimes()
sentPackets.EXPECT().ReceivedPacket(protocol.EncryptionInitial, sendTime).Times(2)
sentPackets.EXPECT().ReceivedPacket(protocol.EncryptionHandshake, sendTime).Times(2)
@@ -31,7 +32,7 @@ func TestGenerateACKsForPacketNumberSpaces(t *testing.T) {
require.NoError(t, handler.ReceivedPacket(4, protocol.ECNCE, protocol.Encryption1RTT, sendTime, true))
// Initial
initialAck := handler.GetAckFrame(protocol.EncryptionInitial, true)
initialAck := handler.GetAckFrame(protocol.EncryptionInitial, now, true)
require.NotNil(t, initialAck)
require.Equal(t, []wire.AckRange{{Smallest: 2, Largest: 3}}, initialAck.AckRanges)
require.Zero(t, initialAck.DelayTime)
@@ -40,7 +41,7 @@ func TestGenerateACKsForPacketNumberSpaces(t *testing.T) {
require.Zero(t, initialAck.ECNCE)
// Handshake
handshakeAck := handler.GetAckFrame(protocol.EncryptionHandshake, true)
handshakeAck := handler.GetAckFrame(protocol.EncryptionHandshake, now, true)
require.NotNil(t, handshakeAck)
require.Equal(t, []wire.AckRange{{Smallest: 1, Largest: 2}}, handshakeAck.AckRanges)
require.Zero(t, handshakeAck.DelayTime)
@@ -49,10 +50,10 @@ func TestGenerateACKsForPacketNumberSpaces(t *testing.T) {
require.Zero(t, handshakeAck.ECNCE)
// 1-RTT
oneRTTAck := handler.GetAckFrame(protocol.Encryption1RTT, true)
oneRTTAck := handler.GetAckFrame(protocol.Encryption1RTT, now, true)
require.NotNil(t, oneRTTAck)
require.Equal(t, []wire.AckRange{{Smallest: 4, Largest: 5}}, oneRTTAck.AckRanges)
require.InDelta(t, time.Second, oneRTTAck.DelayTime, float64(50*time.Millisecond))
require.Equal(t, time.Second, oneRTTAck.DelayTime)
require.Zero(t, oneRTTAck.ECT0)
require.Zero(t, oneRTTAck.ECT1)
require.EqualValues(t, 2, oneRTTAck.ECNCE)
@@ -71,7 +72,7 @@ func TestReceive0RTTAnd1RTT(t *testing.T) {
require.NoError(t, handler.ReceivedPacket(2, protocol.ECNNon, protocol.Encryption0RTT, sendTime, true))
require.NoError(t, handler.ReceivedPacket(3, protocol.ECNNon, protocol.Encryption1RTT, sendTime, true))
ack := handler.GetAckFrame(protocol.Encryption1RTT, true)
ack := handler.GetAckFrame(protocol.Encryption1RTT, time.Now(), true)
require.NotNil(t, ack)
require.Equal(t, []wire.AckRange{{Smallest: 2, Largest: 3}}, ack.AckRanges)
@@ -95,17 +96,17 @@ func TestDropPackets(t *testing.T) {
require.NoError(t, handler.ReceivedPacket(2, protocol.ECNNon, protocol.Encryption1RTT, sendTime, true))
// Initial
require.NotNil(t, handler.GetAckFrame(protocol.EncryptionInitial, true))
require.NotNil(t, handler.GetAckFrame(protocol.EncryptionInitial, time.Now(), true))
handler.DropPackets(protocol.EncryptionInitial)
require.Nil(t, handler.GetAckFrame(protocol.EncryptionInitial, true))
require.Nil(t, handler.GetAckFrame(protocol.EncryptionInitial, time.Now(), true))
// Handshake
require.NotNil(t, handler.GetAckFrame(protocol.EncryptionHandshake, true))
require.NotNil(t, handler.GetAckFrame(protocol.EncryptionHandshake, time.Now(), true))
handler.DropPackets(protocol.EncryptionHandshake)
require.Nil(t, handler.GetAckFrame(protocol.EncryptionHandshake, true))
require.Nil(t, handler.GetAckFrame(protocol.EncryptionHandshake, time.Now(), true))
// 1-RTT
require.NotNil(t, handler.GetAckFrame(protocol.Encryption1RTT, true))
require.NotNil(t, handler.GetAckFrame(protocol.Encryption1RTT, time.Now(), true))
// 0-RTT is a no-op
handler.DropPackets(protocol.Encryption0RTT)
@@ -122,7 +123,7 @@ func TestAckRangePruning(t *testing.T) {
require.NoError(t, handler.ReceivedPacket(1, protocol.ECNNon, protocol.Encryption1RTT, sendTime, true))
require.NoError(t, handler.ReceivedPacket(2, protocol.ECNNon, protocol.Encryption1RTT, sendTime, true))
ack := handler.GetAckFrame(protocol.Encryption1RTT, true)
ack := handler.GetAckFrame(protocol.Encryption1RTT, time.Now(), true)
require.NotNil(t, ack)
require.Equal(t, []wire.AckRange{{Smallest: 1, Largest: 2}}, ack.AckRanges)
@@ -130,7 +131,7 @@ func TestAckRangePruning(t *testing.T) {
sentPackets.EXPECT().GetLowestPacketNotConfirmedAcked().Return(protocol.PacketNumber(2))
require.NoError(t, handler.ReceivedPacket(4, protocol.ECNNon, protocol.Encryption1RTT, sendTime, true))
ack = handler.GetAckFrame(protocol.Encryption1RTT, true)
ack = handler.GetAckFrame(protocol.Encryption1RTT, time.Now(), true)
require.NotNil(t, ack)
require.Equal(t, []wire.AckRange{{Smallest: 2, Largest: 4}}, ack.AckRanges)
}

3
internal/ackhandler/received_packet_tracker.go
View File

@@ -196,8 +196,7 @@ func (h *appDataReceivedPacketTracker) shouldQueueACK(pn protocol.PacketNumber,
return false
}
func (h *appDataReceivedPacketTracker) GetAckFrame(onlyIfQueued bool) *wire.AckFrame {
now := time.Now()
func (h *appDataReceivedPacketTracker) GetAckFrame(now time.Time, onlyIfQueued bool) *wire.AckFrame {
if onlyIfQueued && !h.ackQueued {
if h.ackAlarm.IsZero() || h.ackAlarm.After(now) {
return nil

36
internal/ackhandler/received_packet_tracker_test.go
View File

@@ -63,7 +63,7 @@ func TestAppDataReceivedPacketTrackerECN(t *testing.T) {
require.NoError(t, tr.ReceivedPacket(pn, protocol.ECNCE, time.Now(), true))
pn++
}
ack := tr.GetAckFrame(false)
ack := tr.GetAckFrame(time.Now(), false)
require.Equal(t, uint64(1), ack.ECT0)
require.Equal(t, uint64(2), ack.ECT1)
require.Equal(t, uint64(3), ack.ECNCE)
@@ -73,14 +73,14 @@ func TestAppDataReceivedPacketTrackerAckEverySecondPacket(t *testing.T) {
tr := newAppDataReceivedPacketTracker(utils.DefaultLogger)
// the first packet is always acknowledged
require.NoError(t, tr.ReceivedPacket(0, protocol.ECNNon, time.Now(), true))
require.NotNil(t, tr.GetAckFrame(true))
require.NotNil(t, tr.GetAckFrame(time.Now(), true))
for p := protocol.PacketNumber(1); p <= 20; p++ {
require.NoError(t, tr.ReceivedPacket(p, protocol.ECNNon, time.Now(), true))
switch p % 2 {
case 0:
require.NotNil(t, tr.GetAckFrame(true))
require.NotNil(t, tr.GetAckFrame(time.Now(), true))
case 1:
require.Nil(t, tr.GetAckFrame(true))
require.Nil(t, tr.GetAckFrame(time.Now(), true))
}
}
}
@@ -90,20 +90,20 @@ func TestAppDataReceivedPacketTrackerAlarmTimeout(t *testing.T) {
// the first packet is always acknowledged
require.NoError(t, tr.ReceivedPacket(0, protocol.ECNNon, time.Now(), true))
require.NotNil(t, tr.GetAckFrame(true))
require.NotNil(t, tr.GetAckFrame(time.Now(), true))
now := time.Now()
require.NoError(t, tr.ReceivedPacket(1, protocol.ECNNon, now, false))
require.Nil(t, tr.GetAckFrame(true))
require.Nil(t, tr.GetAckFrame(time.Now(), true))
require.Zero(t, tr.GetAlarmTimeout())
rcvTime := now.Add(10 * time.Millisecond)
require.NoError(t, tr.ReceivedPacket(2, protocol.ECNNon, rcvTime, true))
require.Equal(t, rcvTime.Add(protocol.MaxAckDelay), tr.GetAlarmTimeout())
require.Nil(t, tr.GetAckFrame(true))
require.Nil(t, tr.GetAckFrame(time.Now(), true))
// no timeout after the ACK has been dequeued
require.NotNil(t, tr.GetAckFrame(false))
require.NotNil(t, tr.GetAckFrame(time.Now(), false))
require.Zero(t, tr.GetAlarmTimeout())
}
@@ -112,10 +112,10 @@ func TestAppDataReceivedPacketTrackerQueuesECNCE(t *testing.T) {
// the first packet is always acknowledged
require.NoError(t, tr.ReceivedPacket(0, protocol.ECNNon, time.Now(), true))
require.NotNil(t, tr.GetAckFrame(true))
require.NotNil(t, tr.GetAckFrame(time.Now(), true))
require.NoError(t, tr.ReceivedPacket(1, protocol.ECNCE, time.Now(), true))
ack := tr.GetAckFrame(true)
ack := tr.GetAckFrame(time.Now(), true)
require.NotNil(t, ack)
require.Equal(t, protocol.PacketNumber(1), ack.LargestAcked())
require.EqualValues(t, 1, ack.ECNCE)
@@ -126,16 +126,16 @@ func TestAppDataReceivedPacketTrackerMissingPackets(t *testing.T) {
// the first packet is always acknowledged
require.NoError(t, tr.ReceivedPacket(0, protocol.ECNNon, time.Now(), true))
require.NotNil(t, tr.GetAckFrame(true))
require.NotNil(t, tr.GetAckFrame(time.Now(), true))
require.NoError(t, tr.ReceivedPacket(5, protocol.ECNNon, time.Now(), true))
ack := tr.GetAckFrame(true) // ACK: 0 and 5, missing: 1, 2, 3, 4
ack := tr.GetAckFrame(time.Now(), true) // ACK: 0 and 5, missing: 1, 2, 3, 4
require.NotNil(t, ack)
require.Equal(t, []wire.AckRange{{Smallest: 5, Largest: 5}, {Smallest: 0, Largest: 0}}, ack.AckRanges)
// now receive one of the missing packets
require.NoError(t, tr.ReceivedPacket(3, protocol.ECNNon, time.Now(), true))
require.NotNil(t, tr.GetAckFrame(true))
require.NotNil(t, tr.GetAckFrame(time.Now(), true))
}
func TestAppDataReceivedPacketTrackerDelayTime(t *testing.T) {
@@ -144,13 +144,13 @@ func TestAppDataReceivedPacketTrackerDelayTime(t *testing.T) {
now := time.Now()
require.NoError(t, tr.ReceivedPacket(1, protocol.ECNNon, now, true))
require.NoError(t, tr.ReceivedPacket(2, protocol.ECNNon, now.Add(-1337*time.Millisecond), true))
ack := tr.GetAckFrame(true)
ack := tr.GetAckFrame(now, true)
require.NotNil(t, ack)
require.InDelta(t, 1337*time.Millisecond, ack.DelayTime, float64(50*time.Millisecond))
require.Equal(t, 1337*time.Millisecond, ack.DelayTime)
// don't use a negative delay time
require.NoError(t, tr.ReceivedPacket(3, protocol.ECNNon, now.Add(time.Hour), true))
ack = tr.GetAckFrame(false)
ack = tr.GetAckFrame(now, false)
require.NotNil(t, ack)
require.Zero(t, ack.DelayTime)
}
@@ -166,7 +166,7 @@ func TestAppDataReceivedPacketTrackerIgnoreBelow(t *testing.T) {
for i := 5; i <= 10; i++ {
require.NoError(t, tr.ReceivedPacket(protocol.PacketNumber(i), protocol.ECNNon, time.Now(), true))
}
ack := tr.GetAckFrame(true)
ack := tr.GetAckFrame(time.Now(), true)
require.NotNil(t, ack)
require.Equal(t, []wire.AckRange{{Smallest: 5, Largest: 10}}, ack.AckRanges)
@@ -174,7 +174,7 @@ func TestAppDataReceivedPacketTrackerIgnoreBelow(t *testing.T) {
require.NoError(t, tr.ReceivedPacket(11, protocol.ECNNon, time.Now(), true))
require.NoError(t, tr.ReceivedPacket(12, protocol.ECNNon, time.Now(), true))
ack = tr.GetAckFrame(true)
ack = tr.GetAckFrame(time.Now(), true)
require.NotNil(t, ack)
require.Equal(t, []wire.AckRange{{Smallest: 7, Largest: 12}}, ack.AckRanges)