Merge pull request #694 from lucas-clemente/fix-576

Send ACKs and SWFs even if we are congestion limited

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 {

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},
+			}))
+		})
+	})
 })

session.go

@@ -562,17 +562,30 @@ 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)
+			// 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
@@ -646,6 +659,7 @@ func (s *session) sendPacket() error {
 		for _, f := range windowUpdateFrames {
 			s.packer.QueueControlFrameForNextPacket(f)
 		}
+		windowUpdateFrames = nil
 
 		err = s.sendPackedPacket(packet)
 		if err != nil {

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())
@@ -1069,14 +1080,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,