From 5cb3c0a771557b654395f91e3eb427242d644e2a Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Fri, 12 Aug 2016 22:22:26 +0200 Subject: [PATCH] replace ProbablyHasPacketForRetransmission with a call to MaybeQueueRTOs fixes #261 --- ackhandler/interfaces.go | 2 +- ackhandler/sent_packet_handler.go | 12 ++------- ackhandler/sent_packet_handler_test.go | 19 +++++-------- ackhandlerlegacy/interfaces.go | 2 +- ackhandlerlegacy/sent_packet_handler.go | 13 ++------- ackhandlerlegacy/sent_packet_handler_test.go | 28 +++++++------------- session.go | 3 +++ session_test.go | 19 ++++++++++--- 8 files changed, 40 insertions(+), 58 deletions(-) diff --git a/ackhandler/interfaces.go b/ackhandler/interfaces.go index d478838e..3dc2631b 100644 --- a/ackhandler/interfaces.go +++ b/ackhandler/interfaces.go @@ -15,7 +15,7 @@ type SentPacketHandler interface { GetStopWaitingFrame() *frames.StopWaitingFrame - ProbablyHasPacketForRetransmission() bool + MaybeQueueRTOs() DequeuePacketForRetransmission() (packet *ackhandlerlegacy.Packet) BytesInFlight() protocol.ByteCount diff --git a/ackhandler/sent_packet_handler.go b/ackhandler/sent_packet_handler.go index 276ac853..358e14dc 100644 --- a/ackhandler/sent_packet_handler.go +++ b/ackhandler/sent_packet_handler.go @@ -262,16 +262,8 @@ func (h *sentPacketHandler) ReceivedAck(ackFrame *frames.AckFrame, withPacketNum return nil } -// ProbablyHasPacketForRetransmission returns if there is a packet queued for retransmission -// There is one case where it gets the answer wrong: -// if a packet has already been queued for retransmission, but a belated ACK is received for this packet, this function will return true, although the packet will not be returend for retransmission by DequeuePacketForRetransmission() -func (h *sentPacketHandler) ProbablyHasPacketForRetransmission() bool { - h.maybeQueuePacketsRTO() - return len(h.retransmissionQueue) > 0 -} - func (h *sentPacketHandler) DequeuePacketForRetransmission() *ackhandlerlegacy.Packet { - if !h.ProbablyHasPacketForRetransmission() { + if len(h.retransmissionQueue) == 0 { return nil } @@ -310,7 +302,7 @@ func (h *sentPacketHandler) CheckForError() error { return nil } -func (h *sentPacketHandler) maybeQueuePacketsRTO() { +func (h *sentPacketHandler) MaybeQueueRTOs() { if time.Now().Before(h.TimeOfFirstRTO()) { return } diff --git a/ackhandler/sent_packet_handler_test.go b/ackhandler/sent_packet_handler_test.go index 15da4da2..5c75555a 100644 --- a/ackhandler/sent_packet_handler_test.go +++ b/ackhandler/sent_packet_handler_test.go @@ -568,7 +568,7 @@ var _ = Describe("SentPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) } Expect(getPacketElement(2)).ToNot(BeNil()) - Expect(handler.ProbablyHasPacketForRetransmission()).To(BeFalse()) + handler.MaybeQueueRTOs() Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) }) @@ -578,7 +578,7 @@ var _ = Describe("SentPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) } Expect(getPacketElement(2)).To(BeNil()) - Expect(handler.ProbablyHasPacketForRetransmission()).To(BeTrue()) + handler.MaybeQueueRTOs() Expect(handler.retransmissionQueue).To(HaveLen(1)) Expect(handler.retransmissionQueue[0].PacketNumber).To(Equal(protocol.PacketNumber(2))) }) @@ -819,7 +819,7 @@ var _ = Describe("SentPacketHandler", func() { err := handler.SentPacket(&ackhandlerlegacy.Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1}) Expect(err).NotTo(HaveOccurred()) handler.lastSentPacketTime = time.Now().Add(-time.Second) - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(cong.nCalls).To(Equal(3)) // rttUpdated, bytesInFlight, ackedPackets, lostPackets Expect(cong.argsOnCongestionEvent[0]).To(BeFalse()) @@ -866,7 +866,7 @@ var _ = Describe("SentPacketHandler", func() { It("does nothing if not required", func() { err := handler.SentPacket(&ackhandlerlegacy.Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1}) Expect(err).NotTo(HaveOccurred()) - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(handler.retransmissionQueue).To(BeEmpty()) }) @@ -875,26 +875,19 @@ var _ = Describe("SentPacketHandler", func() { err := handler.SentPacket(p) Expect(err).NotTo(HaveOccurred()) handler.lastSentPacketTime = time.Now().Add(-time.Second) - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(handler.retransmissionQueue).To(HaveLen(1)) Expect(handler.retransmissionQueue[0].PacketNumber).To(Equal(p.PacketNumber)) Expect(time.Now().Sub(handler.lastSentPacketTime)).To(BeNumerically("<", time.Second/2)) }) }) - It("works with HasPacketForRetransmission", func() { - p := &ackhandlerlegacy.Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1} - err := handler.SentPacket(p) - Expect(err).NotTo(HaveOccurred()) - handler.lastSentPacketTime = time.Now().Add(-time.Second) - Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) - }) - It("works with DequeuePacketForRetransmission", func() { p := &ackhandlerlegacy.Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1} err := handler.SentPacket(p) Expect(err).NotTo(HaveOccurred()) handler.lastSentPacketTime = time.Now().Add(-time.Second) + handler.MaybeQueueRTOs() Expect(handler.DequeuePacketForRetransmission().PacketNumber).To(Equal(p.PacketNumber)) }) }) diff --git a/ackhandlerlegacy/interfaces.go b/ackhandlerlegacy/interfaces.go index 51299b7d..ceb5d78c 100644 --- a/ackhandlerlegacy/interfaces.go +++ b/ackhandlerlegacy/interfaces.go @@ -14,7 +14,7 @@ type SentPacketHandler interface { GetStopWaitingFrame() *frames.StopWaitingFrame - ProbablyHasPacketForRetransmission() bool + MaybeQueueRTOs() DequeuePacketForRetransmission() (packet *Packet) BytesInFlight() protocol.ByteCount diff --git a/ackhandlerlegacy/sent_packet_handler.go b/ackhandlerlegacy/sent_packet_handler.go index 325e6fdf..33012fbe 100644 --- a/ackhandlerlegacy/sent_packet_handler.go +++ b/ackhandlerlegacy/sent_packet_handler.go @@ -273,17 +273,8 @@ func (h *sentPacketHandler) ReceivedAck(ackFrameOrig *frames.AckFrame, withPacke return nil } -// ProbablyHasPacketForRetransmission returns if there is a packet queued for retransmission -// There is one case where it gets the answer wrong: -// if a packet has already been queued for retransmission, but a belated ACK is received for this packet, this function will return true, although the packet will not be returend for retransmission by DequeuePacketForRetransmission() -func (h *sentPacketHandler) ProbablyHasPacketForRetransmission() bool { - h.maybeQueuePacketsRTO() - - return len(h.retransmissionQueue) > 0 -} - func (h *sentPacketHandler) DequeuePacketForRetransmission() (packet *Packet) { - if !h.ProbablyHasPacketForRetransmission() { + if len(h.retransmissionQueue) == 0 { return nil } @@ -329,7 +320,7 @@ func (h *sentPacketHandler) CheckForError() error { return nil } -func (h *sentPacketHandler) maybeQueuePacketsRTO() { +func (h *sentPacketHandler) MaybeQueueRTOs() { if time.Now().Before(h.TimeOfFirstRTO()) { return } diff --git a/ackhandlerlegacy/sent_packet_handler_test.go b/ackhandlerlegacy/sent_packet_handler_test.go index e8e5a40e..aa48a117 100644 --- a/ackhandlerlegacy/sent_packet_handler_test.go +++ b/ackhandlerlegacy/sent_packet_handler_test.go @@ -513,7 +513,7 @@ var _ = Describe("SentPacketHandler", func() { _, err := handler.nackPacket(2) Expect(err).ToNot(HaveOccurred()) } - Expect(handler.ProbablyHasPacketForRetransmission()).To(BeFalse()) + handler.MaybeQueueRTOs() Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) }) @@ -522,7 +522,7 @@ var _ = Describe("SentPacketHandler", func() { _, err := handler.nackPacket(2) Expect(err).ToNot(HaveOccurred()) } - Expect(handler.ProbablyHasPacketForRetransmission()).To(BeTrue()) + handler.MaybeQueueRTOs() Expect(handler.retransmissionQueue).To(HaveLen(1)) Expect(handler.retransmissionQueue[0].PacketNumber).To(Equal(protocol.PacketNumber(2))) }) @@ -594,8 +594,7 @@ var _ = Describe("SentPacketHandler", func() { } // this is the belated ACK handler.ackPacket(2) - // this is the edge case where ProbablyHasPacketForRetransmission() get's it wrong: it says there's probably a packet for retransmission, but actually there isn't - Expect(handler.ProbablyHasPacketForRetransmission()).To(BeTrue()) + handler.MaybeQueueRTOs() Expect(handler.DequeuePacketForRetransmission()).To(BeNil()) }) }) @@ -712,7 +711,7 @@ var _ = Describe("SentPacketHandler", func() { err := handler.SentPacket(&Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1}) Expect(err).NotTo(HaveOccurred()) handler.lastSentPacketTime = time.Now().Add(-time.Second) - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(cong.nCalls).To(Equal(3)) // rttUpdated, bytesInFlight, ackedPackets, lostPackets Expect(cong.argsOnCongestionEvent[0]).To(BeFalse()) @@ -757,7 +756,7 @@ var _ = Describe("SentPacketHandler", func() { It("ignores nil packets", func() { handler.packetHistory[1] = nil - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(handler.TimeOfFirstRTO().IsZero()).To(BeTrue()) }) }) @@ -766,7 +765,7 @@ var _ = Describe("SentPacketHandler", func() { It("does nothing if not required", func() { err := handler.SentPacket(&Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1}) Expect(err).NotTo(HaveOccurred()) - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(handler.retransmissionQueue).To(BeEmpty()) }) @@ -775,7 +774,7 @@ var _ = Describe("SentPacketHandler", func() { err := handler.SentPacket(p) Expect(err).NotTo(HaveOccurred()) handler.lastSentPacketTime = time.Now().Add(-time.Second) - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(handler.retransmissionQueue).To(HaveLen(1)) Expect(handler.retransmissionQueue[0]).To(Equal(p)) Expect(time.Now().Sub(handler.lastSentPacketTime)).To(BeNumerically("<", time.Second/2)) @@ -786,30 +785,23 @@ var _ = Describe("SentPacketHandler", func() { err := handler.SentPacket(p) Expect(err).NotTo(HaveOccurred()) handler.lastSentPacketTime = time.Now().Add(-time.Second) - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(handler.retransmissionQueue).To(BeEmpty()) }) It("ignores nil packets", func() { handler.packetHistory[1] = nil - handler.maybeQueuePacketsRTO() + handler.MaybeQueueRTOs() Expect(handler.retransmissionQueue).To(BeEmpty()) }) }) - It("works with HasPacketForRetransmission", func() { - p := &Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1} - err := handler.SentPacket(p) - Expect(err).NotTo(HaveOccurred()) - handler.lastSentPacketTime = time.Now().Add(-time.Second) - Expect(handler.DequeuePacketForRetransmission()).ToNot(BeNil()) - }) - It("works with DequeuePacketForRetransmission", func() { p := &Packet{PacketNumber: 1, Frames: []frames.Frame{}, Length: 1} err := handler.SentPacket(p) Expect(err).NotTo(HaveOccurred()) handler.lastSentPacketTime = time.Now().Add(-time.Second) + handler.MaybeQueueRTOs() Expect(handler.DequeuePacketForRetransmission()).To(Equal(p)) }) }) diff --git a/session.go b/session.go index 5692e6a7..667fac3b 100644 --- a/session.go +++ b/session.go @@ -440,6 +440,9 @@ func (s *Session) sendPacket() error { return err } + // Do this before checking the congestion, since we might de-congestionize here :) + s.sentPacketHandler.MaybeQueueRTOs() + if !s.sentPacketHandler.CongestionAllowsSending() { return nil } diff --git a/session_test.go b/session_test.go index 477484b9..c613f61e 100644 --- a/session_test.go +++ b/session_test.go @@ -47,7 +47,9 @@ func (m *mockUnpacker) Unpack(publicHeaderBinary []byte, hdr *PublicHeader, data } type mockSentPacketHandler struct { - retransmissionQueue []*ackhandlerlegacy.Packet + retransmissionQueue []*ackhandlerlegacy.Packet + congestionLimited bool + maybeQueueRTOsCalled bool } func (h *mockSentPacketHandler) SentPacket(packet *ackhandlerlegacy.Packet) error { return nil } @@ -59,12 +61,12 @@ func (h *mockSentPacketHandler) GetLeastUnacked() protocol.PacketNumber { return func (h *mockSentPacketHandler) GetStopWaitingFrame() *frames.StopWaitingFrame { panic("not implemented") } -func (h *mockSentPacketHandler) CongestionAllowsSending() bool { return true } +func (h *mockSentPacketHandler) CongestionAllowsSending() bool { return !h.congestionLimited } func (h *mockSentPacketHandler) CheckForError() error { return nil } func (h *mockSentPacketHandler) TimeOfFirstRTO() time.Time { panic("not implemented") } -func (h *mockSentPacketHandler) ProbablyHasPacketForRetransmission() bool { - return len(h.retransmissionQueue) > 0 +func (h *mockSentPacketHandler) MaybeQueueRTOs() { + h.maybeQueueRTOsCalled = true } func (h *mockSentPacketHandler) DequeuePacketForRetransmission() *ackhandlerlegacy.Packet { @@ -570,6 +572,15 @@ var _ = Describe("Session", func() { Expect(conn.written[0]).To(ContainSubstring("foobar")) Expect(conn.written[0]).To(ContainSubstring("loremipsum")) }) + + It("calls MaybeQueueRTOs even if congestion blocked, so that bytesInFlight is updated", func() { + sph := newMockSentPacketHandler() + sph.(*mockSentPacketHandler).congestionLimited = true + session.sentPacketHandler = sph + err := session.sendPacket() + Expect(err).NotTo(HaveOccurred()) + Expect(sph.(*mockSentPacketHandler).maybeQueueRTOsCalled).To(BeTrue()) + }) }) Context("scheduling sending", func() {