ackhandler: refactor ACK queueing logic (#4225)

Once an ACK has been queued, there's no need to check futher conditions that
would lead to queueing of an ACK.

connection_test.go

@@ -1270,7 +1270,6 @@ var _ = Describe("Connection", func() {
 			sph.EXPECT().ECNMode(true).AnyTimes()
 			runConn()
 			packer.EXPECT().AppendPacket(gomock.Any(), gomock.Any(), conn.version).Return(shortHeaderPacket{}, errNothingToPack).AnyTimes()
-			conn.receivedPacketHandler.ReceivedPacket(0x035e, protocol.ECNNon, protocol.Encryption1RTT, time.Now(), true)
 			conn.scheduleSending()
 			time.Sleep(50 * time.Millisecond) // make sure there are no calls to mconn.Write()
 		})

internal/ackhandler/received_packet_tracker.go

@@ -56,10 +56,6 @@ func (h *receivedPacketTracker) ReceivedPacket(pn protocol.PacketNumber, ecn pro
 		h.largestObservedRcvdTime = rcvTime
 	}
 
-	if ackEliciting {
-		h.hasNewAck = true
-		h.maybeQueueACK(pn, rcvTime, ecn, isMissing)
-	}
 	//nolint:exhaustive // Only need to count ECT(0), ECT(1) and ECN-CE.
 	switch ecn {
 	case protocol.ECT0:
@@ -69,6 +65,24 @@ func (h *receivedPacketTracker) ReceivedPacket(pn protocol.PacketNumber, ecn pro
 	case protocol.ECNCE:
 		h.ecnce++
 	}
+
+	if !ackEliciting {
+		return nil
+	}
+
+	h.hasNewAck = true
+	h.ackElicitingPacketsReceivedSinceLastAck++
+	if !h.ackQueued && h.shouldQueueACK(pn, ecn, isMissing) {
+		h.ackQueued = true
+		h.ackAlarm = time.Time{} // cancel the ack alarm
+	}
+	if !h.ackQueued {
+		// No ACK queued, but we'll need to acknowledge the packet after max_ack_delay.
+		h.ackAlarm = rcvTime.Add(h.maxAckDelay)
+		if h.logger.Debug() {
+			h.logger.Debugf("\tSetting ACK timer to max ack delay: %s", h.maxAckDelay)
+		}
+	}
 	return nil
 }
 
@@ -101,62 +115,43 @@ func (h *receivedPacketTracker) hasNewMissingPackets() bool {
 	return highestRange.Smallest > h.lastAck.LargestAcked()+1 && highestRange.Len() == 1
 }
 
-// maybeQueueACK queues an ACK, if necessary.
-func (h *receivedPacketTracker) maybeQueueACK(pn protocol.PacketNumber, rcvTime time.Time, ecn protocol.ECN, wasMissing bool) {
+func (h *receivedPacketTracker) shouldQueueACK(pn protocol.PacketNumber, ecn protocol.ECN, wasMissing bool) bool {
 	// always acknowledge the first packet
 	if h.lastAck == nil {
-		if !h.ackQueued {
-			h.logger.Debugf("\tQueueing ACK because the first packet should be acknowledged.")
-		}
-		h.ackQueued = true
-		return
+		h.logger.Debugf("\tQueueing ACK because the first packet should be acknowledged.")
+		return true
 	}
 
-	if h.ackQueued {
-		return
-	}
-
-	h.ackElicitingPacketsReceivedSinceLastAck++
-
 	// Send an ACK if this packet was reported missing in an ACK sent before.
 	// Ack decimation with reordering relies on the timer to send an ACK, but if
 	// missing packets we reported in the previous ack, send an ACK immediately.
 	if wasMissing {
 		if h.logger.Debug() {
 			h.logger.Debugf("\tQueueing ACK because packet %d was missing before.", pn)
 		}
-		h.ackQueued = true
+		return true
 	}
 
 	// send an ACK every 2 ack-eliciting packets
 	if h.ackElicitingPacketsReceivedSinceLastAck >= packetsBeforeAck {
 		if h.logger.Debug() {
 			h.logger.Debugf("\tQueueing ACK because packet %d packets were received after the last ACK (using initial threshold: %d).", h.ackElicitingPacketsReceivedSinceLastAck, packetsBeforeAck)
 		}
-		h.ackQueued = true
-	} else if h.ackAlarm.IsZero() {
-		if h.logger.Debug() {
-			h.logger.Debugf("\tSetting ACK timer to max ack delay: %s", h.maxAckDelay)
-		}
-		h.ackAlarm = rcvTime.Add(h.maxAckDelay)
+		return true
 	}
 
 	// queue an ACK if there are new missing packets to report
 	if h.hasNewMissingPackets() {
 		h.logger.Debugf("\tQueuing ACK because there's a new missing packet to report.")
-		h.ackQueued = true
+		return true
 	}
 
 	// queue an ACK if the packet was ECN-CE marked
 	if ecn == protocol.ECNCE {
 		h.logger.Debugf("\tQueuing ACK because the packet was ECN-CE marked.")
-		h.ackQueued = true
-	}
-
-	if h.ackQueued {
-		// cancel the ack alarm
-		h.ackAlarm = time.Time{}
+		return true
 	}
+	return false
 }
 
 func (h *receivedPacketTracker) GetAckFrame(onlyIfQueued bool) *wire.AckFrame {