From 2c2b758dee96d8a2e2e372788bd4aa0890cd13e8 Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Sat, 28 Nov 2020 12:06:30 +0700
Subject: [PATCH] allow sending of ACKs when pacing limited

An endpoint that is only receiving data won't have an accurate estimate
of the congestion window, and therefore derive a very low pacing
frequency.
In this situation it still needs to be able to send frequent ACKs to the
peer in order to allow full utilization of the bandwidth. We therefore
need to allow ACKs even when pacing-limited.
---
 session.go      | 25 ++++++++++++++++---------
 session_test.go | 21 ++++++++++++++++++---
 2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/session.go b/session.go
index ddcd702e..d035c1c5 100644
--- a/session.go
+++ b/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
diff --git a/session_test.go b/session_test.go
index 3cb6eaf5..23b5377c 100644
--- a/session_test.go
+++ b/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() {