Merge pull request #3010 from lucas-clemente/allow-acks-when-pacing-limited

allow sending of ACKs when pacing limited

Author: marten-seemann

session.go

@@ -1513,7 +1513,22 @@ func (s *session) sendPackets() error {
 
 	var sentPacket bool // only used in for packets sent in send mode SendAny
 	for {
-		switch sendMode := s.sentPacketHandler.SendMode(); sendMode {
+		sendMode := s.sentPacketHandler.SendMode()
+		if sendMode == ackhandler.SendAny && s.handshakeComplete && !s.sentPacketHandler.HasPacingBudget() {
+			deadline := s.sentPacketHandler.TimeUntilSend()
+			if deadline.IsZero() {
+				deadline = deadlineSendImmediately
+			}
+			s.pacingDeadline = deadline
+			// Allow sending of an ACK if we're pacing limit (if we haven't sent out a packet yet).
+			// This makes sure that a peer that is mostly receiving data (and thus has an inaccurate cwnd estimate)
+			// sends enough ACKs to allow its peer to utilize the bandwidth.
+			if sentPacket {
+				return nil
+			}
+			sendMode = ackhandler.SendAck
+		}
+		switch sendMode {
 		case ackhandler.SendNone:
 			return nil
 		case ackhandler.SendAck:
@@ -1540,14 +1555,6 @@ func (s *session) sendPackets() error {
 				return err
 			}
 		case ackhandler.SendAny:
-			if s.handshakeComplete && !s.sentPacketHandler.HasPacingBudget() {
-				deadline := s.sentPacketHandler.TimeUntilSend()
-				if deadline.IsZero() {
-					deadline = deadlineSendImmediately
-				}
-				s.pacingDeadline = deadline
-				return nil
-			}
 			sent, err := s.sendPacket()
 			if err != nil || !sent {
 				return err

session_test.go

@@ -1446,10 +1446,8 @@ var _ = Describe("Session", func() {
 
 		It("sends multiple packets, when the pacer allows immediate sending", func() {
 			sph.EXPECT().SentPacket(gomock.Any())
-			sph.EXPECT().HasPacingBudget()
 			sph.EXPECT().HasPacingBudget().Return(true).AnyTimes()
-			sph.EXPECT().TimeUntilSend() // return the zero value of time.Time{}
-			sph.EXPECT().SendMode().Return(ackhandler.SendAny).Times(3)
+			sph.EXPECT().SendMode().Return(ackhandler.SendAny).Times(2)
 			packer.EXPECT().PackPacket().Return(getPacket(10), nil)
 			packer.EXPECT().PackPacket().Return(nil, nil)
 			sender.EXPECT().WouldBlock().AnyTimes()
@@ -1463,6 +1461,23 @@ var _ = Describe("Session", func() {
 			time.Sleep(50 * time.Millisecond) // make sure that only 1 packet is sent
 		})
 
+		It("allows an ACK to be sent when pacing limited", func() {
+			sph.EXPECT().SentPacket(gomock.Any())
+			sph.EXPECT().HasPacingBudget()
+			sph.EXPECT().TimeUntilSend().Return(time.Now().Add(time.Hour))
+			sph.EXPECT().SendMode().Return(ackhandler.SendAny)
+			packer.EXPECT().MaybePackAckPacket(gomock.Any()).Return(getPacket(10), nil)
+			sender.EXPECT().WouldBlock().AnyTimes()
+			sender.EXPECT().Send(gomock.Any())
+			go func() {
+				defer GinkgoRecover()
+				cryptoSetup.EXPECT().RunHandshake().MaxTimes(1)
+				sess.run()
+			}()
+			sess.scheduleSending()
+			time.Sleep(50 * time.Millisecond) // make sure that only 1 packet is sent
+		})
+
 		// when becoming congestion limited, at some point the SendMode will change from SendAny to SendAck
 		// we shouldn't send the ACK in the same run
 		It("doesn't send an ACK right after becoming congestion limited", func() {