From 605607d39b60c3474d00c48aff165ef1b74e5b94 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 30 Mar 2018 23:59:19 +0700 Subject: [PATCH] implement spurious RTO detection --- internal/ackhandler/sent_packet_handler.go | 32 +++++-- .../ackhandler/sent_packet_handler_test.go | 87 ++++++++++++++++--- 2 files changed, 96 insertions(+), 23 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 90f1d292..3bedcd3f 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -42,6 +42,7 @@ type sentPacketHandler struct { // example: we send an ACK for packets 90-100 with packet number 20 // once we receive an ACK from the peer for packet 20, the lowestPacketNotConfirmedAcked is 101 lowestPacketNotConfirmedAcked protocol.PacketNumber + largestSentBeforeRTO protocol.PacketNumber packetHistory *sentPacketHistory stopWaitingManager stopWaitingManager @@ -316,6 +317,8 @@ func (h *sentPacketHandler) detectLostPackets(now time.Time) error { if err := h.queuePacketForRetransmission(p); err != nil { return err } + } + if p.includedInBytesInFlight { h.congestion.OnPacketLost(p.PacketNumber, p.Length, h.bytesInFlight) } h.packetHistory.Remove(p.PacketNumber) @@ -357,9 +360,6 @@ func (h *sentPacketHandler) onPacketAcked(p *Packet) error { if packet := h.packetHistory.GetPacket(p.PacketNumber); packet == nil { return nil } - h.rtoCount = 0 - h.handshakeCount = 0 - // TODO(#497): h.tlpCount = 0 // only report the acking of this packet to the congestion controller if: // * it is a retransmittable packet @@ -381,16 +381,20 @@ func (h *sentPacketHandler) onPacketAcked(p *Packet) error { } } } + // this also applies to packets that have been retransmitted as probe packets if p.includedInBytesInFlight { h.bytesInFlight -= p.Length - } - // TODO: this will need to change once we implement sending of probe packets - if p.canBeRetransmitted { h.congestion.OnPacketAcked(p.PacketNumber, p.Length, h.bytesInFlight) } + if h.rtoCount > 0 { + h.verifyRTO(p.PacketNumber) + } if err := h.stopRetransmissionsFor(p); err != nil { return err } + h.rtoCount = 0 + h.handshakeCount = 0 + // TODO(#497): h.tlpCount = 0 return h.packetHistory.Remove(p.PacketNumber) } @@ -408,6 +412,18 @@ func (h *sentPacketHandler) stopRetransmissionsFor(p *Packet) error { return nil } +func (h *sentPacketHandler) verifyRTO(pn protocol.PacketNumber) { + if pn <= h.largestSentBeforeRTO { + h.logger.Debugf("Spurious RTO detected. Received an ACK for %#x (largest sent before RTO: %#x)", pn, h.largestSentBeforeRTO) + // Replace SRTT with latest_rtt and increase the variance to prevent + // a spurious RTO from happening again. + h.rttStats.ExpireSmoothedMetrics() + return + } + h.logger.Debugf("RTO verified. Received an ACK for %#x (largest sent before RTO: %#x", pn, h.largestSentBeforeRTO) + h.congestion.OnRetransmissionTimeout(true) +} + func (h *sentPacketHandler) DequeuePacketForRetransmission() *Packet { if len(h.retransmissionQueue) == 0 { return nil @@ -469,18 +485,16 @@ func (h *sentPacketHandler) ShouldSendNumPackets() int { // retransmit the oldest two packets func (h *sentPacketHandler) queueRTOs() error { + h.largestSentBeforeRTO = h.lastSentPacketNumber // Queue the first two outstanding packets for retransmission. // This does NOT declare this packets as lost: // They are still tracked in the packet history and count towards the bytes in flight. - // TODO: don't report them as lost to the congestion controller for i := 0; i < 2; i++ { if p := h.packetHistory.FirstOutstanding(); p != nil { h.logger.Debugf("\tQueueing packet %#x for retransmission (RTO)", p.PacketNumber) if err := h.queuePacketForRetransmission(p); err != nil { return err } - h.congestion.OnPacketLost(p.PacketNumber, p.Length, h.bytesInFlight) - h.congestion.OnRetransmissionTimeout(true) } } return nil diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index 1245a13a..92b65f7a 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -541,24 +541,62 @@ var _ = Describe("SentPacketHandler", func() { Expect(err).NotTo(HaveOccurred()) }) - It("should call MaybeExitSlowStart and OnPacketLost", func() { + It("doesn't call OnPacketLost and OnRetransmissionTimeout when queuing RTOs", func() { cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(3) cong.EXPECT().TimeUntilSend(gomock.Any()).Times(3) - cong.EXPECT().OnRetransmissionTimeout(true).Times(2) - cong.EXPECT().OnPacketLost( - protocol.PacketNumber(1), - protocol.ByteCount(1), - protocol.ByteCount(3), - ) - cong.EXPECT().OnPacketLost( - protocol.PacketNumber(2), - protocol.ByteCount(1), - protocol.ByteCount(3), - ) handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 1})) handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 2})) handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 3})) handler.OnAlarm() // RTO, meaning 2 lost packets + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + }) + + It("declares all lower packets lost and call OnRetransmissionTimeout when verifying an RTO", func() { + cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(5) + cong.EXPECT().TimeUntilSend(gomock.Any()).Times(5) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 2, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 3, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 4, SendTime: time.Now().Add(-time.Hour)})) + handler.OnAlarm() // RTO, meaning 2 lost packets + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + // send one probe packet and receive an ACK for it + gomock.InOrder( + cong.EXPECT().MaybeExitSlowStart(), + cong.EXPECT().OnPacketAcked(protocol.PacketNumber(5), protocol.ByteCount(1), protocol.ByteCount(4)), + cong.EXPECT().OnRetransmissionTimeout(true), + cong.EXPECT().OnPacketLost(protocol.PacketNumber(1), protocol.ByteCount(1), protocol.ByteCount(3)), + cong.EXPECT().OnPacketLost(protocol.PacketNumber(2), protocol.ByteCount(1), protocol.ByteCount(2)), + cong.EXPECT().OnPacketLost(protocol.PacketNumber(3), protocol.ByteCount(1), protocol.ByteCount(1)), + cong.EXPECT().OnPacketLost(protocol.PacketNumber(4), protocol.ByteCount(1), protocol.ByteCount(0)), + ) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5})) + err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 5, LowestAcked: 5}, 1, protocol.EncryptionForwardSecure, time.Now()) + Expect(err).ToNot(HaveOccurred()) + }) + + It("doesn't call OnRetransmissionTimeout when a spurious RTO occurs", func() { + cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(3) + cong.EXPECT().TimeUntilSend(gomock.Any()).Times(3) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 2, SendTime: time.Now()})) + handler.OnAlarm() // RTO, meaning 2 lost packets + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + // send one probe packet + + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 3})) + // receive an ACK for a packet send *before* the probe packet + // don't EXPECT any call to OnRetransmissionTimeout + gomock.InOrder( + cong.EXPECT().MaybeExitSlowStart(), + cong.EXPECT().OnPacketAcked(protocol.PacketNumber(2), protocol.ByteCount(1), protocol.ByteCount(2)), + cong.EXPECT().OnPacketLost(protocol.PacketNumber(1), protocol.ByteCount(1), protocol.ByteCount(1)), + ) + err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 2, LowestAcked: 2}, 1, protocol.EncryptionForwardSecure, time.Now()) + Expect(err).ToNot(HaveOccurred()) }) It("doesn't call OnPacketAcked when a retransmitted packet is acked", func() { @@ -705,13 +743,34 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) expectInPacketHistory([]protocol.PacketNumber{1, 2}) Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(2))) - // send another packet and receive an ACK for it - // This will trigger ack-based loss detection, and declare 1 and 2 lost, and remove them from bytes in flight + // Send a probe packet and receive an ACK for it. + // This verifies the RTO. handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 3})) err = handler.ReceivedAck(&wire.AckFrame{LargestAcked: 3, LowestAcked: 3}, 1, protocol.EncryptionForwardSecure, time.Now()) Expect(err).ToNot(HaveOccurred()) Expect(handler.packetHistory.Len()).To(BeZero()) Expect(handler.bytesInFlight).To(BeZero()) + Expect(handler.retransmissionQueue).To(BeEmpty()) // 1 and 2 were already sent as probe packets + }) + + It("queues packets sent before the probe packet for retransmission", func() { + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 1, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 2, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 3, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 4, SendTime: time.Now().Add(-time.Hour)})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, SendTime: time.Now().Add(-time.Hour)})) + handler.OnAlarm() + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + expectInPacketHistory([]protocol.PacketNumber{1, 2, 3, 4, 5}) + // Send a probe packet and receive an ACK for it. + // This verifies the RTO. + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 6})) + err := handler.ReceivedAck(&wire.AckFrame{LargestAcked: 6, LowestAcked: 6}, 1, protocol.EncryptionForwardSecure, time.Now()) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.packetHistory.Len()).To(BeZero()) + Expect(handler.bytesInFlight).To(BeZero()) + Expect(handler.retransmissionQueue).To(HaveLen(3)) // packets 3, 4, 5 }) It("handles ACKs for the original packet", func() {