From 37f1a3fdda61e7759fb4c92f273e4fd7ac4808ee Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 1 Apr 2020 13:37:17 +0700 Subject: [PATCH 1/4] simplify removing of acked packets from packet history --- internal/ackhandler/sent_packet_handler.go | 47 +++++++++++----------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 3ace110c..e9a216ce 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -271,19 +271,13 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En } } + priorInFlight := h.bytesInFlight ackedPackets, err := h.determineNewlyAckedPackets(ack, encLevel) if err != nil || len(ackedPackets) == 0 { return err } - priorInFlight := h.bytesInFlight for _, p := range ackedPackets { - if p.LargestAcked != protocol.InvalidPacketNumber && encLevel == protocol.Encryption1RTT { - h.lowestNotConfirmedAcked = utils.MaxPacketNumber(h.lowestNotConfirmedAcked, p.LargestAcked+1) - } - if err := h.onPacketAcked(p); err != nil { - return err - } if p.includedInBytesInFlight { h.congestion.OnPacketAcked(p.PacketNumber, p.Length, priorInFlight, rcvTime) } @@ -352,6 +346,28 @@ func (h *sentPacketHandler) determineNewlyAckedPackets( } h.logger.Debugf("\tnewly acked packets (%d): %#x", len(pns), pns) } + + for _, p := range ackedPackets { + if packet := pnSpace.history.GetPacket(p.PacketNumber); packet == nil { + continue + } + if p.LargestAcked != protocol.InvalidPacketNumber && encLevel == protocol.Encryption1RTT { + h.lowestNotConfirmedAcked = utils.MaxPacketNumber(h.lowestNotConfirmedAcked, p.LargestAcked+1) + } + + for _, f := range p.Frames { + if f.OnAcked != nil { + f.OnAcked(f.Frame) + } + } + if p.includedInBytesInFlight { + h.bytesInFlight -= p.Length + } + if err := pnSpace.history.Remove(p.PacketNumber); err != nil { + return nil, err + } + } + return ackedPackets, err } @@ -563,23 +579,6 @@ func (h *sentPacketHandler) GetLossDetectionTimeout() time.Time { return h.alarm } -func (h *sentPacketHandler) onPacketAcked(p *Packet) error { - pnSpace := h.getPacketNumberSpace(p.EncryptionLevel) - if packet := pnSpace.history.GetPacket(p.PacketNumber); packet == nil { - return nil - } - - for _, f := range p.Frames { - if f.OnAcked != nil { - f.OnAcked(f.Frame) - } - } - if p.includedInBytesInFlight { - h.bytesInFlight -= p.Length - } - return pnSpace.history.Remove(p.PacketNumber) -} - func (h *sentPacketHandler) PeekPacketNumber(encLevel protocol.EncryptionLevel) (protocol.PacketNumber, protocol.PacketNumberLen) { pnSpace := h.getPacketNumberSpace(encLevel) From e2e3e10a63e67505df9f438ea81a41b7a952aeb1 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 1 Apr 2020 13:39:55 +0700 Subject: [PATCH 2/4] rename methods in sentPacketHandler to match the draft --- internal/ackhandler/sent_packet_handler.go | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index e9a216ce..f5d9b628 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -272,7 +272,7 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En } priorInFlight := h.bytesInFlight - ackedPackets, err := h.determineNewlyAckedPackets(ack, encLevel) + ackedPackets, err := h.detectAndRemoveAckedPackets(ack, encLevel) if err != nil || len(ackedPackets) == 0 { return err } @@ -283,7 +283,7 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En } } - if err := h.detectLostPackets(rcvTime, encLevel, priorInFlight); err != nil { + if err := h.detectAndRemoveLostPackets(rcvTime, encLevel, priorInFlight); err != nil { return err } @@ -301,15 +301,15 @@ func (h *sentPacketHandler) GetLowestPacketNotConfirmedAcked() protocol.PacketNu return h.lowestNotConfirmedAcked } -func (h *sentPacketHandler) determineNewlyAckedPackets( - ackFrame *wire.AckFrame, +func (h *sentPacketHandler) detectAndRemoveAckedPackets( + ack *wire.AckFrame, encLevel protocol.EncryptionLevel, ) ([]*Packet, error) { pnSpace := h.getPacketNumberSpace(encLevel) var ackedPackets []*Packet ackRangeIndex := 0 - lowestAcked := ackFrame.LowestAcked() - largestAcked := ackFrame.LargestAcked() + lowestAcked := ack.LowestAcked() + largestAcked := ack.LargestAcked() err := pnSpace.history.Iterate(func(p *Packet) (bool, error) { // Ignore packets below the lowest acked if p.PacketNumber < lowestAcked { @@ -320,12 +320,12 @@ func (h *sentPacketHandler) determineNewlyAckedPackets( return false, nil } - if ackFrame.HasMissingRanges() { - ackRange := ackFrame.AckRanges[len(ackFrame.AckRanges)-1-ackRangeIndex] + if ack.HasMissingRanges() { + ackRange := ack.AckRanges[len(ack.AckRanges)-1-ackRangeIndex] - for p.PacketNumber > ackRange.Largest && ackRangeIndex < len(ackFrame.AckRanges)-1 { + for p.PacketNumber > ackRange.Largest && ackRangeIndex < len(ack.AckRanges)-1 { ackRangeIndex++ - ackRange = ackFrame.AckRanges[len(ackFrame.AckRanges)-1-ackRangeIndex] + ackRange = ack.AckRanges[len(ack.AckRanges)-1-ackRangeIndex] } if p.PacketNumber >= ackRange.Smallest { // packet i contained in ACK range @@ -449,7 +449,7 @@ func (h *sentPacketHandler) setLossDetectionTimer() { h.alarm = sentTime.Add(h.rttStats.PTO(encLevel == protocol.Encryption1RTT) << h.ptoCount) } -func (h *sentPacketHandler) detectLostPackets( +func (h *sentPacketHandler) detectAndRemoveLostPackets( now time.Time, encLevel protocol.EncryptionLevel, priorInFlight protocol.ByteCount, @@ -549,7 +549,7 @@ func (h *sentPacketHandler) onVerifiedLossDetectionTimeout() error { h.logger.Debugf("Loss detection alarm fired in loss timer mode. Loss time: %s", earliestLossTime) } // Early retransmit or time loss detection - return h.detectLostPackets(time.Now(), encLevel, h.bytesInFlight) + return h.detectAndRemoveLostPackets(time.Now(), encLevel, h.bytesInFlight) } // PTO From 269f14d86c9682529c06c62f9e8636e6bdca3707 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 1 Apr 2020 13:41:55 +0700 Subject: [PATCH 3/4] notify the congestion controller of losses first --- internal/ackhandler/sent_packet_handler.go | 8 ++++---- internal/ackhandler/sent_packet_handler_test.go | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index f5d9b628..5a83cc95 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -277,16 +277,16 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En return err } + if err := h.detectAndRemoveLostPackets(rcvTime, encLevel, priorInFlight); err != nil { + return err + } + for _, p := range ackedPackets { if p.includedInBytesInFlight { h.congestion.OnPacketAcked(p.PacketNumber, p.Length, priorInFlight, rcvTime) } } - if err := h.detectAndRemoveLostPackets(rcvTime, encLevel, priorInFlight); err != nil { - return err - } - if h.qlogger != nil && h.ptoCount != 0 { h.qlogger.UpdatedPTOCount(0) } diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go index 53a21b66..e6f75bb3 100644 --- a/internal/ackhandler/sent_packet_handler_test.go +++ b/internal/ackhandler/sent_packet_handler_test.go @@ -460,8 +460,8 @@ var _ = Describe("SentPacketHandler", func() { // lose packet 1 gomock.InOrder( cong.EXPECT().MaybeExitSlowStart(), - cong.EXPECT().OnPacketAcked(protocol.PacketNumber(2), protocol.ByteCount(1), protocol.ByteCount(2), gomock.Any()), cong.EXPECT().OnPacketLost(protocol.PacketNumber(1), protocol.ByteCount(1), protocol.ByteCount(2)), + cong.EXPECT().OnPacketAcked(protocol.PacketNumber(2), protocol.ByteCount(1), protocol.ByteCount(2), gomock.Any()), ) ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 2, Largest: 2}}} Expect(handler.ReceivedAck(ack, protocol.Encryption1RTT, time.Now())).To(Succeed()) @@ -480,16 +480,16 @@ var _ = Describe("SentPacketHandler", func() { // receive the first ACK gomock.InOrder( cong.EXPECT().MaybeExitSlowStart(), - cong.EXPECT().OnPacketAcked(protocol.PacketNumber(2), protocol.ByteCount(1), protocol.ByteCount(4), gomock.Any()), cong.EXPECT().OnPacketLost(protocol.PacketNumber(1), protocol.ByteCount(1), protocol.ByteCount(4)), + cong.EXPECT().OnPacketAcked(protocol.PacketNumber(2), protocol.ByteCount(1), protocol.ByteCount(4), gomock.Any()), ) ack := &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 2, Largest: 2}}} Expect(handler.ReceivedAck(ack, protocol.Encryption1RTT, time.Now().Add(-30*time.Minute))).To(Succeed()) // receive the second ACK gomock.InOrder( cong.EXPECT().MaybeExitSlowStart(), - cong.EXPECT().OnPacketAcked(protocol.PacketNumber(4), protocol.ByteCount(1), protocol.ByteCount(2), gomock.Any()), cong.EXPECT().OnPacketLost(protocol.PacketNumber(3), protocol.ByteCount(1), protocol.ByteCount(2)), + cong.EXPECT().OnPacketAcked(protocol.PacketNumber(4), protocol.ByteCount(1), protocol.ByteCount(2), gomock.Any()), ) ack = &wire.AckFrame{AckRanges: []wire.AckRange{{Smallest: 4, Largest: 4}}} Expect(handler.ReceivedAck(ack, protocol.Encryption1RTT, time.Now())).To(Succeed()) From 5ce1eb60132bd09119942fc95c4fe43542e4465f Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 1 Apr 2020 18:44:52 +0700 Subject: [PATCH 4/4] move OnPacketLost out of detectAndRemoveLosPackets --- internal/ackhandler/sent_packet_handler.go | 31 +++++++++++----------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go index 5a83cc95..ee193d90 100644 --- a/internal/ackhandler/sent_packet_handler.go +++ b/internal/ackhandler/sent_packet_handler.go @@ -276,11 +276,13 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En if err != nil || len(ackedPackets) == 0 { return err } - - if err := h.detectAndRemoveLostPackets(rcvTime, encLevel, priorInFlight); err != nil { + lostPackets, err := h.detectAndRemoveLostPackets(rcvTime, encLevel) + if err != nil { return err } - + for _, p := range lostPackets { + h.congestion.OnPacketLost(p.PacketNumber, p.Length, priorInFlight) + } for _, p := range ackedPackets { if p.includedInBytesInFlight { h.congestion.OnPacketAcked(p.PacketNumber, p.Length, priorInFlight, rcvTime) @@ -301,10 +303,7 @@ func (h *sentPacketHandler) GetLowestPacketNotConfirmedAcked() protocol.PacketNu return h.lowestNotConfirmedAcked } -func (h *sentPacketHandler) detectAndRemoveAckedPackets( - ack *wire.AckFrame, - encLevel protocol.EncryptionLevel, -) ([]*Packet, error) { +func (h *sentPacketHandler) detectAndRemoveAckedPackets(ack *wire.AckFrame, encLevel protocol.EncryptionLevel) ([]*Packet, error) { pnSpace := h.getPacketNumberSpace(encLevel) var ackedPackets []*Packet ackRangeIndex := 0 @@ -449,11 +448,7 @@ func (h *sentPacketHandler) setLossDetectionTimer() { h.alarm = sentTime.Add(h.rttStats.PTO(encLevel == protocol.Encryption1RTT) << h.ptoCount) } -func (h *sentPacketHandler) detectAndRemoveLostPackets( - now time.Time, - encLevel protocol.EncryptionLevel, - priorInFlight protocol.ByteCount, -) error { +func (h *sentPacketHandler) detectAndRemoveLostPackets(now time.Time, encLevel protocol.EncryptionLevel) ([]*Packet, error) { pnSpace := h.getPacketNumberSpace(encLevel) pnSpace.lossTime = time.Time{} @@ -506,7 +501,6 @@ func (h *sentPacketHandler) detectAndRemoveLostPackets( // the bytes in flight need to be reduced no matter if this packet will be retransmitted if p.includedInBytesInFlight { h.bytesInFlight -= p.Length - h.congestion.OnPacketLost(p.PacketNumber, p.Length, priorInFlight) } pnSpace.history.Remove(p.PacketNumber) if h.traceCallback != nil { @@ -525,7 +519,7 @@ func (h *sentPacketHandler) detectAndRemoveLostPackets( }) } } - return nil + return lostPackets, nil } func (h *sentPacketHandler) OnLossDetectionTimeout() error { @@ -549,7 +543,14 @@ func (h *sentPacketHandler) onVerifiedLossDetectionTimeout() error { h.logger.Debugf("Loss detection alarm fired in loss timer mode. Loss time: %s", earliestLossTime) } // Early retransmit or time loss detection - return h.detectAndRemoveLostPackets(time.Now(), encLevel, h.bytesInFlight) + priorInFlight := h.bytesInFlight + lostPackets, err := h.detectAndRemoveLostPackets(time.Now(), encLevel) + if err != nil { + return err + } + for _, p := range lostPackets { + h.congestion.OnPacketLost(p.PacketNumber, p.Length, priorInFlight) + } } // PTO