diff --git a/connection.go b/connection.go index 946d17f2..14a7e4c8 100644 --- a/connection.go +++ b/connection.go @@ -1739,7 +1739,7 @@ func (s *connection) sendPackets() error { var sentPacket bool // only used in for packets sent in send mode SendAny for { sendMode := s.sentPacketHandler.SendMode() - if sendMode == ackhandler.SendAny && !s.sentPacketHandler.HasPacingBudget() { + if sendMode == ackhandler.SendPacingLimited { deadline := s.sentPacketHandler.TimeUntilSend() if deadline.IsZero() { deadline = deadlineSendImmediately @@ -1753,6 +1753,7 @@ func (s *connection) sendPackets() error { } sendMode = ackhandler.SendAck } + //nolint:exhaustive // No need to handle pacing limited here. switch sendMode { case ackhandler.SendNone: return nil diff --git a/connection_test.go b/connection_test.go index 0a2ab929..c842e98f 100644 --- a/connection_test.go +++ b/connection_test.go @@ -604,7 +604,6 @@ var _ = Describe("Connection", func() { sph := mockackhandler.NewMockSentPacketHandler(mockCtrl) sph.EXPECT().GetLossDetectionTimeout().Return(time.Now().Add(time.Hour)).AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() // only expect a single SentPacket() call sph.EXPECT().SentPacket(gomock.Any()) tracer.EXPECT().SentShortHeaderPacket(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()) @@ -1206,7 +1205,6 @@ var _ = Describe("Connection", func() { sph.EXPECT().TimeUntilSend().AnyTimes() sph.EXPECT().GetLossDetectionTimeout().AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SentPacket(gomock.Any()) conn.sentPacketHandler = sph runConn() @@ -1254,7 +1252,6 @@ var _ = Describe("Connection", func() { sph.EXPECT().TimeUntilSend().AnyTimes() sph.EXPECT().GetLossDetectionTimeout().AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SentPacket(gomock.Any()) conn.sentPacketHandler = sph fc := mocks.NewMockConnectionFlowController(mockCtrl) @@ -1397,10 +1394,9 @@ var _ = Describe("Connection", func() { It("sends multiple packets one by one immediately", func() { sph.EXPECT().SentPacket(gomock.Any()).Times(2) - sph.EXPECT().HasPacingBudget().Return(true).Times(2) - sph.EXPECT().HasPacingBudget() + sph.EXPECT().SendMode().Return(ackhandler.SendAny).Times(2) + sph.EXPECT().SendMode().Return(ackhandler.SendPacingLimited) sph.EXPECT().TimeUntilSend().Return(time.Now().Add(time.Hour)) - sph.EXPECT().SendMode().Return(ackhandler.SendAny).Times(3) p, buffer := getShortHeaderPacket(10) packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(p, buffer, nil) p, buffer = getShortHeaderPacket(11) @@ -1418,7 +1414,6 @@ var _ = Describe("Connection", func() { It("sends multiple packets, when the pacer allows immediate sending", func() { sph.EXPECT().SentPacket(gomock.Any()) - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).Times(2) p, buffer := getShortHeaderPacket(10) packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(p, buffer, nil) @@ -1436,9 +1431,8 @@ var _ = Describe("Connection", func() { 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) + sph.EXPECT().SendMode().Return(ackhandler.SendPacingLimited) p, buffer := getShortHeaderPacket(10) packer.EXPECT().PackPacket(true, gomock.Any(), gomock.Any(), conn.version).Return(p, buffer, nil) @@ -1457,7 +1451,6 @@ var _ = Describe("Connection", func() { // we shouldn't send the ACK in the same run It("doesn't send an ACK right after becoming congestion limited", func() { sph.EXPECT().SentPacket(gomock.Any()) - sph.EXPECT().HasPacingBudget().Return(true) sph.EXPECT().SendMode().Return(ackhandler.SendAny) sph.EXPECT().SendMode().Return(ackhandler.SendAck) p, buffer := getShortHeaderPacket(100) @@ -1475,19 +1468,18 @@ var _ = Describe("Connection", func() { It("paces packets", func() { pacingDelay := scaleDuration(100 * time.Millisecond) - sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() p1, buffer1 := getShortHeaderPacket(100) p2, buffer2 := getShortHeaderPacket(101) gomock.InOrder( - sph.EXPECT().HasPacingBudget().Return(true), + sph.EXPECT().SendMode().Return(ackhandler.SendAny), packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(p1, buffer1, nil), sph.EXPECT().SentPacket(gomock.Any()), - sph.EXPECT().HasPacingBudget(), + sph.EXPECT().SendMode().Return(ackhandler.SendPacingLimited), sph.EXPECT().TimeUntilSend().Return(time.Now().Add(pacingDelay)), - sph.EXPECT().HasPacingBudget().Return(true), + sph.EXPECT().SendMode().Return(ackhandler.SendAny), packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(p2, buffer2, nil), sph.EXPECT().SentPacket(gomock.Any()), - sph.EXPECT().HasPacingBudget(), + sph.EXPECT().SendMode().Return(ackhandler.SendPacingLimited), sph.EXPECT().TimeUntilSend().Return(time.Now().Add(time.Hour)), ) written := make(chan struct{}, 2) @@ -1506,10 +1498,9 @@ var _ = Describe("Connection", func() { It("sends multiple packets at once", func() { sph.EXPECT().SentPacket(gomock.Any()).Times(3) - sph.EXPECT().HasPacingBudget().Return(true).Times(3) - sph.EXPECT().HasPacingBudget() + sph.EXPECT().SendMode().Return(ackhandler.SendAny).Times(3) + sph.EXPECT().SendMode().Return(ackhandler.SendPacingLimited) sph.EXPECT().TimeUntilSend().Return(time.Now().Add(time.Hour)) - sph.EXPECT().SendMode().Return(ackhandler.SendAny).Times(4) for pn := protocol.PacketNumber(1000); pn < 1003; pn++ { p, buffer := getShortHeaderPacket(pn) packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(p, buffer, nil) @@ -1541,7 +1532,6 @@ var _ = Describe("Connection", func() { written := make(chan struct{}) sender.EXPECT().WouldBlock().AnyTimes() sph.EXPECT().SentPacket(gomock.Any()) - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() p, buffer := getShortHeaderPacket(1000) packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(p, buffer, nil) @@ -1565,7 +1555,6 @@ var _ = Describe("Connection", func() { sph.EXPECT().ReceivedBytes(gomock.Any()) conn.handlePacket(&receivedPacket{buffer: getPacketBuffer()}) }) - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() p, buffer := getShortHeaderPacket(1000) packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(p, buffer, nil) @@ -1580,7 +1569,6 @@ var _ = Describe("Connection", func() { It("stops sending when the send queue is full", func() { sph.EXPECT().SentPacket(gomock.Any()) - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny) p, buffer := getShortHeaderPacket(1000) packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(p, buffer, nil) @@ -1601,7 +1589,6 @@ var _ = Describe("Connection", func() { // now make room in the send queue sph.EXPECT().SentPacket(gomock.Any()) - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() sender.EXPECT().WouldBlock().AnyTimes() p, buffer = getShortHeaderPacket(1001) @@ -1617,7 +1604,6 @@ var _ = Describe("Connection", func() { }) It("doesn't set a pacing timer when there is no data to send", func() { - sph.EXPECT().HasPacingBudget().Return(true) sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() sender.EXPECT().WouldBlock().AnyTimes() packer.EXPECT().PackPacket(false, gomock.Any(), gomock.Any(), conn.version).Return(shortHeaderPacket{}, nil, errNothingToPack) @@ -1636,7 +1622,6 @@ var _ = Describe("Connection", func() { conn.mtuDiscoverer = mtuDiscoverer conn.config.DisablePathMTUDiscovery = false sph.EXPECT().SentPacket(gomock.Any()) - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny) sph.EXPECT().SendMode().Return(ackhandler.SendNone) written := make(chan struct{}, 1) @@ -1688,7 +1673,7 @@ var _ = Describe("Connection", func() { sph.EXPECT().GetLossDetectionTimeout().AnyTimes() sph.EXPECT().TimeUntilSend().AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() + sph.EXPECT().SentPacket(gomock.Any()) conn.sentPacketHandler = sph p, buffer := getShortHeaderPacket(1) @@ -1717,7 +1702,6 @@ var _ = Describe("Connection", func() { sph := mockackhandler.NewMockSentPacketHandler(mockCtrl) sph.EXPECT().GetLossDetectionTimeout().AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SentPacket(gomock.Any()).Do(func(p *ackhandler.Packet) { Expect(p.PacketNumber).To(Equal(protocol.PacketNumber(1234))) }) @@ -1771,7 +1755,6 @@ var _ = Describe("Connection", func() { sph.EXPECT().GetLossDetectionTimeout().AnyTimes() sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() sph.EXPECT().TimeUntilSend().Return(time.Now()).AnyTimes() - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() gomock.InOrder( sph.EXPECT().SentPacket(gomock.Any()).Do(func(p *ackhandler.Packet) { Expect(p.EncryptionLevel).To(Equal(protocol.EncryptionInitial)) @@ -1923,7 +1906,6 @@ var _ = Describe("Connection", func() { sph.EXPECT().SendMode().Return(ackhandler.SendAny).AnyTimes() sph.EXPECT().GetLossDetectionTimeout().AnyTimes() sph.EXPECT().TimeUntilSend().AnyTimes() - sph.EXPECT().HasPacingBudget().Return(true).AnyTimes() sph.EXPECT().SetHandshakeConfirmed() sph.EXPECT().SentPacket(gomock.Any()) mconn.EXPECT().Write(gomock.Any()) diff --git a/internal/ackhandler/interfaces.go b/internal/ackhandler/interfaces.go index 3b1bf486..01ee72c1 100644 --- a/internal/ackhandler/interfaces.go +++ b/internal/ackhandler/interfaces.go @@ -24,8 +24,6 @@ type SentPacketHandler interface { // TimeUntilSend is the time when the next packet should be sent. // It is used for pacing packets. TimeUntilSend() time.Time - // HasPacingBudget says if the pacer allows sending of a (full size) packet at this moment. - HasPacingBudget() bool SetMaxDatagramSize(count protocol.ByteCount) // only to be called once the handshake is complete diff --git a/internal/ackhandler/send_mode.go b/internal/ackhandler/send_mode.go index 3d5fe560..c03f3a6f 100644 --- a/internal/ackhandler/send_mode.go +++ b/internal/ackhandler/send_mode.go @@ -16,6 +16,10 @@ const ( SendPTOHandshake // SendPTOAppData means that an Application data probe packet should be sent SendPTOAppData + // SendPacingLimited means that the pacer doesn't allow sending of a packet right now, + // but will do in a little while. + // The timestamp when sending is allowed again can be obtained via the SentPacketHandler.TimeUntilSend. + SendPacingLimited // SendAny means that any packet should be sent SendAny ) @@ -34,6 +38,8 @@ func (s SendMode) String() string { return "pto (Application Data)" case SendAny: return "any" + case SendPacingLimited: + return "pacing limited" default: return fmt.Sprintf("invalid send mode: %d", s) } diff --git a/internal/ackhandler/send_mode_test.go b/internal/ackhandler/send_mode_test.go index 78ae4d3c..6497c697 100644 --- a/internal/ackhandler/send_mode_test.go +++ b/internal/ackhandler/send_mode_test.go @@ -9,6 +9,7 @@ var _ = Describe("Send Mode", func() { It("has a string representation", func() { Expect(SendNone.String()).To(Equal("none")) Expect(SendAny.String()).To(Equal("any")) + Expect(SendPacingLimited.String()).To(Equal("pacing limited")) Expect(SendAck.String()).To(Equal("ack")) Expect(SendPTOInitial.String()).To(Equal("pto (Initial)")) Expect(SendPTOHandshake.String()).To(Equal("pto (Handshake)")) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 27f72cce..05467a04 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -756,6 +756,9 @@ func (h *sentPacketHandler) SendMode() SendMode { } return SendAck } + if !h.congestion.HasPacingBudget() { + return SendPacingLimited + } return SendAny } diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index 238d2b5d..d0385b9f 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -599,12 +599,14 @@ var _ = Describe("SentPacketHandler", func() { SendTime: time.Now(), }) cong.EXPECT().CanSend(protocol.ByteCount(42)).Return(true) + cong.EXPECT().HasPacingBudget().Return(true) handler.SendMode() }) It("allows sending of ACKs when congestion limited", func() { handler.ReceivedPacket(protocol.EncryptionHandshake) cong.EXPECT().CanSend(gomock.Any()).Return(true) + cong.EXPECT().HasPacingBudget().Return(true) Expect(handler.SendMode()).To(Equal(SendAny)) cong.EXPECT().CanSend(gomock.Any()).Return(false) Expect(handler.SendMode()).To(Equal(SendAck)) @@ -613,6 +615,7 @@ var _ = Describe("SentPacketHandler", func() { It("allows sending of ACKs when we're keeping track of MaxOutstandingSentPackets packets", func() { handler.ReceivedPacket(protocol.EncryptionHandshake) cong.EXPECT().CanSend(gomock.Any()).Return(true).AnyTimes() + cong.EXPECT().HasPacingBudget().Return(true).AnyTimes() cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() for i := protocol.PacketNumber(0); i < protocol.MaxOutstandingSentPackets; i++ { Expect(handler.SendMode()).To(Equal(SendAny)) @@ -889,6 +892,7 @@ var _ = Describe("SentPacketHandler", func() { Context("amplification limit, for the server", func() { It("limits the window to 3x the bytes received, to avoid amplification attacks", func() { + now := time.Now() handler.ReceivedPacket(protocol.EncryptionInitial) // receiving an Initial packet doesn't validate the client's address handler.ReceivedBytes(200) handler.SentPacket(&Packet{ @@ -896,7 +900,7 @@ var _ = Describe("SentPacketHandler", func() { Length: 599, EncryptionLevel: protocol.EncryptionInitial, Frames: []Frame{{Frame: &wire.PingFrame{}}}, - SendTime: time.Now(), + SendTime: now, }) Expect(handler.SendMode()).To(Equal(SendAny)) handler.SentPacket(&Packet{ @@ -904,7 +908,7 @@ var _ = Describe("SentPacketHandler", func() { Length: 1, EncryptionLevel: protocol.EncryptionInitial, Frames: []Frame{{Frame: &wire.PingFrame{}}}, - SendTime: time.Now(), + SendTime: now, }) Expect(handler.SendMode()).To(Equal(SendNone)) }) diff --git a/internal/mocks/ackhandler/sent_packet_handler.go b/internal/mocks/ackhandler/sent_packet_handler.go index 82073ed4..d538cf81 100644 --- a/internal/mocks/ackhandler/sent_packet_handler.go +++ b/internal/mocks/ackhandler/sent_packet_handler.go @@ -63,20 +63,6 @@ func (mr *MockSentPacketHandlerMockRecorder) GetLossDetectionTimeout() *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetLossDetectionTimeout", reflect.TypeOf((*MockSentPacketHandler)(nil).GetLossDetectionTimeout)) } -// HasPacingBudget mocks base method. -func (m *MockSentPacketHandler) HasPacingBudget() bool { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "HasPacingBudget") - ret0, _ := ret[0].(bool) - return ret0 -} - -// HasPacingBudget indicates an expected call of HasPacingBudget. -func (mr *MockSentPacketHandlerMockRecorder) HasPacingBudget() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasPacingBudget", reflect.TypeOf((*MockSentPacketHandler)(nil).HasPacingBudget)) -} - // OnLossDetectionTimeout mocks base method. func (m *MockSentPacketHandler) OnLossDetectionTimeout() error { m.ctrl.T.Helper()