ackhandler: remove unneeded error return from packet history iterator (#4917)

No functional change expected.

internal/ackhandler/sent_packet_handler.go

@@ -174,9 +174,9 @@ func (h *sentPacketHandler) DropPackets(encLevel protocol.EncryptionLevel, now t
 		if pnSpace == nil {
 			return
 		}
-		pnSpace.history.Iterate(func(p *packet) (bool, error) {
+		pnSpace.history.Iterate(func(p *packet) bool {
 			h.removeFromBytesInFlight(p)
-			return true, nil
+			return true
 		})
 	}
 	// drop the packet history
@@ -194,13 +194,13 @@ func (h *sentPacketHandler) DropPackets(encLevel protocol.EncryptionLevel, now t
 		// and not when the client drops 0-RTT keys when the handshake completes.
 		// When 0-RTT is rejected, all application data sent so far becomes invalid.
 		// Delete the packets from the history and remove them from bytes_in_flight.
-		h.appDataPackets.history.Iterate(func(p *packet) (bool, error) {
+		h.appDataPackets.history.Iterate(func(p *packet) bool {
 			if p.EncryptionLevel != protocol.Encryption0RTT && !p.skippedPacket {
-				return false, nil
+				return false
 			}
 			h.removeFromBytesInFlight(p)
 			h.appDataPackets.history.Remove(p.PacketNumber)
-			return true, nil
+			return true
 		})
 	default:
 		panic(fmt.Sprintf("Cannot drop keys for encryption level %s", encLevel))
@@ -365,9 +365,7 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En
 
 	pnSpace.largestAcked = max(pnSpace.largestAcked, largestAcked)
 
-	if err := h.detectLostPackets(rcvTime, encLevel); err != nil {
+	h.detectLostPackets(rcvTime, encLevel)
-		return false, err
-	}
 	var acked1RTTPacket bool
 	for _, p := range ackedPackets {
 		if p.includedInBytesInFlight && !p.declaredLost {
@@ -411,14 +409,15 @@ func (h *sentPacketHandler) detectAndRemoveAckedPackets(ack *wire.AckFrame, encL
 	ackRangeIndex := 0
 	lowestAcked := ack.LowestAcked()
 	largestAcked := ack.LargestAcked()
-	err := pnSpace.history.Iterate(func(p *packet) (bool, error) {
+	var processErr error
+	pnSpace.history.Iterate(func(p *packet) bool {
 		// Ignore packets below the lowest acked
 		if p.PacketNumber < lowestAcked {
-			return true, nil
+			return true
 		}
 		// Break after largest acked is reached
 		if p.PacketNumber > largestAcked {
-			return false, nil
+			return false
 		}
 
 		if ack.HasMissingRanges() {
@@ -430,20 +429,22 @@ func (h *sentPacketHandler) detectAndRemoveAckedPackets(ack *wire.AckFrame, encL
 			}
 
 			if p.PacketNumber < ackRange.Smallest { // packet not contained in ACK range
-				return true, nil
+				return true
 			}
 			if p.PacketNumber > ackRange.Largest {
-				return false, fmt.Errorf("BUG: ackhandler would have acked wrong packet %d, while evaluating range %d -> %d", p.PacketNumber, ackRange.Smallest, ackRange.Largest)
+				processErr = fmt.Errorf("BUG: ackhandler would have acked wrong packet %d, while evaluating range %d -> %d", p.PacketNumber, ackRange.Smallest, ackRange.Largest)
+				return false
 			}
 		}
 		if p.skippedPacket {
-			return false, &qerr.TransportError{
+			processErr = &qerr.TransportError{
 				ErrorCode:    qerr.ProtocolViolation,
 				ErrorMessage: fmt.Sprintf("received an ACK for skipped packet number: %d (%s)", p.PacketNumber, encLevel),
 			}
+			return false
 		}
 		h.ackedPackets = append(h.ackedPackets, p)
-		return true, nil
+		return true
 	})
 	if h.logger.Debug() && len(h.ackedPackets) > 0 {
 		pns := make([]protocol.PacketNumber, len(h.ackedPackets))
@@ -452,6 +453,9 @@ func (h *sentPacketHandler) detectAndRemoveAckedPackets(ack *wire.AckFrame, encL
 		}
 		h.logger.Debugf("\tnewly acked packets (%d): %d", len(pns), pns)
 	}
+	if processErr != nil {
+		return nil, processErr
+	}
 
 	for _, p := range h.ackedPackets {
 		if p.LargestAcked != protocol.InvalidPacketNumber && encLevel == protocol.Encryption1RTT {
@@ -475,8 +479,7 @@ func (h *sentPacketHandler) detectAndRemoveAckedPackets(ack *wire.AckFrame, encL
 			h.tracer.AcknowledgedPacket(encLevel, p.PacketNumber)
 		}
 	}
-
+	return h.ackedPackets, nil
-	return h.ackedPackets, err
 }
 
 func (h *sentPacketHandler) getLossTimeAndSpace() (time.Time, protocol.EncryptionLevel) {
@@ -604,7 +607,7 @@ func (h *sentPacketHandler) lossDetectionTime(now time.Time) alarmTimer {
 	}
 }
 
-func (h *sentPacketHandler) detectLostPackets(now time.Time, encLevel protocol.EncryptionLevel) error {
+func (h *sentPacketHandler) detectLostPackets(now time.Time, encLevel protocol.EncryptionLevel) {
 	pnSpace := h.getPacketNumberSpace(encLevel)
 	pnSpace.lossTime = time.Time{}
 
@@ -618,9 +621,9 @@ func (h *sentPacketHandler) detectLostPackets(now time.Time, encLevel protocol.E
 	lostSendTime := now.Add(-lossDelay)
 
 	priorInFlight := h.bytesInFlight
-	return pnSpace.history.Iterate(func(p *packet) (bool, error) {
+	pnSpace.history.Iterate(func(p *packet) bool {
 		if p.PacketNumber > pnSpace.largestAcked {
-			return false, nil
+			return false
 		}
 
 		var packetLost bool
@@ -666,7 +669,7 @@ func (h *sentPacketHandler) detectLostPackets(now time.Time, encLevel protocol.E
 				}
 			}
 		}
-		return true, nil
+		return true
 	})
 }
 
@@ -681,7 +684,8 @@ func (h *sentPacketHandler) OnLossDetectionTimeout(now time.Time) error {
 			h.tracer.LossTimerExpired(logging.TimerTypeACK, encLevel)
 		}
 		// Early retransmit or time loss detection
-		return h.detectLostPackets(now, encLevel)
+		h.detectLostPackets(now, encLevel)
+		return nil
 	}
 
 	// PTO
@@ -868,23 +872,23 @@ func (h *sentPacketHandler) queueFramesForRetransmission(p *packet) {
 func (h *sentPacketHandler) ResetForRetry(now time.Time) {
 	h.bytesInFlight = 0
 	var firstPacketSendTime time.Time
-	h.initialPackets.history.Iterate(func(p *packet) (bool, error) {
+	h.initialPackets.history.Iterate(func(p *packet) bool {
 		if firstPacketSendTime.IsZero() {
 			firstPacketSendTime = p.SendTime
 		}
 		if p.declaredLost || p.skippedPacket {
-			return true, nil
+			return true
 		}
 		h.queueFramesForRetransmission(p)
-		return true, nil
+		return true
 	})
 	// All application data packets sent at this point are 0-RTT packets.
 	// In the case of a Retry, we can assume that the server dropped all of them.
-	h.appDataPackets.history.Iterate(func(p *packet) (bool, error) {
+	h.appDataPackets.history.Iterate(func(p *packet) bool {
 		if !p.declaredLost && !p.skippedPacket {
 			h.queueFramesForRetransmission(p)
 		}
-		return true, nil
+		return true
 	})
 
 	// Only use the Retry to estimate the RTT if we didn't send any retransmission for the Initial.

internal/ackhandler/sent_packet_history.go

@@ -61,20 +61,16 @@ func (h *sentPacketHistory) SentAckElicitingPacket(p *packet) {
 }
 
 // Iterate iterates through all packets.
-func (h *sentPacketHistory) Iterate(cb func(*packet) (cont bool, err error)) error {
+func (h *sentPacketHistory) Iterate(cb func(*packet) (cont bool)) {
 	for _, p := range h.packets {
 		if p == nil {
 			continue
 		}
-		cont, err := cb(p)
+		cont := cb(p)
-		if err != nil {
-			return err
-		}
 		if !cont {
-			return nil
+			return
 		}
 	}
-	return nil
 }
 
 // FirstOutstanding returns the first outstanding packet.

internal/ackhandler/sent_packet_history_test.go

@@ -1,7 +1,6 @@
 package ackhandler
 
 import (
-	"errors"
 	"testing"
 	"time"
 
@@ -150,14 +149,14 @@ func TestSentPacketHistoryIterating(t *testing.T) {
 	require.NoError(t, hist.Remove(4))
 
 	var packets, skippedPackets []protocol.PacketNumber
-	require.NoError(t, hist.Iterate(func(p *packet) (bool, error) {
+	hist.Iterate(func(p *packet) bool {
 		if p.skippedPacket {
 			skippedPackets = append(skippedPackets, p.PacketNumber)
 		} else {
 			packets = append(packets, p.PacketNumber)
 		}
-		return true, nil
+		return true
-	}))
+	})
 
 	require.Equal(t, []protocol.PacketNumber{1, 2, 6}, packets)
 	require.Equal(t, []protocol.PacketNumber{0, 5}, skippedPackets)
@@ -170,34 +169,16 @@ func TestSentPacketHistoryStopIterating(t *testing.T) {
 	hist.SentAckElicitingPacket(&packet{PacketNumber: 2})
 
 	var iterations []protocol.PacketNumber
-	require.NoError(t, hist.Iterate(func(p *packet) (bool, error) {
+	hist.Iterate(func(p *packet) bool {
 		if p.skippedPacket {
-			return true, nil
+			return true
 		}
 		iterations = append(iterations, p.PacketNumber)
-		return p.PacketNumber < 1, nil
+		return p.PacketNumber < 1
-	}))
+	})
 	require.Equal(t, []protocol.PacketNumber{1}, iterations)
 }
 
-func TestSentPacketHistoryIterateError(t *testing.T) {
-	hist := newSentPacketHistory(true)
-	hist.SentAckElicitingPacket(&packet{PacketNumber: 0})
-	hist.SentAckElicitingPacket(&packet{PacketNumber: 1})
-	hist.SentAckElicitingPacket(&packet{PacketNumber: 2})
-
-	testErr := errors.New("test error")
-	var iterations []protocol.PacketNumber
-	require.Equal(t, testErr, hist.Iterate(func(p *packet) (bool, error) {
-		iterations = append(iterations, p.PacketNumber)
-		if p.PacketNumber == 1 {
-			return false, testErr
-		}
-		return true, nil
-	}))
-	require.Equal(t, []protocol.PacketNumber{0, 1}, iterations)
-}
-
 func TestSentPacketHistoryDeleteWhileIterating(t *testing.T) {
 	hist := newSentPacketHistory(true)
 	hist.SentAckElicitingPacket(&packet{PacketNumber: 0})
@@ -208,7 +189,7 @@ func TestSentPacketHistoryDeleteWhileIterating(t *testing.T) {
 	hist.SentAckElicitingPacket(&packet{PacketNumber: 5})
 
 	var iterations []protocol.PacketNumber
-	require.NoError(t, hist.Iterate(func(p *packet) (bool, error) {
+	hist.Iterate(func(p *packet) bool {
 		iterations = append(iterations, p.PacketNumber)
 		switch p.PacketNumber {
 		case 0:
@@ -216,8 +197,8 @@ func TestSentPacketHistoryDeleteWhileIterating(t *testing.T) {
 		case 4:
 			require.NoError(t, hist.Remove(4))
 		}
-		return true, nil
+		return true
-	}))
+	})
 
 	require.Equal(t, []protocol.PacketNumber{0, 1, 2, 3, 4, 5}, iterations)
 	require.Equal(t, []protocol.PacketNumber{1, 3, 5}, hist.getPacketNumbers())