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() {