Skip to content

QUIC: bound how long a due ACK waits for a packet to carry it #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
deadcaf3 wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

QUIC: bound how long a due ACK waits for a packet to carry it #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Sources/SwiftNetwork/QUIC/Ack.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,14 @@ struct Ack: ~Copyable, PrefixedLoggable, NonCopyableTimerUser {
)
}

// How long an ACK that is due, but would be the only frame in its packet, waits for a
// packet to ride on. This is the time tolerance of the ACK policy in processPending(),
// but no shorter than the timer granularity: on a path with a negligible RTT a shorter
// wait sends an ACK for nearly every batch of packets received.
func bundlingDelay(on path: QUICPath) -> NetworkDuration {
min(maxDelay, max(path.rtt.smoothedRTT / 2, Recovery.timerGranularity))
}

mutating func scheduleDelayedAck(in eventContext: inout NetworkContext.EventContext) {
// ACK timer is already scheduled
if timerScheduled {
Expand Down
36 changes: 36 additions & 0 deletions Sources/SwiftNetwork/QUIC/QUICConnection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,9 @@ public final class QUICConnection: ManyToManyApplicationStreamProtocol,
var lastShorthandTimestamp: NetworkClock.Instant = .zero

var idleTimerID: Timer.TimerID?
// Sends an ACK that is due, but has no packet to ride on, once the bundling delay is up
var ackBundlingTimerID: Timer.TimerID?
var ackBundlingTimerScheduled = false
var logIDNumber: Int = 0
let signpostID = QUICSignpost.makeSignpostID()
var signpostConnectInterval: QUICSignpost.IntervalState?
Expand Down Expand Up @@ -509,6 +512,14 @@ public final class QUICConnection: ManyToManyApplicationStreamProtocol,
}
self.ack = Ack(connection: self, timerID: ackTimerID, logPrefixer: logPrefixer)

ackBundlingTimerID = timer.insert(
description: "ACK bundling",
timerNow: self.now,
in: &eventContext
) { _, timerState in
self.fireAckBundlingTimer(in: &timerState)
}

let recoveryTimerID = timer.insert(
description: "Recovery",
timerNow: self.now,
Expand Down Expand Up @@ -3322,6 +3333,19 @@ public final class QUICConnection: ManyToManyApplicationStreamProtocol,
}
}

func fireAckBundlingTimer(in eventContext: inout NetworkContext.EventContext) {
ackBundlingTimerScheduled = false
// A packet may have carried the ACK in the meantime
guard applicationPendingItems.isAckOnly else {
return
}
sendFrames(delayedACK: true, in: &eventContext)

// As for the delayed ACK timer, sending an ACK-only packet leaves nothing behind
// that would observe the connection going idle.
checkConnectionIdle(unackedPacketCount: ack.unackedPacketCount, in: &eventContext)
}

// Adds recovery and applicationPendingItems to avoid extra begin/end acccess checking overhead
@discardableResult
/// Sends pending frames using an event context the caller already holds.
Expand Down Expand Up @@ -3371,6 +3395,18 @@ public final class QUICConnection: ManyToManyApplicationStreamProtocol,
guard !applicationPendingItems.isAckOnly || delayedACK || ack.immediateAcks > 0 else {
// Make sure the ack-delay timer is armed if returning early
ack.scheduleDelayedAck(in: &eventContext)
// This ACK was due now, so it only waits for a packet to ride on for the bundling
// delay: a peer whose congestion window is waiting on it would otherwise stall
// for the max ACK delay.
if !ackBundlingTimerScheduled, let ackBundlingTimerID, let path = currentPath {
ackBundlingTimerScheduled = true
timer.reschedule(
identifier: ackBundlingTimerID,
fromNow: ack.bundlingDelay(on: path),
timerNow: now,
in: &eventContext
)
}

@agnosticdev agnosticdev Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we run into a situation where we have two timers potentially scheduled here now?

@deadcaf3 deadcaf3 Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think both can be armed. Whatever fires second should find nothing to do, but it's an extra wakeup. I am all for optimizations. Would it be cleaner to skip the new timer and just pull the delayed ack timer earlier when an ACK is due? Open to thinking things thru cuz there's a tiny design decision here. cc @rpaulo

@deadcaf3 deadcaf3 Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One caveat tho, the ACK_FREQUENCY extension where sender asks receiver to ACK less often to save resources exists in QUIC, although there's no handling for it in the repo apart from maybe like a few placeholders, if it's implemented, bundling delay should respect the peer's request than using half the rtt.

return false
}
guard let path = currentPath else {
Expand Down
12 changes: 10 additions & 2 deletions Sources/SwiftNetwork/QUIC/Timer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,9 @@ final class Timer: PrefixedLoggable {
/// ignored. Tightening it trades re-arms for precision and wants a benchmark behind it.
///
/// It also decides whether that coalescing is attempted at all. A deadline nearer than this
/// always re-arms, because tolerating up to a millisecond of error would dominate it: half a
/// millisecond out, a coalesced wakeup could land after the deadline had already passed.
/// re-arms whenever it moves, because tolerating up to a millisecond of error would dominate
/// it: half a millisecond out, a coalesced wakeup could land after the deadline had already
/// passed.
static let timerThreshold = NetworkDuration.milliseconds(1)

init(
Expand Down Expand Up @@ -231,6 +232,13 @@ final class Timer: PrefixedLoggable {
delta = .zero
}

// The pending wakeup is already for this deadline, so there is nothing to re-arm. The
// check below does not cover a deadline within the threshold, and while one is that
// near every reschedule of another timer would re-arm the wakeup for the same instant.
if case .armed(let nextDeadline) = wakeup, delta > .zero, nextDeadline == earliestDeadline {
return
}

// If the timer is over one millisecond in the future,
// check if it is redundant with the existing deadline (nextDeadline).
// This optimisation is only valid if there's a pending wakeup, otherwise
Expand Down
19 changes: 13 additions & 6 deletions Tests/SwiftNetworkTests/QUICTestHarness.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,8 @@ class QUICTestHarness {
streamIndex: Int,
readChunkSize: Int = .max,
timeout: TimeInterval = 5.0,
shouldBatchSends: Bool = false
shouldBatchSends: Bool = false,
shouldEchoData: Bool = true
) {
guard let state else {
XCTFail("State must be non-nil")
Expand Down Expand Up @@ -811,9 +812,11 @@ class QUICTestHarness {
}
serverReadBytes += response.count

let receivedFIN = serverStreamHarness.receivedFIN
let writeResult = serverStreamHarness.write(response, sendFIN: receivedFIN, in: &state)
XCTAssertTrue(writeResult, "Server failed send response")
if shouldEchoData {
let receivedFIN = serverStreamHarness.receivedFIN
let writeResult = serverStreamHarness.write(response, sendFIN: receivedFIN, in: &state)
XCTAssertTrue(writeResult, "Server failed send response")
}
}

if serverReadBytes >= dataGenerator.totalSize {
Expand All @@ -839,7 +842,9 @@ class QUICTestHarness {
}

wait(for: [serverReadExpectation], timeout: timeout)
wait(for: [clientReadExpectation], timeout: timeout)
if shouldEchoData {
wait(for: [clientReadExpectation], timeout: timeout)
}

// If FINs are sent, also ensure that the streams move to disconnected
if dataGenerator.sendFIN {
Expand Down Expand Up @@ -1066,6 +1071,7 @@ class QUICTestHarness {
verifyResetStreamHalfClosure: Bool = false,
shouldMarkIdle: Bool = false,
shouldBatchSends: Bool = false,
shouldEchoData: Bool = true,
clientOptions: ProtocolOptions<QUICProtocol> = QUICProtocol.options(),
serverOptions: ProtocolOptions<QUICProtocol> = QUICProtocol.options(),
sendMaxStreamUpdate: Bool = false,
Expand Down Expand Up @@ -1173,7 +1179,8 @@ class QUICTestHarness {
streamIndex: index,
readChunkSize: clientReadChunkSize,
timeout: timeout,
shouldBatchSends: shouldBatchSends
shouldBatchSends: shouldBatchSends,
shouldEchoData: shouldEchoData
)
} else {
XCTAssertTrue(dataBlock == nil && blockSize == 0 && blockCount == 0)
Expand Down
25 changes: 25 additions & 0 deletions Tests/SwiftNetworkTests/SwiftNetworkQUICHarnessTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,31 @@ final class SwiftNetworkQUICHarnessTests: NetTestCase {
)
}

// A receiver with nothing to send has no packet for its ACKs to ride on. An ACK that is
// due must still go out after the bundling delay rather than the max ACK delay, or a
// sender whose congestion window is waiting on it stalls. Stretching the max ACK delay
// makes such a stall outlast the test.
func testQUICOneWayTransferDoesNotWaitForMaxAckDelay() {
QUICTestHarness().runQUICTest(
blockSize: 10240,
blockCount: 100,
sendFIN: false,
shouldEchoData: false,
afterHandshake: { harness in
let expectation = XCTestExpectation(description: "Wait for max ACK delay to be extended")
harness.context.async {
harness.state?.serverInstance.ack.maxDelay = .seconds(30)
// Keep the client's view of the server's ACK delay consistent, otherwise
// its PTO fires while an ACK is held and the probe restarts the transfer.
harness.state?.clientInstance.currentPath?.rtt.remoteMaxAckDelay = .seconds(30)
expectation.fulfill()
}
let waitResult = XCTWaiter.wait(for: [expectation], timeout: 2.0)
XCTAssertEqual(waitResult, .completed, "Max ACK delay should be extended")
}
)
}

#if !NETWORK_PRIVATE
// Note: These tests takes too long to in for automation
// Changed to 30 seconds to give it leeway to run locally
Expand Down
16 changes: 12 additions & 4 deletions Tests/SwiftNetworkTests/SwiftNetworkQUICIdleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,14 @@ final class SwiftNetworkQUICIdleTests: NetTestCase {
let client = harness.state?.clientInstance
XCTAssertNotNil(client, "Client instance needs to be present to proceed")
if let client, let path = client.currentPath {
XCTAssertGreaterThan(
client.ack.unackedPacketCount,
0,
"Client should still owe the peer a delayed ACK"
// The ACK for the echoed data may have been sent already, alone or along
// with a PMTUD probe, so owe the peer one for the timer to send.
client.fromExternal { _ in
client.ack.shouldTransmit(packetNumberSpace: .applicationData)
}
XCTAssertTrue(
client.ack.ackRequiresAssembly(packetNumberSpace: .applicationData),
"Client should owe the peer an ACK"
)

// Acknowledge a probe short of the path maximum, which leaves the
Expand All @@ -195,6 +199,10 @@ final class SwiftNetworkQUICIdleTests: NetTestCase {
client.fireDelayedAckTimer(at: .systemNow, in: &eventContext)
}

XCTAssertFalse(
client.ack.ackRequiresAssembly(packetNumberSpace: .applicationData),
"Client should not owe the peer an ACK once the delayed ACK has been sent"
)
XCTAssertEqual(
client.ack.unackedPacketCount,
0,
Expand Down