diff --git a/Sources/SwiftNetwork/QUIC/Recovery.swift b/Sources/SwiftNetwork/QUIC/Recovery.swift index acda37d..93eb7fb 100644 --- a/Sources/SwiftNetwork/QUIC/Recovery.swift +++ b/Sources/SwiftNetwork/QUIC/Recovery.swift @@ -656,14 +656,16 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { _ packets: consuming NetworkUniqueDeque, connection: QUICConnection ) -> Bool { - var packets = packets - guard !packets.isEmpty else { + if packets.isEmpty { return false } + + var packets = packets while !packets.isEmpty { - let packet = packets.remove(at: 0) + let packet = packets.removeFirst() sentPacket(packet, time: connection.now, connection: connection) } + return true } } @@ -754,7 +756,7 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { ) { var sentPackets = sentPackets while !sentPackets.isEmpty { - let packet = sentPackets.remove(at: 0) + let packet = sentPackets.removeFirst() sentPacket(packet, time: connection.now, connection: connection) } if inBatch { @@ -1115,25 +1117,27 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { mutating func sendPTO(connection: QUICConnection, path: QUICPath) { var sentPTO = false - let (_, pnSpace) = getEarliestTime( + + let (_, packetNumberSpace) = getEarliestTime( earliestTimeType: EarliestTimeType.lastSentAckElicitingTime, connection: connection ) - var hasAckEliciting = false - connection.withPendingItems(for: pnSpace) { pendingItems in - hasAckEliciting = pendingItems.hasAckElicitingPendingItems - } - let peerCompletedValidation = peerCompletedValidation(connection: connection) - var shouldClearTimer = false + + let hasAckEliciting = connection.withPendingItems(for: packetNumberSpace) { $0.hasAckElicitingPendingItems } var discardInitialRecoveryState = false applyToAllInnerStatesMutable { innerState, packetNumberSpace in let ackElicitingPacketsInFlight = innerState.ackElicitingPacketsInFlight - if ackElicitingPacketsInFlight == 0 { + guard ackElicitingPacketsInFlight > 0 else { + if _slowPath(ackElicitingPacketsInFlight < 0) { + connection.log.fault("ackElicitingPacketsInFlight negative: \(ackElicitingPacketsInFlight)") + } return } + connection.log.datapath( "PTO \(path.recoveryState.PTOCount) (\(packetNumberSpace)) fired on path \(path.identifier) with \(ackElicitingPacketsInFlight) ack-eliciting packets in flight" ) + if hasAckEliciting { connection.log.datapath("Sending next frames with new data as PTOs") sentPTO = true @@ -1147,8 +1151,10 @@ struct Recovery: ~Copyable, PrefixedLoggable, NonCopyableTimerUser { "Unable to force send PTOs, likely flow-controlled or unavailable" ) } - } else if ackElicitingPacketsInFlight > 0 { + + } else { connection.log.datapath("Retransmitting two tail-packets as PTO") + var addedPackets = 0 let packetCount = innerState.outstandingPackets.count for i in 0.. 0 (so the PTO stays armed) while + // being unrebuildable once the flow is closed. + var packet = SentPacketRecord() + packet.identifier = .init(space: .applicationData, number: 0) + packet.isInFlightEligible = true + packet.isAckEliciting = true + packet.totalLength = 20 + 96 + packet.sentPath = connection.currentPath?.identifier ?? .none + packet.transmittedItems.sentStreams.append( + TransmittedItems.SentStream( + flowID: stream.identifier, + streamID: QUICStreamID(0), + offset: 0, + length: 32, + isFinal: true + ) + ) + XCTAssertTrue(packet.transmittedItems.hasRetransmissibleItems) + sentPacket(packet, connection: connection) + + connection.recovery.withImmutableInnerState(packetNumberSpace: .applicationData) { innerState in + XCTAssertEqual(innerState.ackElicitingPacketsInFlight, 1) + } + + // Establish (address-validated) connection: peerCompletedValidation must be true. This is + // the condition under which the anti-deadlock PING is incorrectly skipped. + connection.recovery.received1RTTAck = true + XCTAssertTrue(connection.recovery.peerCompletedValidation(connection: connection)) + XCTAssertEqual(path.recoveryState.PTOCount, 0) + + // Fire the PTO with no new ack-eliciting data pending. + let expectation = XCTestExpectation() + self.connection.context.async { + self.connection.withCurrentPath { path in + self.connection.recovery.sendPTO(connection: self.connection, path: path) + } + expectation.fulfill() + } + wait(for: [expectation], timeout: 5.0) + + // The PTO must have sent a probe and advanced the PTO count. On the buggy code no probe is + // sent and PTOCount stays 0, so the connection would spin until idle timeout. + XCTAssertEqual( + path.recoveryState.PTOCount, + 1, + "PTO produced no probe for a closed-flow tail packet; connection would spin to idle timeout" + ) + } } #endif