From b60451b2ce80ce107c0e404b83294f00b891f96b Mon Sep 17 00:00:00 2001 From: deadcafe Date: Wed, 7 Oct 2026 07:00:57 +0530 Subject: [PATCH] QUIC: bound how long a due ACK waits for a packet to carry it - An ACK that is due but would be alone in its packet is held back to be bundled with data, and only the delayed ACK timer sent it, after the max ACK delay. An endpoint that only receives then acknowledges once per 25 ms, and a sender whose congestion window waits on those ACKs sends one window per 25 ms whatever the RTT - Such an ACK now waits max(smoothed RTT / 2, timer granularity), capped at the max ACK delay, on a timer of its own. A packet that carries the ACK first makes that timer a no-op, and the delayed ACK timer is unchanged - Timer.recalculate keeps a wakeup that is already armed for the same deadline. With a deadline inside the 1 ms threshold pending, every reschedule of another timer re-armed it - Tests: a one-way transfer completes with the max ACK delay stretched to 30 s, and the PMTUD delayed ACK test owes its ACK explicitly --- Sources/SwiftNetwork/QUIC/Ack.swift | 8 +++++ .../SwiftNetwork/QUIC/QUICConnection.swift | 36 +++++++++++++++++++ Sources/SwiftNetwork/QUIC/Timer.swift | 12 +++++-- Tests/SwiftNetworkTests/QUICTestHarness.swift | 19 ++++++---- .../SwiftNetworkQUICHarnessTests.swift | 25 +++++++++++++ .../SwiftNetworkQUICIdleTests.swift | 16 ++++++--- 6 files changed, 104 insertions(+), 12 deletions(-) diff --git a/Sources/SwiftNetwork/QUIC/Ack.swift b/Sources/SwiftNetwork/QUIC/Ack.swift index 706c31e7..f9b069d7 100644 --- a/Sources/SwiftNetwork/QUIC/Ack.swift +++ b/Sources/SwiftNetwork/QUIC/Ack.swift @@ -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 { diff --git a/Sources/SwiftNetwork/QUIC/QUICConnection.swift b/Sources/SwiftNetwork/QUIC/QUICConnection.swift index c8c67719..e93831d7 100644 --- a/Sources/SwiftNetwork/QUIC/QUICConnection.swift +++ b/Sources/SwiftNetwork/QUIC/QUICConnection.swift @@ -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? @@ -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, @@ -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. @@ -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 + ) + } return false } guard let path = currentPath else { diff --git a/Sources/SwiftNetwork/QUIC/Timer.swift b/Sources/SwiftNetwork/QUIC/Timer.swift index ce3be11e..e0e3039a 100644 --- a/Sources/SwiftNetwork/QUIC/Timer.swift +++ b/Sources/SwiftNetwork/QUIC/Timer.swift @@ -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( @@ -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 diff --git a/Tests/SwiftNetworkTests/QUICTestHarness.swift b/Tests/SwiftNetworkTests/QUICTestHarness.swift index ca58b88f..ff96c3e9 100644 --- a/Tests/SwiftNetworkTests/QUICTestHarness.swift +++ b/Tests/SwiftNetworkTests/QUICTestHarness.swift @@ -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") @@ -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 { @@ -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 { @@ -1066,6 +1071,7 @@ class QUICTestHarness { verifyResetStreamHalfClosure: Bool = false, shouldMarkIdle: Bool = false, shouldBatchSends: Bool = false, + shouldEchoData: Bool = true, clientOptions: ProtocolOptions = QUICProtocol.options(), serverOptions: ProtocolOptions = QUICProtocol.options(), sendMaxStreamUpdate: Bool = false, @@ -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) diff --git a/Tests/SwiftNetworkTests/SwiftNetworkQUICHarnessTests.swift b/Tests/SwiftNetworkTests/SwiftNetworkQUICHarnessTests.swift index fcffe0ce..dec06e4a 100644 --- a/Tests/SwiftNetworkTests/SwiftNetworkQUICHarnessTests.swift +++ b/Tests/SwiftNetworkTests/SwiftNetworkQUICHarnessTests.swift @@ -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 diff --git a/Tests/SwiftNetworkTests/SwiftNetworkQUICIdleTests.swift b/Tests/SwiftNetworkTests/SwiftNetworkQUICIdleTests.swift index 265f274f..d7bf573e 100644 --- a/Tests/SwiftNetworkTests/SwiftNetworkQUICIdleTests.swift +++ b/Tests/SwiftNetworkTests/SwiftNetworkQUICIdleTests.swift @@ -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 @@ -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,