From 1d7cf74e485beeb6abf6b4a232ef3458042c0192 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Thu, 18 Aug 2016 15:02:18 +0700 Subject: [PATCH] always send a StopWaiting with a packet containing a retransmission fixes #259 --- session.go | 6 +++--- session_test.go | 47 +++++++++++++++++++++++++++++++++++++++---- stream_framer.go | 4 ++++ stream_framer_test.go | 6 ++++++ 4 files changed, 56 insertions(+), 7 deletions(-) diff --git a/session.go b/session.go index 2e1b6a3f..c9737f4b 100644 --- a/session.go +++ b/session.go @@ -448,7 +448,6 @@ func (s *Session) sendPacket() error { } var controlFrames []frames.Frame - var hasRetransmission bool // check for retransmissions first for { @@ -456,7 +455,6 @@ func (s *Session) sendPacket() error { if retransmitPacket == nil { break } - hasRetransmission = true utils.Debugf("\tDequeueing retransmission for packet 0x%x", retransmitPacket.PacketNumber) if s.version <= protocol.Version33 { @@ -489,12 +487,14 @@ func (s *Session) sendPacket() error { // Check whether we are allowed to send a packet containing only an ACK maySendOnlyAck := time.Now().Sub(s.delayedAckOriginTime) > protocol.AckSendDelay + hasRetransmission := s.streamFramer.HasFramesForRetransmission() + var stopWaitingFrame *frames.StopWaitingFrame if s.version <= protocol.Version33 { stopWaitingFrame = s.stopWaitingManager.GetStopWaitingFrame() } else { if ack != nil || hasRetransmission { - stopWaitingFrame = s.sentPacketHandler.GetStopWaitingFrame(false) + stopWaitingFrame = s.sentPacketHandler.GetStopWaitingFrame(hasRetransmission) } } packet, err := s.packer.PackPacket(stopWaitingFrame, controlFrames, s.sentPacketHandler.GetLeastUnacked(), maySendOnlyAck) diff --git a/session_test.go b/session_test.go index 902aa8cc..899e4d1d 100644 --- a/session_test.go +++ b/session_test.go @@ -48,12 +48,16 @@ func (m *mockUnpacker) Unpack(publicHeaderBinary []byte, hdr *PublicHeader, data type mockSentPacketHandler struct { retransmissionQueue []*ackhandlerlegacy.Packet + sentPackets []*ackhandlerlegacy.Packet congestionLimited bool maybeQueueRTOsCalled bool requestedStopWaiting bool } -func (h *mockSentPacketHandler) SentPacket(packet *ackhandlerlegacy.Packet) error { return nil } +func (h *mockSentPacketHandler) SentPacket(packet *ackhandlerlegacy.Packet) error { + h.sentPackets = append(h.sentPackets, packet) + return nil +} func (h *mockSentPacketHandler) ReceivedAck(ackFrame *frames.AckFrame, withPacketNumber protocol.PacketNumber) error { return nil } @@ -61,7 +65,7 @@ func (h *mockSentPacketHandler) BytesInFlight() protocol.ByteCount { return func (h *mockSentPacketHandler) GetLeastUnacked() protocol.PacketNumber { return 1 } func (h *mockSentPacketHandler) GetStopWaitingFrame(force bool) *frames.StopWaitingFrame { h.requestedStopWaiting = true - return nil + return &frames.StopWaitingFrame{LeastUnacked: 0x1337} } func (h *mockSentPacketHandler) CongestionAllowsSending() bool { return !h.congestionLimited } func (h *mockSentPacketHandler) CheckForError() error { return nil } @@ -529,8 +533,12 @@ var _ = Describe("Session", func() { Context("retransmissions", func() { It("sends a StreamFrame from a packet queued for retransmission", func() { - // for QUIC 33, a StopWaitingFrame is added, so make sure the packet number of the new package is higher than the packet number of the retransmitted packet + // a StopWaitingFrame is added, so make sure the packet number of the new package is higher than the packet number of the retransmitted packet session.packer.lastPacketNumber = 0x1337 + 10 + if session.version > protocol.Version33 { + session.packer.packetNumberGenerator.next = 0x1337 + 9 + } + f := frames.StreamFrame{ StreamID: 0x5, Data: []byte("foobar1234567"), @@ -553,8 +561,12 @@ var _ = Describe("Session", func() { }) It("sends a StreamFrame from a packet queued for retransmission", func() { - // for QUIC 33, a StopWaitingFrame is added, so make sure the packet number of the new package is higher than the packet number of the retransmitted packet + // a StopWaitingFrame is added, so make sure the packet number of the new package is higher than the packet number of the retransmitted packet session.packer.lastPacketNumber = 0x1337 + 10 + if session.version > protocol.Version33 { + session.packer.packetNumberGenerator.next = 0x1337 + 9 + } + f1 := frames.StreamFrame{ StreamID: 0x5, Data: []byte("foobar"), @@ -582,6 +594,33 @@ var _ = Describe("Session", func() { Expect(conn.written[0]).To(ContainSubstring("loremipsum")) }) + // this test is not necessary before QUIC 34, since the legacy StopWaitingManager repeats StopWaitingFrames with every packet until the client ACKs the receipt of any of them + if version > protocol.Version33 { + It("always attaches a StopWaiting to a packet that contains a retransmission", func() { + // make sure the packet number of the new package is higher than the packet number of the retransmitted packet + session.packer.packetNumberGenerator.next = 0x1337 + 9 + + f := &frames.StreamFrame{ + StreamID: 0x5, + Data: bytes.Repeat([]byte{'f'}, int(1.5*float32(protocol.MaxPacketSize))), + } + session.streamFramer.AddFrameForRetransmission(f) + + sph := newMockSentPacketHandler() + session.sentPacketHandler = sph + + err := session.sendPacket() + Expect(err).NotTo(HaveOccurred()) + Expect(conn.written).To(HaveLen(2)) + sentPackets := sph.(*mockSentPacketHandler).sentPackets + Expect(sentPackets).To(HaveLen(2)) + _, ok := sentPackets[0].Frames[0].(*frames.StopWaitingFrame) + Expect(ok).To(BeTrue()) + _, ok = sentPackets[1].Frames[0].(*frames.StopWaitingFrame) + Expect(ok).To(BeTrue()) + }) + } + It("calls MaybeQueueRTOs even if congestion blocked, so that bytesInFlight is updated", func() { sph := newMockSentPacketHandler() sph.(*mockSentPacketHandler).congestionLimited = true diff --git a/stream_framer.go b/stream_framer.go index 29cdf867..0900660c 100644 --- a/stream_framer.go +++ b/stream_framer.go @@ -44,6 +44,10 @@ func (f *streamFramer) PopBlockedFrame() *frames.BlockedFrame { return frame } +func (f *streamFramer) HasFramesForRetransmission() bool { + return len(f.retransmissionQueue) > 0 +} + func (f *streamFramer) maybePopFramesForRetransmission(maxLen protocol.ByteCount) (res []*frames.StreamFrame, currentLen protocol.ByteCount) { for len(f.retransmissionQueue) > 0 { frame := f.retransmissionQueue[0] diff --git a/stream_framer_test.go b/stream_framer_test.go index c6957e1e..3240da4d 100644 --- a/stream_framer_test.go +++ b/stream_framer_test.go @@ -44,6 +44,12 @@ var _ = Describe("Stream Framer", func() { framer = newStreamFramer(streamsMap, fcm) }) + It("says if it has retransmissions", func() { + Expect(framer.HasFramesForRetransmission()).To(BeFalse()) + framer.AddFrameForRetransmission(retransmittedFrame1) + Expect(framer.HasFramesForRetransmission()).To(BeTrue()) + }) + It("sets the DataLenPresent for dequeued retransmitted frames", func() { framer.AddFrameForRetransmission(retransmittedFrame1) fs := framer.PopStreamFrames(protocol.MaxByteCount)