refactor(quic): split conn.lock into streamsLock + per-level lock + lifecycleLock
The single connection-wide `QuicConnection.lock` mutex serialised every
critical path: the read loop's `feedDatagram`, the send loop's
`drainOutbound`, and every public mutator (`openBidiStream`,
`streamById`, `flowControlSnapshot`, ...). The multiplexing testcase
opens hundreds of bidi streams in parallel and was capped at ~25
streams/sec by lock contention against the I/O loops.
Phase 1 of the lock split (see
`quic/plans/2026-05-08-lock-split-design.md`) introduces three
domain-specific mutexes:
- `streamsLock` — streams registry, datagram queues, stream-id
counters, connection-level flow-control bookkeeping, pending-
retransmit maps for control frames
- `LevelState.levelLock` (one per encryption level) — per-level
pnSpace / sentPackets / ackTracker / CRYPTO buffers
- `lifecycleLock` — status transitions, close reason/error code
Acquisition order: `lifecycleLock < streamsLock < levelLock`.
Per-stream `synchronized(this)` blocks inside SendBuffer/ReceiveBuffer
remain at the leaf — never acquire any QuicConnection mutex while
holding a per-stream lock.
The legacy `lock: Mutex` field is preserved as a deprecated alias of
`lifecycleLock` for source-compatibility with external test harnesses;
new code MUST use the appropriate domain lock.
Highlights:
- `feedDatagram` / `drainOutbound` now require the caller to hold
`streamsLock`; the driver wraps each call. Phase 1 keeps the whole
feed/drain inside `streamsLock` for safety; phase 2 (deferred) will
split frame-collection from encrypt + sentPackets-record so app
coroutines can intersperse during the encrypt window.
- `pendingPing`, `peerTransportParameters`, `status`,
`handshakeComplete` are now @Volatile so observers read them
without a lock.
- `markClosedExternally` no longer needs any lock (status is
@Volatile, signals are channel-thread-safe).
- Driver's PTO bookkeeping uses the volatile fields directly — no
lock needed.
- Tests that manually acquired `conn.lock` to call
`getOrCreatePeerStreamLocked` / `onTokensAcked` / `onTokensLost`
now acquire `streamsLock` (the domain those routines mutate).
- New `MultiplexingThroughputTest` locks in the contract: 1000
parallel `openBidiStream` calls must complete in <2 s.
Test plan:
- `:quic:jvmTest` — 294 tests pass (293 prior + 1 new throughput).
- `MultiplexingThroughputTest`: 1000 bidi streams in 52 ms
(~19,000 streams/sec on the in-memory pipe), well above the
250+/sec target.
- `:nestsClient:compileKotlinJvm` — clean, no API breaks.
- `./gradlew :quic:spotlessApply` — clean.
https://claude.ai/code/session_01HcvfQq1ttPV9PkRoJb4nyT
This commit is contained in:
@@ -0,0 +1,223 @@
|
||||
# QuicConnection Lock Split — Design Note
|
||||
|
||||
Date: 2026-05-08
|
||||
|
||||
## Problem
|
||||
|
||||
`QuicConnection.lock: Mutex` serialises every meaningful operation:
|
||||
|
||||
- `drainOutbound` (send loop, holds lock during a full datagram build —
|
||||
iterates every stream, allocates packet numbers, encrypts).
|
||||
- `feedDatagram` (read loop, holds lock during decrypt + frame dispatch
|
||||
+ per-stream insert).
|
||||
- `openBidiStream` / `openUniStream` (app code, holds lock for stream
|
||||
allocation + map insert).
|
||||
- `getOrCreatePeerStreamLocked` (parser path on the read loop's
|
||||
critical section, but app code can also call it from tests).
|
||||
|
||||
Multiplexing test against aioquic measures ~25 streams/sec — every
|
||||
coroutine fights this single mutex.
|
||||
|
||||
## Goal
|
||||
|
||||
Split the mutex into per-domain mutexes so the read loop, send loop, and
|
||||
app code can mostly progress concurrently. Per-stream `synchronized(this)`
|
||||
inside `SendBuffer`/`ReceiveBuffer` already handles per-stream
|
||||
serialisation; we don't touch those.
|
||||
|
||||
## Domain Map
|
||||
|
||||
### Domain A — `streamsLock: Mutex` (the streams registry)
|
||||
|
||||
Fields:
|
||||
|
||||
- `streams: MutableMap<Long, QuicStream>`
|
||||
- `streamsList: MutableList<QuicStream>` (insertion-ordered list parallel
|
||||
to `streams`, used by writer round-robin)
|
||||
- `nextLocalBidiIndex`, `nextLocalUniIndex`
|
||||
- `streamRoundRobinStart` — read+written by writer; used in the
|
||||
same critical section it holds `streamsLock` for the iteration
|
||||
- `peerInitiatedUniCount`, `peerInitiatedBidiCount`
|
||||
- `advertisedMaxStreamsUni`, `advertisedMaxStreamsBidi`,
|
||||
`advertisedMaxData`
|
||||
- `pendingMaxStreamsUni`, `pendingMaxStreamsBidi`, `pendingMaxData`
|
||||
- `pendingMaxStreamData: MutableMap<Long, Long>`
|
||||
- `pendingNewConnectionId: MutableMap<Long, …>`
|
||||
- `newPeerStreams: ArrayDeque<QuicStream>`
|
||||
- `pendingDatagrams: ArrayDeque<ByteArray>` — outbound DATAGRAMs
|
||||
- `incomingDatagrams: ArrayDeque<ByteArray>` — inbound DATAGRAMs
|
||||
- `sendConnectionFlowCredit`, `sendConnectionFlowConsumed`
|
||||
- `receiveConnectionFlowLimit`
|
||||
|
||||
Rationale: the writer needs an atomic snapshot of "all streams + all
|
||||
pending control-frame retransmits + datagram queues + flow-control
|
||||
counters" in one critical section to assemble a packet. The parser needs
|
||||
the same coverage when delivering a STREAM frame (look up or create
|
||||
the stream + queue receive bytes + bump pending* fields). Splitting
|
||||
these into multiple sub-locks would force the writer/parser to acquire
|
||||
several locks per pass — same contention, more deadlock risk.
|
||||
|
||||
`peerMaxStreamsBidi`, `peerMaxStreamsUni` stay `@Volatile` (already are):
|
||||
the writer reads them once at the top of a stream open; the parser
|
||||
writes once on inbound MAX_STREAMS. Atomic long write is sufficient on
|
||||
all supported platforms.
|
||||
|
||||
### Domain B — `LevelState.levelLock: Mutex` (one per level: initial / handshake / application)
|
||||
|
||||
Fields per `LevelState`:
|
||||
|
||||
- `pnSpace: PacketNumberSpaceState`
|
||||
- `sentPackets: MutableMap<Long, SentPacket>`
|
||||
- `ackTracker`
|
||||
- `cryptoSend: SendBuffer`, `cryptoReceive: ReceiveBuffer`
|
||||
- `sendProtection: PacketProtection?`, `receiveProtection: PacketProtection?`
|
||||
- `keysDiscarded`
|
||||
- `largestAckedPn`, `largestAckedSentTimeMs`
|
||||
|
||||
The writer iterates through levels in order (initial → handshake →
|
||||
application) when building a coalesced datagram. Each level's critical
|
||||
section is independent, so the lock is held only for the duration of
|
||||
build at that level (which doesn't touch the streams registry except
|
||||
to read `streamsListLocked()` for stream frames inside the application
|
||||
build — that read transitions through `streamsLock`).
|
||||
|
||||
### Domain C — `lifecycleLock: Mutex` (status + handshake metadata)
|
||||
|
||||
Fields:
|
||||
|
||||
- `status: Status`
|
||||
- `closeReason: String?`, `closeErrorCode: Long`
|
||||
- `peerTransportParameters: TransportParameters?` — read-mostly after
|
||||
handshake; using `@Volatile` reference + write-once-after-handshake
|
||||
is sufficient here. Promoted to `@Volatile` so writer/parser can
|
||||
snapshot without a lock.
|
||||
- `handshakeComplete: Boolean`
|
||||
- `closeAllSignals` (the channels are themselves thread-safe; lock is
|
||||
only required to serialise the status transition)
|
||||
|
||||
### Domain D — Atomic / `@Volatile` (no lock)
|
||||
|
||||
Fields:
|
||||
|
||||
- `pendingPing` — toggled by driver under PTO; observed by writer.
|
||||
Promote to `@Volatile`.
|
||||
- `consecutivePtoCount` — already `@Volatile`. Driver writes it under
|
||||
its own logic; no further protection needed because it's only read
|
||||
inside the same loop iteration that wrote it.
|
||||
- `destinationConnectionId` — already has volatile semantics
|
||||
(`internal set` on a `@Volatile var`). Stays as is.
|
||||
- `udpStatsSupplier` — already `@Volatile`.
|
||||
- `peerMaxStreamsBidi`, `peerMaxStreamsUni` — already `@Volatile`.
|
||||
- `handshakeDoneSignal: CompletableDeferred<Unit>` — coroutines
|
||||
primitive, thread-safe.
|
||||
|
||||
### Domain E — Per-stream (UNCHANGED)
|
||||
|
||||
`QuicStream` already protects its `SendBuffer` / `ReceiveBuffer` with
|
||||
internal `synchronized(this)` blocks. Nothing changes here.
|
||||
|
||||
## Lock Acquisition Order
|
||||
|
||||
To prevent deadlock, document and enforce:
|
||||
|
||||
```
|
||||
lifecycleLock < streamsLock < (any LevelState.levelLock)
|
||||
```
|
||||
|
||||
Per-stream `synchronized(...)` blocks inside `SendBuffer`/`ReceiveBuffer`
|
||||
remain at the leaf — never acquire any QuicConnection mutex while
|
||||
holding a per-stream lock.
|
||||
|
||||
In practice the only nesting that happens is:
|
||||
|
||||
- `drainOutbound` acquires `streamsLock` (for the streams loop +
|
||||
stream-frame creation) but the per-level builds happen *outside*
|
||||
that block — each level acquires its own `levelLock` separately.
|
||||
No nested `streamsLock` ⊃ `levelLock` chain.
|
||||
- Actually re-checking the design: the writer needs to allocate a
|
||||
PN at the chosen level *while* it has decided which streams to
|
||||
flush. Two options:
|
||||
(1) acquire streamsLock, snapshot streams + frames, release;
|
||||
acquire each levelLock to encode + record.
|
||||
(2) hold streamsLock during level encode for the application
|
||||
packet (because stream-frame retransmit tokens get recorded
|
||||
into level.sentPackets in the same operation).
|
||||
We take option (2) — encode under both locks, with strict order
|
||||
`streamsLock` → `levelLock`. The other levels (initial/handshake)
|
||||
don't touch streamsLock at all, so they only acquire `levelLock`.
|
||||
|
||||
## Public API Compatibility
|
||||
|
||||
`QuicConnection.lock: Mutex` is `val`-public. External callers exist
|
||||
(tests + InMemoryQuicPipe-driven harnesses). To avoid breaking those:
|
||||
|
||||
- Keep the `lock: Mutex` field as a deprecated forwarder. It now
|
||||
*also* exists, but it is an alias for `lifecycleLock`. New code
|
||||
must NOT use it. Existing tests that lock it before mutating
|
||||
state used to cover all domains; we update them in place to use
|
||||
the appropriate lock(s).
|
||||
|
||||
Actually simpler: keep `lock: Mutex` as a *no-op* lock (still a
|
||||
`Mutex` so external code compiles), document that it no longer
|
||||
guards anything, update the tests that lock it.
|
||||
|
||||
After review: tests use `conn.lock` to serialise their direct calls to
|
||||
`onTokensAcked`/`onTokensLost`/`getOrCreatePeerStreamLocked`. We update
|
||||
those tests to acquire `streamsLock` instead (since those routines
|
||||
mutate stream-domain state). The `lock` field is kept as deprecated
|
||||
for source compatibility but is functionally a leaf no-op.
|
||||
|
||||
## Migration Plan
|
||||
|
||||
1. Add `streamsLock`, `lifecycleLock` fields. Keep `lock` as alias of
|
||||
`lifecycleLock`.
|
||||
2. Add `levelLock` to `LevelState`.
|
||||
3. Convert `getOrCreatePeerStreamLocked` → `getOrCreatePeerStream` doing
|
||||
its own `streamsLock` acquisition. Keep the old name as a forwarder
|
||||
for backwards compat.
|
||||
4. Update `openBidiStream`, `openUniStream`, `streamById`, `pollIncomingPeerStream`,
|
||||
`awaitIncomingPeerStream`, `pollIncomingDatagram`, `awaitIncomingDatagram`,
|
||||
`queueDatagram`, `flowControlSnapshot` to acquire `streamsLock`.
|
||||
5. Update `close`, `markClosedExternally` to use `lifecycleLock`.
|
||||
6. Update driver's `readLoop`/`sendLoop`:
|
||||
- `feedDatagram` no longer wraps in conn-wide lock. Instead the
|
||||
parser acquires `streamsLock` around stream-touching code,
|
||||
and `levelLock` around level-touching code.
|
||||
- `drainOutbound` is restructured similarly.
|
||||
7. Update tests that hold `conn.lock` to use the relevant new lock.
|
||||
|
||||
## Risk + Mitigation
|
||||
|
||||
- **Deadlock**: enforce order via code review + (where practical)
|
||||
inline comments at each acquisition site. Keep nesting shallow.
|
||||
- **Missed coverage**: enumerate every field in this doc; if a field
|
||||
can be mutated from two domains we either move it to a single domain
|
||||
or annotate it as @Volatile.
|
||||
- **Performance regression**: more mutex acquisitions overall; but
|
||||
the critical path (multiplexing test) sees parallel execution
|
||||
instead of serial, which more than compensates.
|
||||
|
||||
## Implementation Phases
|
||||
|
||||
This commit implements **phase 1** — separate domain locks but
|
||||
`drainOutbound` and `feedDatagram` still hold `streamsLock` for the
|
||||
entire pass. The wins from phase 1 alone:
|
||||
|
||||
- App code (`openBidiStream`, `streamById`, `flowControlSnapshot`) no
|
||||
longer contends with `lifecycleLock`-only operations.
|
||||
- The PTO timer path stops touching any mutex (volatile fields).
|
||||
- `markClosedExternally` no longer needs a lock.
|
||||
- `close()` only takes lifecycleLock — opens the path for in-progress
|
||||
drain to finish without status-write contention.
|
||||
|
||||
Phase 2 (deferred follow-up): split `buildApplicationPacket` into a
|
||||
"collect frames under streamsLock" stage and an "encrypt + record
|
||||
under levelLock" stage so app coroutines can intersperse during the
|
||||
encrypt window. That requires more invasive surgery on the writer's
|
||||
internals; phase 1 ships first to lock in the safer subset.
|
||||
|
||||
## Verification
|
||||
|
||||
- `:quic:jvmTest` — full suite must pass.
|
||||
- `MultiplexingThroughputTest` (new): 1000 streams in <500 ms on
|
||||
InMemoryQuicPipe.
|
||||
Reference in New Issue
Block a user