diff --git a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt index 9a6efe315..aeaafd667 100644 --- a/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt +++ b/quic/src/commonMain/kotlin/com/vitorpamplona/quic/connection/QuicConnection.kt @@ -770,6 +770,19 @@ class QuicConnection( * into coalesced packets instead of emitting one tiny packet per * stream. * + * **`init` runs under `streamsLock`.** It must not suspend + * (the type signature enforces this) and SHOULD be fast — any + * expensive work (encoding, allocation-heavy formatting) belongs + * outside the call so it doesn't extend the lock-hold time. The + * intended shape per caller: + * + * val encoded = items.map { encode(it) } // outside + * conn.openBidiStreamsBatch(encoded) { stream, payload -> // under lock + * stream.send.enqueue(payload) + * stream.send.finish() + * Handle(stream) + * } + * * This is the bug-resistant API for the prepareRequests pattern. * The previous shape (caller manually wraps `streamsLock.withLock` * around a loop of [openBidiStreamLocked]) regressed twice: once @@ -785,10 +798,12 @@ class QuicConnection( suspend fun openBidiStreamsBatch( items: List, init: (QuicStream, I) -> R, - ): List = - streamsLock.withLock { + ): List { + if (items.isEmpty()) return emptyList() + return streamsLock.withLock { items.map { init(openBidiStreamLocked(), it) } } + } /** * The streamsLock-holding primitive used by [openBidiStream] and @@ -868,20 +883,26 @@ class QuicConnection( * entry under a single [streamsLock] hold and run [init] for * each (stream, item). * + * **`init` runs under `streamsLock`** — same caveat as + * [openBidiStreamsBatch]: keep it fast, encode outside the call. + * * Use this for moq audio-rooms and any other path that opens many * uni streams in burst — without batching, each open releases the * lock and the send loop can interject, emitting one stream per * packet (the same shape that broke bidi multiplexing on - * 2026-05-06). + * 2026-05-06). [bestEffort] applies uniformly to every stream + * in the batch; mixed-mode batches need separate calls. */ suspend fun openUniStreamsBatch( items: List, bestEffort: Boolean = false, init: (QuicStream, I) -> R, - ): List = - streamsLock.withLock { + ): List { + if (items.isEmpty()) return emptyList() + return streamsLock.withLock { items.map { init(openUniStreamLocked(bestEffort), it) } } + } /** Snapshot of peer-granted bidi cap. Reads do not need the lock — long writes are atomic on every supported platform. */ fun peerMaxStreamsBidiSnapshot(): Long = peerMaxStreamsBidi diff --git a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/BatchedOpenLockContractTest.kt b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/BatchedOpenLockContractTest.kt index 9f2b4909d..65c7bfb8e 100644 --- a/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/BatchedOpenLockContractTest.kt +++ b/quic/src/commonTest/kotlin/com/vitorpamplona/quic/connection/BatchedOpenLockContractTest.kt @@ -28,6 +28,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNotNull +import kotlin.test.assertTrue /** * Lock contract for batched stream opens — the production pattern used by @@ -117,21 +118,43 @@ class BatchedOpenLockContractTest { } @Test - fun `openBidiStreamsBatch is the bug-resistant API for the prepareRequests pattern`() { + fun `openBidiStreamsBatch holds streamsLock for the entire batch`() { runBlocking { val client = handshakedClient() - // The new high-level API — caller can't hold the wrong - // lock because it doesn't take any. Encapsulates the - // streamsLock acquisition + per-item init under the lock. + // Verify the API actually holds streamsLock during init — + // the whole point of the function. A regression that + // released the lock between opens (the 2026-05-06 bug + // shape) would let isLocked drop to false inside init. val payloads = (0 until 64).map { "req-$it".encodeToByteArray() } + var lockedAtSomePoint = false val streams = client.openBidiStreamsBatch(payloads) { stream, payload -> + if (client.streamsLock.isLocked) lockedAtSomePoint = true stream.send.enqueue(payload) stream.send.finish() stream } assertEquals(64, streams.size) assertEquals(64, streams.map { it.streamId }.toSet().size) + assertTrue( + lockedAtSomePoint, + "streamsLock must be held while init runs — without that, " + + "the send loop interleaves between opens", + ) + } + } + + @Test + fun `openBidiStreamsBatch with empty list does not throw`() { + runBlocking { + val client = handshakedClient() + // Empty-batch short-circuit: no items, no lock acquisition, + // no per-item init call. Returns empty list cleanly. + val result: List = + client.openBidiStreamsBatch(emptyList()) { _, _ -> + error("init must not run for empty batch") + } + assertEquals(0, result.size) } } @@ -146,7 +169,7 @@ class BatchedOpenLockContractTest { } @Test - fun `openUniStreamsBatch holds streamsLock for the whole batch`() { + fun `openUniStreamsBatch holds streamsLock for the entire batch`() { runBlocking { val client = handshakedClient() // moq audio-rooms shape: many uni streams in burst. Without @@ -154,10 +177,15 @@ class BatchedOpenLockContractTest { // send loop interjects (the same shape that broke bidi // multiplexing on 2026-05-06). val items = List(16) { it } + var lockedAtSomePoint = false val streams = - client.openUniStreamsBatch(items) { stream, _ -> stream } + client.openUniStreamsBatch(items) { stream, _ -> + if (client.streamsLock.isLocked) lockedAtSomePoint = true + stream + } assertEquals(16, streams.size) assertEquals(16, streams.map { it.streamId }.toSet().size) + assertTrue(lockedAtSomePoint, "streamsLock must be held while init runs") } }