From 0e4b12654d93f5ac6a0950dd6be79f9be3f044c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 9 May 2026 02:05:55 +0000 Subject: [PATCH] =?UTF-8?q?perf(quic):=20lock-free=20hot=20paths=20?= =?UTF-8?q?=E2=80=94=20ThreadLocal=20Cipher,=20AtomicReference=20close,=20?= =?UTF-8?q?@Volatile=20getters?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit of blocking and synchronized code in the QUIC module surfaced four hot-path wins, all verified against :quic:jvmTest. - PlatformCrypto: header-protection AES-ECB now uses a per-thread cached Cipher. Previously every inbound and outbound packet paid for Cipher.getInstance("AES/ECB/NoPadding") provider lookup. ThreadLocal is safe because the call is stateless — every invocation re-init's with the caller-supplied key. - JdkCertificateValidator: gate the SAN-side InetAddress.getByName behind looksLikeIpLiteral so a malformed cert with a hostname in a type 7 SAN cannot trigger DNS resolution on the TLS validation path. - QuicConnectionDriver.close: replace synchronized(this) double-checked init with AtomicReference + compareAndSet on a CoroutineStart.LAZY job. Lock-free, removes a synchronized from commonMain, preserves the original "first caller wins, second awaits same Job" contract. - SendBuffer: mark _nextOffset, nextSendOffset, _finPending, _finSent, _finAcked @Volatile and drop synchronized from their single-field getters (nextOffset, sentOffset, finPending, finSent, finAcked). The compound-formula readableBytes getter still synchronizes. https://claude.ai/code/session_01CXTjnuHKCNXDmpfyKxgG3V --- .../quic/connection/QuicConnectionDriver.kt | 83 +++++++++++-------- .../vitorpamplona/quic/stream/SendBuffer.kt | 28 +++++-- .../quic/crypto/PlatformCrypto.kt | 16 +++- .../quic/tls/JdkCertificateValidator.kt | 16 +++- 4 files changed, 95 insertions(+), 48 deletions(-) diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt index 3be70de9b..5adedc9f1 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnectionDriver.kt @@ -31,6 +31,8 @@ import kotlinx.coroutines.joinAll import kotlinx.coroutines.launch import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withTimeoutOrNull +import kotlin.concurrent.atomics.AtomicReference +import kotlin.concurrent.atomics.ExperimentalAtomicApi /** * Owns the UDP socket and runs the read + send loops for a [QuicConnection]. @@ -48,6 +50,7 @@ import kotlinx.coroutines.withTimeoutOrNull * and [QuicConnection.openBidiStream]/[com.vitorpamplona.quic.stream.SendBuffer.enqueue]) * call [wakeup] to nudge the send loop. */ +@OptIn(ExperimentalAtomicApi::class) class QuicConnectionDriver( val connection: QuicConnection, private val socket: UdpSocket, @@ -94,17 +97,20 @@ class QuicConnectionDriver( * to poll `connection.status == CLOSED` and trust that the rest of * the cleanup eventually settled. */ - internal val closeTeardownJob: Job? get() = closeJob + internal val closeTeardownJob: Job? get() = closeJob.load() /** * Round-5 concurrency #5: close() guard. A second concurrent invocation * (e.g. session close + read-loop death close racing) used to launch a * parallel teardown that called scope.cancel() and socket.close() while - * the first close was mid-joinAll. We now memoize the teardown Job so - * the second caller awaits the first's completion instead. + * the first close was mid-joinAll. We memoize the teardown Job so the + * second caller awaits the first's completion instead. + * + * Lock-free CAS replaces the previous `synchronized(this)` double-checked + * init: the close path is single-shot ("first writer wins"), which is + * exactly what `compareAndSet` expresses. */ - @Volatile - private var closeJob: Job? = null + private val closeJob: AtomicReference = AtomicReference(null) fun start() { connection.start() @@ -365,38 +371,45 @@ class QuicConnectionDriver( // second concurrent caller (which is common: session.close() and // read-loop death both race to close()) awaits the same Job rather // than launching a parallel teardown. - if (closeJob != null) return - synchronized(this) { - if (closeJob != null) return - closeJob = - parentScope.launch { - connection.close(0L, "") - wakeup() - val send = sendJob - // Bounded wait for the send loop to flush CONNECTION_CLOSE. - // We don't want to hang forever if the writer is wedged — - // the timeout is the upper bound on how long close() blocks. - withTimeoutOrNull(CLOSE_FLUSH_TIMEOUT_MILLIS) { - // Spin until the writer has actually drained the queued - // close. The CLOSING-status check transitions to CLOSED - // once drainOutbound builds the CONNECTION_CLOSE packet. - while (connection.status == QuicConnection.Status.CLOSING) { - kotlinx.coroutines.delay(1) - } + if (closeJob.load() != null) return + // Build the teardown coroutine LAZY so we can race-test the CAS + // without paying for a launched-and-cancelled Job on the loser. + val teardown = + parentScope.launch(start = kotlinx.coroutines.CoroutineStart.LAZY) { + connection.close(0L, "") + wakeup() + val send = sendJob + // Bounded wait for the send loop to flush CONNECTION_CLOSE. + // We don't want to hang forever if the writer is wedged — + // the timeout is the upper bound on how long close() blocks. + withTimeoutOrNull(CLOSE_FLUSH_TIMEOUT_MILLIS) { + // Spin until the writer has actually drained the queued + // close. The CLOSING-status check transitions to CLOSED + // once drainOutbound builds the CONNECTION_CLOSE packet. + while (connection.status == QuicConnection.Status.CLOSING) { + kotlinx.coroutines.delay(1) } - // Now flip to CLOSED so both loops exit their while-guards. - connection.markClosedExternally("driver close requested") - wakeup() - // Wait for both loops to actually exit — joinAll won't - // return until the in-flight socket.send() completes. - withTimeoutOrNull(CLOSE_FLUSH_TIMEOUT_MILLIS) { - listOfNotNull(readJob, send).joinAll() - } - // Final teardown — cancel guarantees both jobs are done - // before we close the socket. - scope.cancel() - socket.close() } + // Now flip to CLOSED so both loops exit their while-guards. + connection.markClosedExternally("driver close requested") + wakeup() + // Wait for both loops to actually exit — joinAll won't + // return until the in-flight socket.send() completes. + withTimeoutOrNull(CLOSE_FLUSH_TIMEOUT_MILLIS) { + listOfNotNull(readJob, send).joinAll() + } + // Final teardown — cancel guarantees both jobs are done + // before we close the socket. + scope.cancel() + socket.close() + } + if (closeJob.compareAndSet(null, teardown)) { + teardown.start() + } else { + // A concurrent close() already installed the teardown Job; drop + // ours without ever starting it. The winner's Job runs, this + // call is a no-op (matching the original idempotent contract). + teardown.cancel() } } diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/SendBuffer.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/SendBuffer.kt index 043009979..c5e488050 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/SendBuffer.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/stream/SendBuffer.kt @@ -60,9 +60,14 @@ package com.vitorpamplona.quic.stream * send loop under the connection mutex; [markAcked] / [markLost] run on * the parser path also under the connection mutex. The two execution * paths are NOT serialised by a shared lock, so all internal state is - * mutated under `synchronized(this)`. Even the cheap getters - * ([readableBytes], [sentOffset], [finPending], [finSent]) take the - * monitor so a writer pre-flight check can't observe torn state. + * mutated under `synchronized(this)`. Single-field reads + * ([nextOffset], [sentOffset], [finPending], [finSent], [finAcked]) + * use `@Volatile` backing fields and bypass the monitor — they cannot + * tear (Boolean is single-byte; Long writes happen inside the + * synchronized block on JVM/Android, where `@Volatile Long` is + * atomic). [readableBytes] still synchronizes because its formula + * combines two fields and would otherwise observe a transient + * negative value if read mid-`takeChunk`. * * # FIN * @@ -105,6 +110,7 @@ class SendBuffer( private var flushedFloor: Long = 0L /** Logical offset just past the last byte. Advances on [enqueue]. */ + @Volatile private var _nextOffset: Long = 0L /** @@ -115,6 +121,7 @@ class SendBuffer( * * Invariant: `flushedFloor <= nextSendOffset <= nextOffset`. */ + @Volatile private var nextSendOffset: Long = 0L /** @@ -151,14 +158,19 @@ class SendBuffer( */ private var retransmitTotalBytes: Long = 0L + @Volatile private var _finPending: Boolean = false + + @Volatile private var _finSent: Boolean = false + + @Volatile private var _finAcked: Boolean = false - val nextOffset: Long get() = synchronized(this) { _nextOffset } - val finPending: Boolean get() = synchronized(this) { _finPending } - val finSent: Boolean get() = synchronized(this) { _finSent } - val finAcked: Boolean get() = synchronized(this) { _finAcked } + val nextOffset: Long get() = _nextOffset + val finPending: Boolean get() = _finPending + val finSent: Boolean get() = _finSent + val finAcked: Boolean get() = _finAcked /** * Bytes the writer would emit on the next [takeChunk] before any @@ -183,7 +195,7 @@ class SendBuffer( * the high-water mark of *fresh* sends only, not the cumulative * retransmit volume. */ - val sentOffset: Long get() = synchronized(this) { nextSendOffset } + val sentOffset: Long get() = nextSendOffset fun enqueue(bytes: ByteArray) { if (bytes.isEmpty()) return diff --git a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/PlatformCrypto.kt b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/PlatformCrypto.kt index 1d593a364..8d148d1ea 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/PlatformCrypto.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/crypto/PlatformCrypto.kt @@ -26,11 +26,23 @@ import javax.crypto.spec.SecretKeySpec /** * One-block AES-ECB encryption via JCA. Used only by QUIC header protection - * (one block per packet, so no need for a more elaborate API). + * (one block per packet). The `Cipher` instance is cached per-thread so the + * read/send loops avoid `Cipher.getInstance(...)` provider lookup on every + * packet — which is the dominant cost for a 16-byte one-shot AEAD-less call. + * + * `ThreadLocal` is safe here because the call is stateless: every invocation + * re-`init`s with the caller-supplied key before `doFinal`, so a coroutine + * that hops dispatchers between calls just lands on whichever thread's cached + * `Cipher` it ends up on. No state leaks across calls. */ +private val aesEcbCipher: ThreadLocal = + ThreadLocal.withInitial { Cipher.getInstance("AES/ECB/NoPadding") } + actual val PlatformAesOneBlock: AesOneBlockEncrypt = AesOneBlockEncrypt { key, block -> - val cipher = Cipher.getInstance("AES/ECB/NoPadding") + // .get() is non-null because withInitial supplies a Cipher, but + // Kotlin sees the Java return type as platform-nullable. + val cipher = aesEcbCipher.get()!! cipher.init(Cipher.ENCRYPT_MODE, SecretKeySpec(key, "AES")) cipher.doFinal(block) } diff --git a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt index 74ec4ed9b..dfc12b886 100644 --- a/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt +++ b/quic/src/jvmAndroid/kotlin/com/vitorpamplona/quic/tls/JdkCertificateValidator.kt @@ -191,10 +191,20 @@ class JdkCertificateValidator( // GeneralName type 2 = dNSName, type 7 = iPAddress. if (type == 2 && dnsMatches(idnAscii(value), normalizedHost)) return true if (type == 7 && hostAsIp != null) { + // Defense-in-depth: a malformed cert could put a hostname in + // a type 7 SAN. Without this gate, InetAddress.getByName(value) + // would perform a DNS A/AAAA lookup on the validation path, + // both leaking the name in plaintext and blocking the read + // loop on the system resolver. Forcing a literal check keeps + // the JDK call to pure parsing (no I/O, no name service). val sanIp = - try { - InetAddress.getByName(value).hostAddress - } catch (_: Throwable) { + if (looksLikeIpLiteral(value)) { + try { + InetAddress.getByName(value).hostAddress + } catch (_: Throwable) { + null + } + } else { null } if (sanIp != null && sanIp.equals(hostAsIp, ignoreCase = true)) return true