From fc8d937fce8314a459b394f142b4c2167f619099 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Tue, 20 Jun 2017 11:50:43 +0200 Subject: [PATCH 1/3] Move calls to GetWindowUpdate out of the send loop --- session.go | 15 ++++++++------- session_test.go | 3 +-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/session.go b/session.go index e9c3bec5..15942648 100644 --- a/session.go +++ b/session.go @@ -562,19 +562,19 @@ func (s *session) handleCloseError(closeErr closeError) error { } func (s *session) sendPacket() error { + // Get WindowUpdate frames + // this call triggers the flow controller to increase the flow control windows, if necessary + windowUpdateFrames := s.getWindowUpdateFrames() + for _, wuf := range windowUpdateFrames { + s.packer.QueueControlFrameForNextPacket(wuf) + } + // Repeatedly try sending until we don't have any more data, or run out of the congestion window for { if !s.sentPacketHandler.SendingAllowed() { return nil } - // get WindowUpdate frames - // this call triggers the flow controller to increase the flow control windows, if necessary - windowUpdateFrames := s.getWindowUpdateFrames() - for _, wuf := range windowUpdateFrames { - s.packer.QueueControlFrameForNextPacket(wuf) - } - // check for retransmissions first for { retransmitPacket := s.sentPacketHandler.DequeuePacketForRetransmission() @@ -646,6 +646,7 @@ func (s *session) sendPacket() error { for _, f := range windowUpdateFrames { s.packer.QueueControlFrameForNextPacket(f) } + windowUpdateFrames = nil err = s.sendPackedPacket(packet) if err != nil { diff --git a/session_test.go b/session_test.go index 266e3e70..681324d3 100644 --- a/session_test.go +++ b/session_test.go @@ -1069,14 +1069,13 @@ var _ = Describe("Session", func() { Expect(ok).To(BeTrue()) }) - It("retransmits a WindowUpdates if it hasn't already sent a WindowUpdate with a higher ByteOffset", func() { + It("retransmits a WindowUpdate if it hasn't already sent a WindowUpdate with a higher ByteOffset", func() { _, err := sess.GetOrOpenStream(5) Expect(err).ToNot(HaveOccurred()) fcm := mocks_fc.NewMockFlowControlManager(mockCtrl) sess.flowControlManager = fcm fcm.EXPECT().GetWindowUpdates() fcm.EXPECT().GetReceiveWindow(protocol.StreamID(5)).Return(protocol.ByteCount(0x1000), nil) - fcm.EXPECT().GetWindowUpdates() wuf := &frames.WindowUpdateFrame{ StreamID: 5, ByteOffset: 0x1000, From ff8c75a22e83ed65d2c66851242d1bb154fbb16a Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Tue, 20 Jun 2017 12:01:28 +0200 Subject: [PATCH 2/3] Send ACKs and SWFs even if we are congestion limited Fixes #576. --- packet_packer.go | 19 +++++++++++++++++++ packet_packer_test.go | 19 +++++++++++++++++++ session.go | 15 ++++++++++++++- 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/packet_packer.go b/packet_packer.go index 783c067d..da2e4932 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -64,6 +64,25 @@ func (p *packetPacker) PackConnectionClose(ccf *frames.ConnectionCloseFrame, lea }, err } +func (p *packetPacker) PackAckPacket(leastUnacked protocol.PacketNumber, ackframe *frames.AckFrame) (*packedPacket, error) { + encLevel, sealer := p.cryptoSetup.GetSealer() + ph := p.getPublicHeader(leastUnacked, encLevel) + frames := []frames.Frame{ackframe} + if p.stopWaiting != nil { + p.stopWaiting.PacketNumber = ph.PacketNumber + p.stopWaiting.PacketNumberLen = ph.PacketNumberLen + frames = append(frames, p.stopWaiting) + p.stopWaiting = nil + } + raw, err := p.writeAndSealPacket(ph, frames, sealer) + return &packedPacket{ + number: ph.PacketNumber, + raw: raw, + frames: frames, + encryptionLevel: encLevel, + }, err +} + // RetransmitNonForwardSecurePacket retransmits a handshake packet, that was sent with less than forward-secure encryption func (p *packetPacker) RetransmitNonForwardSecurePacket(packet *ackhandler.Packet) (*packedPacket, error) { if packet.EncryptionLevel == protocol.EncryptionForwardSecure { diff --git a/packet_packer_test.go b/packet_packer_test.go index a556332f..971c9afd 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -2,6 +2,7 @@ package quic import ( "bytes" + "math" "github.com/lucas-clemente/quic-go/ackhandler" "github.com/lucas-clemente/quic-go/frames" @@ -703,4 +704,22 @@ var _ = Describe("Packet packer", func() { Expect(err).To(MatchError("PacketPacker BUG: Handshake retransmissions must contain a StopWaitingFrame")) }) }) + + Context("packing ACK packets", func() { + It("packs ACK packets", func() { + p, err := packer.PackAckPacket(0, &frames.AckFrame{}) + Expect(err).NotTo(HaveOccurred()) + Expect(p.frames).To(Equal([]frames.Frame{&frames.AckFrame{DelayTime: math.MaxInt64}})) + }) + + It("packs ACK packets with SWFs", func() { + packer.QueueControlFrameForNextPacket(&frames.StopWaitingFrame{}) + p, err := packer.PackAckPacket(0, &frames.AckFrame{}) + Expect(err).NotTo(HaveOccurred()) + Expect(p.frames).To(Equal([]frames.Frame{ + &frames.AckFrame{DelayTime: math.MaxInt64}, + &frames.StopWaitingFrame{PacketNumber: 1, PacketNumberLen: 2}, + })) + }) + }) }) diff --git a/session.go b/session.go index 15942648..eda61169 100644 --- a/session.go +++ b/session.go @@ -572,7 +572,20 @@ func (s *session) sendPacket() error { // Repeatedly try sending until we don't have any more data, or run out of the congestion window for { if !s.sentPacketHandler.SendingAllowed() { - return nil + // If we aren't allowed to send, at least try sending an ACK frame + ack := s.receivedPacketHandler.GetAckFrame() + if ack == nil { + return nil + } + swf := s.sentPacketHandler.GetStopWaitingFrame(false) + if swf != nil { + s.packer.QueueControlFrameForNextPacket(swf) + } + packet, err := s.packer.PackAckPacket(s.sentPacketHandler.GetLeastUnacked(), ack) + if err != nil { + return err + } + return s.sendPackedPacket(packet) } // check for retransmissions first From 2b69cc2e3dfa9c533add039a6d8bec8cb3516250 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Wed, 21 Jun 2017 11:09:42 +0200 Subject: [PATCH 3/3] Add a session test for sending ACK-only packets --- session_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/session_test.go b/session_test.go index 681324d3..c66379e4 100644 --- a/session_test.go +++ b/session_test.go @@ -900,6 +900,17 @@ var _ = Describe("Session", func() { Expect(mconn.written[0]).To(ContainSubstring(string([]byte{0x5E, 0x03}))) }) + It("sends ACK frames when congestion limited", func() { + sess.sentPacketHandler = &mockSentPacketHandler{congestionLimited: true} + sess.packer.packetNumberGenerator.next = 0x1338 + packetNumber := protocol.PacketNumber(0x035E) + sess.receivedPacketHandler.ReceivedPacket(packetNumber, true) + err := sess.sendPacket() + Expect(err).NotTo(HaveOccurred()) + Expect(mconn.written).To(HaveLen(1)) + Expect(mconn.written[0]).To(ContainSubstring(string([]byte{0x5E, 0x03}))) + }) + It("sends two WindowUpdate frames", func() { _, err := sess.GetOrOpenStream(5) Expect(err).ToNot(HaveOccurred())