quic: send occasional ack-eliciting packets
A receiver that is sending only non-ack-eliciting packets
(for example, a connection reading data from a stream but not sending
anything other than ACKs in response) can accumulate a large amount
of state for in-flight, unacknowledged packets.
Add an occasional PING frame when in this state, to cause the peer
to send an ACK for our outstanding packets.
Change-Id: Iaf6b5a9735fa356fdebaff24200420a280b0c9a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/545215
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/internal/quic/conn_send.go b/internal/quic/conn_send.go
index e2240f2..a8d9308 100644
--- a/internal/quic/conn_send.go
+++ b/internal/quic/conn_send.go
@@ -222,11 +222,7 @@
// Either we are willing to send an ACK-only packet,
// or we've added additional frames.
c.acks[space].sentAck()
- if !c.w.sent.ackEliciting && c.keysAppData.needAckEliciting() {
- // The peer has initiated a key update.
- // We haven't sent them any packets yet in the new phase.
- // Make this an ack-eliciting packet.
- // Their ack of this packet will complete the key update.
+ if !c.w.sent.ackEliciting && c.shouldMakePacketAckEliciting() {
c.w.appendPingFrame()
}
}()
@@ -331,6 +327,30 @@
}
}
+// shouldMakePacketAckEliciting is called when sending a packet containing nothing but an ACK frame.
+// It reports whether we should add a PING frame to the packet to make it ack-eliciting.
+func (c *Conn) shouldMakePacketAckEliciting() bool {
+ if c.keysAppData.needAckEliciting() {
+ // The peer has initiated a key update.
+ // We haven't sent them any packets yet in the new phase.
+ // Make this an ack-eliciting packet.
+ // Their ack of this packet will complete the key update.
+ return true
+ }
+ if c.loss.consecutiveNonAckElicitingPackets >= 19 {
+ // We've sent a run of non-ack-eliciting packets.
+ // Add in an ack-eliciting one every once in a while so the peer
+ // lets us know which ones have arrived.
+ //
+ // Google QUICHE injects a PING after sending 19 packets. We do the same.
+ //
+ // https://www.rfc-editor.org/rfc/rfc9000#section-13.2.4-2
+ return true
+ }
+ // TODO: Consider making every packet sent when in PTO ack-eliciting to speed up recovery.
+ return false
+}
+
func (c *Conn) appendAckFrame(now time.Time, space numberSpace) bool {
seen, delay := c.acks[space].acksToSend(now)
if len(seen) == 0 {
diff --git a/internal/quic/conn_send_test.go b/internal/quic/conn_send_test.go
new file mode 100644
index 0000000..822783c
--- /dev/null
+++ b/internal/quic/conn_send_test.go
@@ -0,0 +1,40 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build go1.21
+
+package quic
+
+import (
+ "testing"
+ "time"
+)
+
+func TestAckElicitingAck(t *testing.T) {
+ // "A receiver that sends only non-ack-eliciting packets [...] might not receive
+ // an acknowledgment for a long period of time.
+ // [...] a receiver could send a [...] ack-eliciting frame occasionally [...]
+ // to elicit an ACK from the peer."
+ // https://www.rfc-editor.org/rfc/rfc9000#section-13.2.4-2
+ //
+ // Send a bunch of ack-eliciting packets, verify that the conn doesn't just
+ // send ACKs in response.
+ tc := newTestConn(t, clientSide, permissiveTransportParameters)
+ tc.handshake()
+ const count = 100
+ for i := 0; i < count; i++ {
+ tc.advance(1 * time.Millisecond)
+ tc.writeFrames(packetType1RTT,
+ debugFramePing{},
+ )
+ got, _ := tc.readFrame()
+ switch got.(type) {
+ case debugFrameAck:
+ continue
+ case debugFramePing:
+ return
+ }
+ }
+ t.Errorf("after sending %v PINGs, got no ack-eliciting response", count)
+}
diff --git a/internal/quic/loss.go b/internal/quic/loss.go
index 4a0767b..a59081f 100644
--- a/internal/quic/loss.go
+++ b/internal/quic/loss.go
@@ -50,6 +50,9 @@
// https://www.rfc-editor.org/rfc/rfc9000#section-8-2
antiAmplificationLimit int
+ // Count of non-ack-eliciting packets (ACKs) sent since the last ack-eliciting one.
+ consecutiveNonAckElicitingPackets int
+
rtt rttState
pacer pacerState
cc *ccReno
@@ -192,6 +195,11 @@
}
c.scheduleTimer(now)
}
+ if sent.ackEliciting {
+ c.consecutiveNonAckElicitingPackets = 0
+ } else {
+ c.consecutiveNonAckElicitingPackets++
+ }
}
// datagramReceived records a datagram (not packet!) received from the peer.
