From 77b1021a54be62d20a622bf51baf84096b76105a Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 12 May 2020 18:25:41 +0700 Subject: [PATCH] make it possible to generate an ACK frame, even if no ACK is queued yet --- .../ackhandler/received_packet_handler.go | 6 +- .../ackhandler/received_packet_tracker.go | 28 +- .../received_packet_tracker_test.go | 300 ++++++++++-------- 3 files changed, 193 insertions(+), 141 deletions(-) diff --git a/internal/ackhandler/received_packet_handler.go b/internal/ackhandler/received_packet_handler.go index 814addb2..af67a0c4 100644 --- a/internal/ackhandler/received_packet_handler.go +++ b/internal/ackhandler/received_packet_handler.go @@ -118,15 +118,15 @@ func (h *receivedPacketHandler) GetAckFrame(encLevel protocol.EncryptionLevel) * switch encLevel { case protocol.EncryptionInitial: if h.initialPackets != nil { - ack = h.initialPackets.GetAckFrame() + ack = h.initialPackets.GetAckFrame(true) } case protocol.EncryptionHandshake: if h.handshakePackets != nil { - ack = h.handshakePackets.GetAckFrame() + ack = h.handshakePackets.GetAckFrame(true) } case protocol.Encryption1RTT: // 0-RTT packets can't contain ACK frames - return h.appDataPackets.GetAckFrame() + return h.appDataPackets.GetAckFrame(true) default: return nil } diff --git a/internal/ackhandler/received_packet_tracker.go b/internal/ackhandler/received_packet_tracker.go index 5cd973e3..b2c06084 100644 --- a/internal/ackhandler/received_packet_tracker.go +++ b/internal/ackhandler/received_packet_tracker.go @@ -19,9 +19,11 @@ type receivedPacketTracker struct { maxAckDelay time.Duration rttStats *congestion.RTTStats + hasNewAck bool // true as soon as we received an ack-eliciting new packet + ackQueued bool // true once we received more than 2 (or later in the connection 10) ack-eliciting packets + packetsReceivedSinceLastAck int ackElicitingPacketsReceivedSinceLastAck int - ackQueued bool ackAlarm time.Time lastAck *wire.AckFrame @@ -55,7 +57,9 @@ func (h *receivedPacketTracker) ReceivedPacket(packetNumber protocol.PacketNumbe h.largestObservedReceivedTime = rcvTime } - h.packetHistory.ReceivedPacket(packetNumber) + if isNew := h.packetHistory.ReceivedPacket(packetNumber); isNew && shouldInstigateAck { + h.hasNewAck = true + } h.maybeQueueAck(packetNumber, rcvTime, shouldInstigateAck, isMissing) } @@ -96,7 +100,9 @@ func (h *receivedPacketTracker) maybeQueueAck(packetNumber protocol.PacketNumber // always ack the first packet if h.lastAck == nil { - h.logger.Debugf("\tQueueing ACK because the first packet should be acknowledged.") + if !h.ackQueued { + h.logger.Debugf("\tQueueing ACK because the first packet should be acknowledged.") + } h.ackQueued = true return } @@ -163,13 +169,18 @@ func (h *receivedPacketTracker) maybeQueueAck(packetNumber protocol.PacketNumber } } -func (h *receivedPacketTracker) GetAckFrame() *wire.AckFrame { - now := time.Now() - if !h.ackQueued && (h.ackAlarm.IsZero() || h.ackAlarm.After(now)) { +func (h *receivedPacketTracker) GetAckFrame(onlyIfQueued bool) *wire.AckFrame { + if !h.hasNewAck { return nil } - if h.logger.Debug() && !h.ackQueued && !h.ackAlarm.IsZero() { - h.logger.Debugf("Sending ACK because the ACK timer expired.") + now := time.Now() + if onlyIfQueued { + if !h.ackQueued && (h.ackAlarm.IsZero() || h.ackAlarm.After(now)) { + return nil + } + if h.logger.Debug() && !h.ackQueued && !h.ackAlarm.IsZero() { + h.logger.Debugf("Sending ACK because the ACK timer expired.") + } } ack := &wire.AckFrame{ @@ -182,6 +193,7 @@ func (h *receivedPacketTracker) GetAckFrame() *wire.AckFrame { h.lastAck = ack h.ackAlarm = time.Time{} h.ackQueued = false + h.hasNewAck = false h.packetsReceivedSinceLastAck = 0 h.ackElicitingPacketsReceivedSinceLastAck = 0 return ack diff --git a/internal/ackhandler/received_packet_tracker_test.go b/internal/ackhandler/received_packet_tracker_test.go index 87262c4e..0d6f2227 100644 --- a/internal/ackhandler/received_packet_tracker_test.go +++ b/internal/ackhandler/received_packet_tracker_test.go @@ -55,7 +55,7 @@ var _ = Describe("Received Packet Tracker", func() { for i := 1; i <= 10; i++ { tracker.ReceivedPacket(protocol.PacketNumber(i), time.Time{}, true) } - Expect(tracker.GetAckFrame()).ToNot(BeNil()) + Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) Expect(tracker.ackQueued).To(BeFalse()) } @@ -63,22 +63,22 @@ var _ = Describe("Received Packet Tracker", func() { for i := 1; i <= minReceivedBeforeAckDecimation; i++ { tracker.ReceivedPacket(protocol.PacketNumber(i), time.Time{}, true) } - Expect(tracker.GetAckFrame()).ToNot(BeNil()) + Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) Expect(tracker.ackQueued).To(BeFalse()) } It("always queues an ACK for the first packet", func() { - tracker.ReceivedPacket(1, time.Now(), false) + tracker.ReceivedPacket(1, time.Now(), true) Expect(tracker.ackQueued).To(BeTrue()) Expect(tracker.GetAlarmTimeout()).To(BeZero()) - Expect(tracker.GetAckFrame().DelayTime).To(BeNumerically("~", 0, time.Second)) + Expect(tracker.GetAckFrame(true).DelayTime).To(BeNumerically("~", 0, time.Second)) }) It("works with packet number 0", func() { - tracker.ReceivedPacket(0, time.Now(), false) + tracker.ReceivedPacket(0, time.Now(), true) Expect(tracker.ackQueued).To(BeTrue()) Expect(tracker.GetAlarmTimeout()).To(BeZero()) - Expect(tracker.GetAckFrame().DelayTime).To(BeNumerically("~", 0, time.Second)) + Expect(tracker.GetAckFrame(true).DelayTime).To(BeNumerically("~", 0, time.Second)) }) It("queues an ACK for every second ack-eliciting packet at the beginning", func() { @@ -92,10 +92,21 @@ var _ = Describe("Received Packet Tracker", func() { Expect(tracker.ackQueued).To(BeTrue()) p++ // dequeue the ACK frame - Expect(tracker.GetAckFrame()).ToNot(BeNil()) + Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) } }) + It("resets the counter when a non-queued ACK frame is generated", func() { + receiveAndAck10Packets() + rcvTime := time.Now() + tracker.ReceivedPacket(11, rcvTime, true) + Expect(tracker.GetAckFrame(false)).ToNot(BeNil()) + tracker.ReceivedPacket(12, rcvTime, true) + Expect(tracker.GetAckFrame(true)).To(BeNil()) + tracker.ReceivedPacket(13, rcvTime, true) + Expect(tracker.GetAckFrame(false)).ToNot(BeNil()) + }) + It("queues an ACK for every 10 ack-eliciting packet, if they are arriving fast", func() { receiveAndAck10Packets() p := protocol.PacketNumber(10000) @@ -125,7 +136,7 @@ var _ = Describe("Received Packet Tracker", func() { receiveAndAck10Packets() tracker.ReceivedPacket(11, time.Time{}, true) tracker.ReceivedPacket(13, time.Time{}, true) - ack := tracker.GetAckFrame() // ACK: 1-11 and 13, missing: 12 + ack := tracker.GetAckFrame(true) // ACK: 1-11 and 13, missing: 12 Expect(ack).ToNot(BeNil()) Expect(ack.HasMissingRanges()).To(BeTrue()) Expect(tracker.ackQueued).To(BeFalse()) @@ -138,12 +149,12 @@ var _ = Describe("Received Packet Tracker", func() { // 11 is missing tracker.ReceivedPacket(12, time.Time{}, true) tracker.ReceivedPacket(13, time.Time{}, true) - ack := tracker.GetAckFrame() // ACK: 1-10, 12-13 + ack := tracker.GetAckFrame(true) // ACK: 1-10, 12-13 Expect(ack).ToNot(BeNil()) // now receive 11 tracker.IgnoreBelow(12) tracker.ReceivedPacket(11, time.Time{}, false) - ack = tracker.GetAckFrame() + ack = tracker.GetAckFrame(true) Expect(ack).To(BeNil()) }) @@ -169,151 +180,180 @@ var _ = Describe("Received Packet Tracker", func() { tracker.ReceivedPacket(p+10, now, true) // we now know that packets p+7, p+8 and p+9 Expect(rttStats.MinRTT()).To(Equal(rtt)) Expect(tracker.ackAlarm.Sub(now)).To(Equal(rtt / 8)) - ack := tracker.GetAckFrame() + ack := tracker.GetAckFrame(true) Expect(ack.HasMissingRanges()).To(BeTrue()) Expect(ack).ToNot(BeNil()) }) }) Context("ACK generation", func() { - BeforeEach(func() { - tracker.ackQueued = true - }) - - It("generates a simple ACK frame", func() { + It("generates an ACK for an ack-eliciting packet, if no ACK is queued yet", func() { tracker.ReceivedPacket(1, time.Time{}, true) + // The first packet is always acknowledged. + Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) + tracker.ReceivedPacket(2, time.Time{}, true) - ack := tracker.GetAckFrame() + Expect(tracker.GetAckFrame(true)).To(BeNil()) + ack := tracker.GetAckFrame(false) Expect(ack).ToNot(BeNil()) + Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(1))) Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(2))) - Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(1))) - Expect(ack.HasMissingRanges()).To(BeFalse()) }) - It("generates an ACK for packet number 0", func() { - tracker.ReceivedPacket(0, time.Time{}, true) - ack := tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(0))) - Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(0))) - Expect(ack.HasMissingRanges()).To(BeFalse()) - }) - - It("sets the delay time", func() { + It("doesn't generate ACK for a non-ack-eliciting packet, if no ACK is queued yet", func() { tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ReceivedPacket(2, time.Now().Add(-1337*time.Millisecond), true) - ack := tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(ack.DelayTime).To(BeNumerically("~", 1337*time.Millisecond, 50*time.Millisecond)) - }) + // The first packet is always acknowledged. + Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) - It("uses a 0 delay time if the delay would be negative", func() { - tracker.ReceivedPacket(0, time.Now().Add(time.Hour), true) - ack := tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(ack.DelayTime).To(BeZero()) - }) - - It("saves the last sent ACK", func() { - tracker.ReceivedPacket(1, time.Time{}, true) - ack := tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(tracker.lastAck).To(Equal(ack)) - tracker.ReceivedPacket(2, time.Time{}, true) - tracker.ackQueued = true - ack = tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(tracker.lastAck).To(Equal(ack)) - }) - - It("generates an ACK frame with missing packets", func() { - tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ReceivedPacket(4, time.Time{}, true) - ack := tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(4))) - Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(1))) - Expect(ack.AckRanges).To(Equal([]wire.AckRange{ - {Smallest: 4, Largest: 4}, - {Smallest: 1, Largest: 1}, - })) - }) - - It("generates an ACK for packet number 0 and other packets", func() { - tracker.ReceivedPacket(0, time.Time{}, true) - tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(2, time.Time{}, false) + Expect(tracker.GetAckFrame(false)).To(BeNil()) tracker.ReceivedPacket(3, time.Time{}, true) - ack := tracker.GetAckFrame() + ack := tracker.GetAckFrame(false) Expect(ack).ToNot(BeNil()) + Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(1))) Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(3))) - Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(0))) - Expect(ack.AckRanges).To(Equal([]wire.AckRange{ - {Smallest: 3, Largest: 3}, - {Smallest: 0, Largest: 1}, - })) }) - It("doesn't add delayed packets to the packetHistory", func() { - tracker.IgnoreBelow(7) - tracker.ReceivedPacket(4, time.Time{}, true) - tracker.ReceivedPacket(10, time.Time{}, true) - ack := tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(10))) - Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(10))) - }) + Context("for queued ACKs", func() { + BeforeEach(func() { + tracker.ackQueued = true + }) - It("deletes packets from the packetHistory when a lower limit is set", func() { - for i := 1; i <= 12; i++ { - tracker.ReceivedPacket(protocol.PacketNumber(i), time.Time{}, true) - } - tracker.IgnoreBelow(7) - // check that the packets were deleted from the receivedPacketHistory by checking the values in an ACK frame - ack := tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(12))) - Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(7))) - Expect(ack.HasMissingRanges()).To(BeFalse()) - }) + It("generates a simple ACK frame", func() { + tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(2, time.Time{}, true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(2))) + Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(1))) + Expect(ack.HasMissingRanges()).To(BeFalse()) + }) - // TODO: remove this test when dropping support for STOP_WAITINGs - It("handles a lower limit of 0", func() { - tracker.IgnoreBelow(0) - tracker.ReceivedPacket(1337, time.Time{}, true) - ack := tracker.GetAckFrame() - Expect(ack).ToNot(BeNil()) - Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(1337))) - }) + It("generates an ACK for packet number 0", func() { + tracker.ReceivedPacket(0, time.Time{}, true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(0))) + Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(0))) + Expect(ack.HasMissingRanges()).To(BeFalse()) + }) - It("resets all counters needed for the ACK queueing decision when sending an ACK", func() { - tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ackAlarm = time.Now().Add(-time.Minute) - Expect(tracker.GetAckFrame()).ToNot(BeNil()) - Expect(tracker.packetsReceivedSinceLastAck).To(BeZero()) - Expect(tracker.GetAlarmTimeout()).To(BeZero()) - Expect(tracker.ackElicitingPacketsReceivedSinceLastAck).To(BeZero()) - Expect(tracker.ackQueued).To(BeFalse()) - }) + It("sets the delay time", func() { + tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(2, time.Now().Add(-1337*time.Millisecond), true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.DelayTime).To(BeNumerically("~", 1337*time.Millisecond, 50*time.Millisecond)) + }) - It("doesn't generate an ACK when none is queued and the timer is not set", func() { - tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ackQueued = false - tracker.ackAlarm = time.Time{} - Expect(tracker.GetAckFrame()).To(BeNil()) - }) + It("uses a 0 delay time if the delay would be negative", func() { + tracker.ReceivedPacket(0, time.Now().Add(time.Hour), true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.DelayTime).To(BeZero()) + }) - It("doesn't generate an ACK when none is queued and the timer has not yet expired", func() { - tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ackQueued = false - tracker.ackAlarm = time.Now().Add(time.Minute) - Expect(tracker.GetAckFrame()).To(BeNil()) - }) + It("saves the last sent ACK", func() { + tracker.ReceivedPacket(1, time.Time{}, true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(tracker.lastAck).To(Equal(ack)) + tracker.ReceivedPacket(2, time.Time{}, true) + tracker.ackQueued = true + ack = tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(tracker.lastAck).To(Equal(ack)) + }) - It("generates an ACK when the timer has expired", func() { - tracker.ReceivedPacket(1, time.Time{}, true) - tracker.ackQueued = false - tracker.ackAlarm = time.Now().Add(-time.Minute) - Expect(tracker.GetAckFrame()).ToNot(BeNil()) + It("generates an ACK frame with missing packets", func() { + tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(4, time.Time{}, true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(4))) + Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(1))) + Expect(ack.AckRanges).To(Equal([]wire.AckRange{ + {Smallest: 4, Largest: 4}, + {Smallest: 1, Largest: 1}, + })) + }) + + It("generates an ACK for packet number 0 and other packets", func() { + tracker.ReceivedPacket(0, time.Time{}, true) + tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ReceivedPacket(3, time.Time{}, true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(3))) + Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(0))) + Expect(ack.AckRanges).To(Equal([]wire.AckRange{ + {Smallest: 3, Largest: 3}, + {Smallest: 0, Largest: 1}, + })) + }) + + It("doesn't add delayed packets to the packetHistory", func() { + tracker.IgnoreBelow(7) + tracker.ReceivedPacket(4, time.Time{}, true) + tracker.ReceivedPacket(10, time.Time{}, true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(10))) + Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(10))) + }) + + It("deletes packets from the packetHistory when a lower limit is set", func() { + for i := 1; i <= 12; i++ { + tracker.ReceivedPacket(protocol.PacketNumber(i), time.Time{}, true) + } + tracker.IgnoreBelow(7) + // check that the packets were deleted from the receivedPacketHistory by checking the values in an ACK frame + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(12))) + Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(7))) + Expect(ack.HasMissingRanges()).To(BeFalse()) + }) + + // TODO: remove this test when dropping support for STOP_WAITINGs + It("handles a lower limit of 0", func() { + tracker.IgnoreBelow(0) + tracker.ReceivedPacket(1337, time.Time{}, true) + ack := tracker.GetAckFrame(true) + Expect(ack).ToNot(BeNil()) + Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(1337))) + }) + + It("resets all counters needed for the ACK queueing decision when sending an ACK", func() { + tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ackAlarm = time.Now().Add(-time.Minute) + Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) + Expect(tracker.packetsReceivedSinceLastAck).To(BeZero()) + Expect(tracker.GetAlarmTimeout()).To(BeZero()) + Expect(tracker.ackElicitingPacketsReceivedSinceLastAck).To(BeZero()) + Expect(tracker.ackQueued).To(BeFalse()) + }) + + It("doesn't generate an ACK when none is queued and the timer is not set", func() { + tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ackQueued = false + tracker.ackAlarm = time.Time{} + Expect(tracker.GetAckFrame(true)).To(BeNil()) + }) + + It("doesn't generate an ACK when none is queued and the timer has not yet expired", func() { + tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ackQueued = false + tracker.ackAlarm = time.Now().Add(time.Minute) + Expect(tracker.GetAckFrame(true)).To(BeNil()) + }) + + It("generates an ACK when the timer has expired", func() { + tracker.ReceivedPacket(1, time.Time{}, true) + tracker.ackQueued = false + tracker.ackAlarm = time.Now().Add(-time.Minute) + Expect(tracker.GetAckFrame(true)).ToNot(BeNil()) + }) }) }) })