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,