From 4633acd7bf0241e25b9ee556de7be2f969168a31 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Mon, 18 Jun 2018 10:42:45 +0700 Subject: [PATCH] only set the loss detection alarm if packets are outstanding --- internal/ackhandler/sent_packet_handler.go | 4 +- .../ackhandler/sent_packet_handler_test.go | 29 +++--- internal/ackhandler/sent_packet_history.go | 41 ++++++++ .../ackhandler/sent_packet_history_test.go | 97 +++++++++++++++++++ 4 files changed, 156 insertions(+), 15 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 3e6ffd90..7bb0ee81 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -301,12 +301,12 @@ func (h *sentPacketHandler) maybeUpdateRTT(largestAcked protocol.PacketNumber, a func (h *sentPacketHandler) updateLossDetectionAlarm() { // Cancel the alarm if no packets are outstanding - if h.packetHistory.Len() == 0 { + if !h.packetHistory.HasOutstandingPackets() { h.alarm = time.Time{} return } - if !h.handshakeComplete { + if h.packetHistory.HasOutstandingHandshakePackets() { h.alarm = h.lastSentHandshakePacketTime.Add(h.computeHandshakeTimeout()) } else if !h.lossTime.IsZero() { // Early retransmit timer or time loss detection. diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index 78279ffb..ebb5561d 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -782,6 +782,15 @@ var _ = Describe("SentPacketHandler", func() { }) }) + It("doesn't set an alarm if there are no outstanding packets", func() { + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 10})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 11})) + ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 10, Largest: 11}}} + err := handler.ReceivedAck(ack, 1, protocol.EncryptionForwardSecure, time.Now()) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.GetAlarmTimeout()).To(BeZero()) + }) + Context("TLPs", func() { It("uses the RTT from RTT stats", func() { rtt := 2 * time.Second @@ -997,7 +1006,6 @@ var _ = Describe("SentPacketHandler", func() { // Packet 1 should be considered lost (1+1/8) RTTs after it was sent. Expect(handler.lossTime.IsZero()).To(BeFalse()) Expect(handler.lossTime.Sub(getPacket(1).SendTime)).To(Equal(time.Second * 9 / 8)) - // Expect(time.Until(handler.GetAlarmTimeout())).To(BeNumerically("~", time.Hour*9/8, time.Minute)) err = handler.OnAlarm() Expect(err).ToNot(HaveOccurred()) @@ -1016,13 +1024,11 @@ var _ = Describe("SentPacketHandler", func() { now := time.Now() sendTime := now.Add(-time.Minute) lastHandshakePacketSendTime := now.Add(-30 * time.Second) - // send handshake packets: 1, 2, 4 - // send a forward-secure packet: 3 + // send handshake packets: 1, 3 + // send a forward-secure packet: 2 handler.SentPacket(handshakePacket(&Packet{PacketNumber: 1, SendTime: sendTime})) - handler.SentPacket(handshakePacket(&Packet{PacketNumber: 2, SendTime: sendTime})) - handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 3, SendTime: sendTime})) - handler.SentPacket(handshakePacket(&Packet{PacketNumber: 4, SendTime: lastHandshakePacketSendTime})) - handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 5, SendTime: now})) + handler.SentPacket(retransmittablePacket(&Packet{PacketNumber: 2, SendTime: sendTime})) + handler.SentPacket(handshakePacket(&Packet{PacketNumber: 3, SendTime: sendTime})) ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 1, Largest: 1}}} err := handler.ReceivedAck(ack, 1, protocol.EncryptionForwardSecure, now) @@ -1030,18 +1036,15 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.rttStats.SmoothedRTT()).To(Equal(time.Minute)) Expect(err).NotTo(HaveOccurred()) Expect(handler.lossTime.IsZero()).To(BeTrue()) - Expect(handler.GetAlarmTimeout().Sub(lastHandshakePacketSendTime)).To(Equal(2 * time.Minute)) + Expect(handler.GetAlarmTimeout().Sub(sendTime)).To(Equal(2 * time.Minute)) err = handler.OnAlarm() Expect(err).ToNot(HaveOccurred()) p := handler.DequeuePacketForRetransmission() Expect(p).ToNot(BeNil()) - Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(2))) - p = handler.DequeuePacketForRetransmission() - Expect(p).ToNot(BeNil()) - Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(4))) - Expect(getPacket(3)).ToNot(BeNil()) + Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(3))) Expect(handler.handshakeCount).To(BeEquivalentTo(1)) + handler.SentPacket(handshakePacket(&Packet{PacketNumber: 4, SendTime: lastHandshakePacketSendTime})) // make sure the exponential backoff is used Expect(handler.GetAlarmTimeout().Sub(lastHandshakePacketSendTime)).To(Equal(4 * time.Minute)) }) diff --git a/internal/ackhandler/sent_packet_history.go b/internal/ackhandler/sent_packet_history.go index 38a2a0e5..91aa2697 100644 --- a/internal/ackhandler/sent_packet_history.go +++ b/internal/ackhandler/sent_packet_history.go @@ -10,6 +10,9 @@ type sentPacketHistory struct { packetList *PacketList packetMap map[protocol.PacketNumber]*PacketElement + numOutstandingPackets int + numOutstandingHandshakePackets int + firstOutstanding *PacketElement } @@ -30,6 +33,12 @@ func (h *sentPacketHistory) sentPacketImpl(p *Packet) *PacketElement { if h.firstOutstanding == nil { h.firstOutstanding = el } + if p.canBeRetransmitted { + h.numOutstandingPackets++ + if p.EncryptionLevel < protocol.EncryptionForwardSecure { + h.numOutstandingHandshakePackets++ + } + } return el } @@ -92,6 +101,18 @@ func (h *sentPacketHistory) MarkCannotBeRetransmitted(pn protocol.PacketNumber) if !ok { return fmt.Errorf("sent packet history: packet %d not found", pn) } + if el.Value.canBeRetransmitted { + h.numOutstandingPackets-- + if h.numOutstandingPackets < 0 { + panic("numOutstandingHandshakePackets negative") + } + if el.Value.EncryptionLevel < protocol.EncryptionForwardSecure { + h.numOutstandingHandshakePackets-- + if h.numOutstandingHandshakePackets < 0 { + panic("numOutstandingHandshakePackets negative") + } + } + } el.Value.canBeRetransmitted = false if el == h.firstOutstanding { h.readjustFirstOutstanding() @@ -121,7 +142,27 @@ func (h *sentPacketHistory) Remove(p protocol.PacketNumber) error { if el == h.firstOutstanding { h.readjustFirstOutstanding() } + if el.Value.canBeRetransmitted { + h.numOutstandingPackets-- + if h.numOutstandingPackets < 0 { + panic("numOutstandingHandshakePackets negative") + } + if el.Value.EncryptionLevel < protocol.EncryptionForwardSecure { + h.numOutstandingHandshakePackets-- + if h.numOutstandingHandshakePackets < 0 { + panic("numOutstandingHandshakePackets negative") + } + } + } h.packetList.Remove(el) delete(h.packetMap, p) return nil } + +func (h *sentPacketHistory) HasOutstandingPackets() bool { + return h.numOutstandingPackets > 0 +} + +func (h *sentPacketHistory) HasOutstandingHandshakePackets() bool { + return h.numOutstandingHandshakePackets > 0 +} diff --git a/internal/ackhandler/sent_packet_history_test.go b/internal/ackhandler/sent_packet_history_test.go index fae82c2a..dc6ed5dd 100644 --- a/internal/ackhandler/sent_packet_history_test.go +++ b/internal/ackhandler/sent_packet_history_test.go @@ -197,4 +197,101 @@ var _ = Describe("SentPacketHistory", func() { Expect(hist.GetPacket(13).retransmissionOf).To(BeZero()) }) }) + + Context("outstanding packets", func() { + It("says if it has outstanding handshake packets", func() { + Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse()) + hist.SentPacket(&Packet{ + EncryptionLevel: protocol.EncryptionUnencrypted, + canBeRetransmitted: true, + }) + Expect(hist.HasOutstandingHandshakePackets()).To(BeTrue()) + }) + + It("says if it has outstanding packets", func() { + Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse()) + Expect(hist.HasOutstandingPackets()).To(BeFalse()) + hist.SentPacket(&Packet{ + EncryptionLevel: protocol.EncryptionForwardSecure, + canBeRetransmitted: true, + }) + Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse()) + Expect(hist.HasOutstandingPackets()).To(BeTrue()) + }) + + It("doesn't consider non-retransmittable packets as outstanding", func() { + hist.SentPacket(&Packet{ + EncryptionLevel: protocol.EncryptionUnencrypted, + }) + Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse()) + Expect(hist.HasOutstandingPackets()).To(BeFalse()) + }) + + It("accounts for deleted handshake packets", func() { + hist.SentPacket(&Packet{ + PacketNumber: 5, + EncryptionLevel: protocol.EncryptionSecure, + canBeRetransmitted: true, + }) + Expect(hist.HasOutstandingHandshakePackets()).To(BeTrue()) + err := hist.Remove(5) + Expect(err).ToNot(HaveOccurred()) + Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse()) + }) + + It("accounts for deleted packets", func() { + hist.SentPacket(&Packet{ + PacketNumber: 10, + EncryptionLevel: protocol.EncryptionForwardSecure, + canBeRetransmitted: true, + }) + Expect(hist.HasOutstandingPackets()).To(BeTrue()) + err := hist.Remove(10) + Expect(err).ToNot(HaveOccurred()) + Expect(hist.HasOutstandingPackets()).To(BeFalse()) + }) + + It("doesn't count handshake packets marked as non-retransmittable", func() { + hist.SentPacket(&Packet{ + PacketNumber: 5, + EncryptionLevel: protocol.EncryptionUnencrypted, + canBeRetransmitted: true, + }) + Expect(hist.HasOutstandingHandshakePackets()).To(BeTrue()) + err := hist.MarkCannotBeRetransmitted(5) + Expect(err).ToNot(HaveOccurred()) + Expect(hist.HasOutstandingHandshakePackets()).To(BeFalse()) + }) + + It("doesn't count packets marked as non-retransmittable", func() { + hist.SentPacket(&Packet{ + PacketNumber: 10, + EncryptionLevel: protocol.EncryptionForwardSecure, + canBeRetransmitted: true, + }) + Expect(hist.HasOutstandingPackets()).To(BeTrue()) + err := hist.MarkCannotBeRetransmitted(10) + Expect(err).ToNot(HaveOccurred()) + Expect(hist.HasOutstandingPackets()).To(BeFalse()) + }) + + It("counts the number of packets", func() { + hist.SentPacket(&Packet{ + PacketNumber: 10, + EncryptionLevel: protocol.EncryptionForwardSecure, + canBeRetransmitted: true, + }) + hist.SentPacket(&Packet{ + PacketNumber: 11, + EncryptionLevel: protocol.EncryptionForwardSecure, + canBeRetransmitted: true, + }) + err := hist.Remove(11) + Expect(err).ToNot(HaveOccurred()) + Expect(hist.HasOutstandingPackets()).To(BeTrue()) + err = hist.Remove(10) + Expect(err).ToNot(HaveOccurred()) + Expect(hist.HasOutstandingPackets()).To(BeFalse()) + }) + }) })