From 833da82726740f5cfc8ffb34515febbd1ff9714b Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Thu, 15 Jun 2017 19:52:21 +0200 Subject: [PATCH 1/4] Move retransmittable frame logic to the ackhandler package --- ackhandler/retransmittable.go | 38 ++++++++++++++++++++++++ ackhandler/retransmittable_test.go | 44 ++++++++++++++++++++++++++++ packet_unpacker.go | 5 ++++ session.go | 3 +- unpacked_packet.go | 31 -------------------- unpacked_packet_test.go | 46 ------------------------------ 6 files changed, 89 insertions(+), 78 deletions(-) create mode 100644 ackhandler/retransmittable.go create mode 100644 ackhandler/retransmittable_test.go delete mode 100644 unpacked_packet.go delete mode 100644 unpacked_packet_test.go diff --git a/ackhandler/retransmittable.go b/ackhandler/retransmittable.go new file mode 100644 index 000000000..17437b8c9 --- /dev/null +++ b/ackhandler/retransmittable.go @@ -0,0 +1,38 @@ +package ackhandler + +import ( + "github.com/lucas-clemente/quic-go/frames" +) + +// Returns a new slice with all non-retransmittable frames deleted. +func stripNonRetransmittableFrames(fs []frames.Frame) []frames.Frame { + res := make([]frames.Frame, 0, len(fs)) + for _, f := range fs { + if IsFrameRetransmittable(f) { + res = append(res, f) + } + } + return res +} + +// IsFrameRetransmittable returns true if the frame should be retransmitted. +func IsFrameRetransmittable(f frames.Frame) bool { + switch f.(type) { + case *frames.StopWaitingFrame: + return false + case *frames.AckFrame: + return false + default: + return true + } +} + +// HasRetransmittableFrames returns true if at least one frame is retransmittable. +func HasRetransmittableFrames(fs []frames.Frame) bool { + for _, f := range fs { + if IsFrameRetransmittable(f) { + return true + } + } + return false +} diff --git a/ackhandler/retransmittable_test.go b/ackhandler/retransmittable_test.go new file mode 100644 index 000000000..4a5ea858b --- /dev/null +++ b/ackhandler/retransmittable_test.go @@ -0,0 +1,44 @@ +package ackhandler + +import ( + "reflect" + + "github.com/lucas-clemente/quic-go/frames" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("retransmittable frames", func() { + for fl, el := range map[frames.Frame]bool{ + &frames.AckFrame{}: false, + &frames.StopWaitingFrame{}: false, + &frames.BlockedFrame{}: true, + &frames.ConnectionCloseFrame{}: true, + &frames.GoawayFrame{}: true, + &frames.PingFrame{}: true, + &frames.RstStreamFrame{}: true, + &frames.StreamFrame{}: true, + &frames.WindowUpdateFrame{}: true, + } { + f := fl + e := el + fName := reflect.ValueOf(f).Elem().Type().Name() + + It("works for "+fName, func() { + Expect(IsFrameRetransmittable(f)).To(Equal(e)) + }) + + It("stripping non-retransmittable frames works for "+fName, func() { + s := []frames.Frame{f} + if e { + Expect(stripNonRetransmittableFrames(s)).To(Equal([]frames.Frame{f})) + } else { + Expect(stripNonRetransmittableFrames(s)).To(BeEmpty()) + } + }) + + It("HasRetransmittableFrames works for "+fName, func() { + Expect(HasRetransmittableFrames([]frames.Frame{f})).To(Equal(e)) + }) + } +}) diff --git a/packet_unpacker.go b/packet_unpacker.go index 30dee80a9..c92e6a53f 100644 --- a/packet_unpacker.go +++ b/packet_unpacker.go @@ -10,6 +10,11 @@ import ( "github.com/lucas-clemente/quic-go/qerr" ) +type unpackedPacket struct { + encryptionLevel protocol.EncryptionLevel + frames []frames.Frame +} + type quicAEAD interface { Open(dst, src []byte, packetNumber protocol.PacketNumber, associatedData []byte) ([]byte, protocol.EncryptionLevel, error) } diff --git a/session.go b/session.go index 6e187a7e2..0bebe993e 100644 --- a/session.go +++ b/session.go @@ -405,7 +405,8 @@ func (s *session) handlePacketImpl(p *receivedPacket) error { // Only do this after decrypting, so we are sure the packet is not attacker-controlled s.largestRcvdPacketNumber = utils.MaxPacketNumber(s.largestRcvdPacketNumber, hdr.PacketNumber) - if err = s.receivedPacketHandler.ReceivedPacket(hdr.PacketNumber, packet.IsRetransmittable()); err != nil { + isRetransmittable := ackhandler.HasRetransmittableFrames(packet.frames) + if err = s.receivedPacketHandler.ReceivedPacket(hdr.PacketNumber, isRetransmittable); err != nil { return err } diff --git a/unpacked_packet.go b/unpacked_packet.go deleted file mode 100644 index 0636b8f1e..000000000 --- a/unpacked_packet.go +++ /dev/null @@ -1,31 +0,0 @@ -package quic - -import ( - "github.com/lucas-clemente/quic-go/frames" - "github.com/lucas-clemente/quic-go/protocol" -) - -type unpackedPacket struct { - encryptionLevel protocol.EncryptionLevel - frames []frames.Frame -} - -func (u *unpackedPacket) IsRetransmittable() bool { - for _, f := range u.frames { - switch f.(type) { - case *frames.StreamFrame: - return true - case *frames.RstStreamFrame: - return true - case *frames.WindowUpdateFrame: - return true - case *frames.BlockedFrame: - return true - case *frames.PingFrame: - return true - case *frames.GoawayFrame: - return true - } - } - return false -} diff --git a/unpacked_packet_test.go b/unpacked_packet_test.go deleted file mode 100644 index 82112a260..000000000 --- a/unpacked_packet_test.go +++ /dev/null @@ -1,46 +0,0 @@ -package quic - -import ( - "github.com/lucas-clemente/quic-go/frames" - . "github.com/onsi/ginkgo" - . "github.com/onsi/gomega" -) - -var _ = Describe("Unpacked packet", func() { - var packet *unpackedPacket - BeforeEach(func() { - packet = &unpackedPacket{} - }) - - It("says that an empty packet is not retransmittable", func() { - Expect(packet.IsRetransmittable()).To(BeFalse()) - }) - - It("detects the frame types", func() { - packet.frames = []frames.Frame{&frames.AckFrame{}} - Expect(packet.IsRetransmittable()).To(BeFalse()) - packet.frames = []frames.Frame{&frames.BlockedFrame{}} - Expect(packet.IsRetransmittable()).To(BeTrue()) - packet.frames = []frames.Frame{&frames.GoawayFrame{}} - Expect(packet.IsRetransmittable()).To(BeTrue()) - packet.frames = []frames.Frame{&frames.PingFrame{}} - Expect(packet.IsRetransmittable()).To(BeTrue()) - packet.frames = []frames.Frame{&frames.StreamFrame{}} - Expect(packet.IsRetransmittable()).To(BeTrue()) - packet.frames = []frames.Frame{&frames.RstStreamFrame{}} - Expect(packet.IsRetransmittable()).To(BeTrue()) - packet.frames = []frames.Frame{&frames.StopWaitingFrame{}} - Expect(packet.IsRetransmittable()).To(BeFalse()) - packet.frames = []frames.Frame{&frames.WindowUpdateFrame{}} - Expect(packet.IsRetransmittable()).To(BeTrue()) - }) - - It("says that a packet is retransmittable if it contains one retransmittable frame", func() { - packet.frames = []frames.Frame{ - &frames.AckFrame{}, - &frames.WindowUpdateFrame{}, - &frames.StopWaitingFrame{}, - } - Expect(packet.IsRetransmittable()).To(BeTrue()) - }) -}) From 3cf7e061c55cb3a766b3d945bc0bab9e0d0c2447 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Mon, 19 Jun 2017 14:06:57 +0200 Subject: [PATCH 2/4] Fix the order of dequeueing lost packets --- ackhandler/sent_packet_handler.go | 9 +++++---- ackhandler/sent_packet_handler_test.go | 8 ++++++-- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/ackhandler/sent_packet_handler.go b/ackhandler/sent_packet_handler.go index 320f81a90..179a1e1b7 100644 --- a/ackhandler/sent_packet_handler.go +++ b/ackhandler/sent_packet_handler.go @@ -310,10 +310,11 @@ func (h *sentPacketHandler) DequeuePacketForRetransmission() *Packet { if len(h.retransmissionQueue) == 0 { return nil } - queueLen := len(h.retransmissionQueue) - // packets are usually NACKed in descending order. So use the slice as a stack - packet := h.retransmissionQueue[queueLen-1] - h.retransmissionQueue = h.retransmissionQueue[:queueLen-1] + packet := h.retransmissionQueue[0] + // Shift the slice and don't retain anything that isn't needed. + copy(h.retransmissionQueue, h.retransmissionQueue[1:]) + h.retransmissionQueue[len(h.retransmissionQueue)-1] = nil + h.retransmissionQueue = h.retransmissionQueue[:len(h.retransmissionQueue)-1] return packet } diff --git a/ackhandler/sent_packet_handler_test.go b/ackhandler/sent_packet_handler_test.go index cd531fb15..2e5d7faee 100644 --- a/ackhandler/sent_packet_handler_test.go +++ b/ackhandler/sent_packet_handler_test.go @@ -777,8 +777,12 @@ var _ = Describe("SentPacketHandler", func() { Expect(handler.GetAlarmTimeout().Sub(time.Now())).To(BeNumerically("~", handler.computeRTOTimeout(), time.Minute)) handler.OnAlarm() - Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) - Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) + p := handler.DequeuePacketForRetransmission() + Expect(p).ToNot(BeNil()) + Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(1))) + p = handler.DequeuePacketForRetransmission() + Expect(p).ToNot(BeNil()) + Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(2))) Expect(handler.rtoCount).To(BeEquivalentTo(1)) }) From 515f4b3dcd231b0e38e8a4e217f401bd0ad066f3 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Mon, 19 Jun 2017 14:07:34 +0200 Subject: [PATCH 3/4] 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()) From 472f2b24c00947586469842f018b0fb492b7df96 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Mon, 19 Jun 2017 16:13:09 +0200 Subject: [PATCH 4/4] Don't call OnAlarm() if no alarm is set --- session.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/session.go b/session.go index 0bebe993e..cee4912b5 100644 --- a/session.go +++ b/session.go @@ -293,7 +293,7 @@ runLoop: } now := time.Now() - if s.sentPacketHandler.GetAlarmTimeout().Before(now) { + if timeout := s.sentPacketHandler.GetAlarmTimeout(); !timeout.IsZero() && timeout.Before(now) { // This could cause packets to be retransmitted, so check it before trying // to send packets. s.sentPacketHandler.OnAlarm()