From 515f4b3dcd231b0e38e8a4e217f401bd0ad066f3 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Mon, 19 Jun 2017 14:07:34 +0200 Subject: [PATCH] Don't require ACKs of non-retransmittable packets The way this is implemented here is not particularly nice, but will improve in the future once we implement spurious RTO decection #687 (where we'll need a packetHistory similar to Chrome's). In particular, we don't consider non-retransmittable packets for RTT measurements or for early retransmit. While that might impact performance in some edge cases, it shouldn't be worse than before :) Fixes #505. --- ackhandler/interfaces.go | 1 + ackhandler/sent_packet_handler.go | 21 ++++++----- ackhandler/sent_packet_handler_test.go | 51 +++++++++++++++++--------- 3 files changed, 46 insertions(+), 27 deletions(-) diff --git a/ackhandler/interfaces.go b/ackhandler/interfaces.go index a9fc824b9..7dec882a8 100644 --- a/ackhandler/interfaces.go +++ b/ackhandler/interfaces.go @@ -9,6 +9,7 @@ import ( // SentPacketHandler handles ACKs received for outgoing packets type SentPacketHandler interface { + // SentPacket may modify the packet SentPacket(packet *Packet) error ReceivedAck(ackFrame *frames.AckFrame, withPacketNumber protocol.PacketNumber, recvTime time.Time) error diff --git a/ackhandler/sent_packet_handler.go b/ackhandler/sent_packet_handler.go index 179a1e1b7..300b665ed 100644 --- a/ackhandler/sent_packet_handler.go +++ b/ackhandler/sent_packet_handler.go @@ -106,26 +106,27 @@ func (h *sentPacketHandler) SentPacket(packet *Packet) error { } } - now := time.Now() - packet.SendTime = now - if packet.Length == 0 { - return errors.New("SentPacketHandler: packet cannot be empty") - } - h.bytesInFlight += packet.Length - h.lastSentPacketNumber = packet.PacketNumber - h.packetHistory.PushBack(*packet) + now := time.Now() + + packet.Frames = stripNonRetransmittableFrames(packet.Frames) + isRetransmittable := len(packet.Frames) != 0 + + if isRetransmittable { + packet.SendTime = now + h.bytesInFlight += packet.Length + h.packetHistory.PushBack(*packet) + } h.congestion.OnPacketSent( now, h.bytesInFlight, packet.PacketNumber, packet.Length, - true, /* TODO: is retransmittable */ + isRetransmittable, ) h.updateLossDetectionAlarm() - return nil } diff --git a/ackhandler/sent_packet_handler_test.go b/ackhandler/sent_packet_handler_test.go index 2e5d7faee..22dfbf875 100644 --- a/ackhandler/sent_packet_handler_test.go +++ b/ackhandler/sent_packet_handler_test.go @@ -57,6 +57,10 @@ func (m *mockCongestion) OnPacketLost(n protocol.PacketNumber, l protocol.ByteCo m.packetsLost = append(m.packetsLost, []interface{}{n, l, bif}) } +func retransmittablePacket(num protocol.PacketNumber) *Packet { + return &Packet{PacketNumber: num, Length: 1, Frames: []frames.Frame{&frames.PingFrame{}}} +} + var _ = Describe("SentPacketHandler", func() { var ( handler *sentPacketHandler @@ -133,6 +137,12 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.packetHistory.Front().Value.SendTime.Unix()).To(BeNumerically("~", time.Now().Unix(), 1)) }) + It("does not store non-retransmittable packets", func() { + err := handler.SentPacket(&Packet{PacketNumber: 1, Length: 1}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.packetHistory.Len()).To(BeZero()) + }) + Context("skipped packet numbers", func() { It("works with non-consecutive packet numbers", func() { packet1 := Packet{PacketNumber: 1, Frames: []frames.Frame{&streamFrame}, Length: 1} @@ -216,12 +226,10 @@ var _ = Describe("SentPacketHandler", func() { It("checks the size of the packet history, for unacked packets", func() { i := protocol.PacketNumber(1) for ; i <= protocol.MaxTrackedSentPackets; i++ { - packet := Packet{PacketNumber: protocol.PacketNumber(i), Length: 1} - err := handler.SentPacket(&packet) + err := handler.SentPacket(retransmittablePacket(i)) Expect(err).ToNot(HaveOccurred()) } - packet := Packet{PacketNumber: protocol.PacketNumber(i), Length: 1} - err := handler.SentPacket(&packet) + err := handler.SentPacket(retransmittablePacket(i)) Expect(err).To(MatchError(ErrTooManyTrackedSentPackets)) }) @@ -631,6 +639,7 @@ var _ = Describe("SentPacketHandler", func() { p := &Packet{ PacketNumber: 1, Length: 42, + Frames: []frames.Frame{&frames.PingFrame{}}, } err := handler.SentPacket(p) Expect(err).NotTo(HaveOccurred()) @@ -641,8 +650,8 @@ var _ = Describe("SentPacketHandler", func() { }) It("should call MaybeExitSlowStart and OnPacketAcked", func() { - handler.SentPacket(&Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1}) - handler.SentPacket(&Packet{PacketNumber: 2, Frames: []frames.Frame{}, Length: 1}) + handler.SentPacket(retransmittablePacket(1)) + handler.SentPacket(retransmittablePacket(2)) err := handler.ReceivedAck(&frames.AckFrame{LargestAcked: 1, LowestAcked: 1}, 1, time.Now()) Expect(err).NotTo(HaveOccurred()) Expect(cong.maybeExitSlowStart).To(BeTrue()) @@ -653,9 +662,9 @@ var _ = Describe("SentPacketHandler", func() { }) It("should call MaybeExitSlowStart and OnPacketLost", func() { - handler.SentPacket(&Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1}) - handler.SentPacket(&Packet{PacketNumber: 2, Frames: []frames.Frame{}, Length: 1}) - handler.SentPacket(&Packet{PacketNumber: 3, Frames: []frames.Frame{}, Length: 1}) + handler.SentPacket(retransmittablePacket(1)) + handler.SentPacket(retransmittablePacket(2)) + handler.SentPacket(retransmittablePacket(3)) handler.OnAlarm() // RTO, meaning 2 lost packets Expect(cong.maybeExitSlowStart).To(BeFalse()) Expect(cong.onRetransmissionTimeout).To(BeTrue()) @@ -668,7 +677,11 @@ var _ = Describe("SentPacketHandler", func() { It("allows or denies sending based on congestion", func() { Expect(handler.SendingAllowed()).To(BeTrue()) - err := handler.SentPacket(&Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: protocol.DefaultTCPMSS + 1}) + err := handler.SentPacket(&Packet{ + PacketNumber: 1, + Frames: []frames.Frame{&frames.PingFrame{}}, + Length: protocol.DefaultTCPMSS + 1, + }) Expect(err).NotTo(HaveOccurred()) Expect(handler.SendingAllowed()).To(BeFalse()) }) @@ -680,7 +693,11 @@ var _ = Describe("SentPacketHandler", func() { }) It("allows sending if there are retransmisisons outstanding", func() { - err := handler.SentPacket(&Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: protocol.DefaultTCPMSS + 1}) + err := handler.SentPacket(&Packet{ + PacketNumber: 1, + Frames: []frames.Frame{&frames.PingFrame{}}, + Length: protocol.DefaultTCPMSS + 1, + }) Expect(err).NotTo(HaveOccurred()) Expect(handler.SendingAllowed()).To(BeFalse()) handler.retransmissionQueue = []*Packet{nil} @@ -724,9 +741,9 @@ var _ = Describe("SentPacketHandler", func() { Context("Delay-based loss detection", func() { It("detects a packet as lost", func() { - err := handler.SentPacket(&Packet{PacketNumber: 1, Length: 1}) + err := handler.SentPacket(retransmittablePacket(1)) Expect(err).NotTo(HaveOccurred()) - err = handler.SentPacket(&Packet{PacketNumber: 2, Length: 1}) + err = handler.SentPacket(retransmittablePacket(2)) Expect(err).NotTo(HaveOccurred()) Expect(handler.lossTime.IsZero()).To(BeTrue()) @@ -747,9 +764,9 @@ var _ = Describe("SentPacketHandler", func() { It("does not detect packets as lost without ACKs", func() { err := handler.SentPacket(&Packet{PacketNumber: 1, Length: 1}) Expect(err).NotTo(HaveOccurred()) - err = handler.SentPacket(&Packet{PacketNumber: 2, Length: 1}) + err = handler.SentPacket(retransmittablePacket(2)) Expect(err).NotTo(HaveOccurred()) - err = handler.SentPacket(&Packet{PacketNumber: 3, Length: 1}) + err = handler.SentPacket(retransmittablePacket(3)) Expect(err).NotTo(HaveOccurred()) Expect(handler.lossTime.IsZero()).To(BeTrue()) @@ -767,9 +784,9 @@ var _ = Describe("SentPacketHandler", func() { Context("RTO retransmission", func() { It("queues two packets if RTO expires", func() { - err := handler.SentPacket(&Packet{PacketNumber: 1, Length: 1}) + err := handler.SentPacket(retransmittablePacket(1)) Expect(err).NotTo(HaveOccurred()) - err = handler.SentPacket(&Packet{PacketNumber: 2, Length: 1}) + err = handler.SentPacket(retransmittablePacket(2)) Expect(err).NotTo(HaveOccurred()) handler.rttStats.UpdateRTT(time.Hour, 0, time.Now())