The lock held across a network call, and the deadlock that followed

TL;DR#

A lock's cost is not the lock. It is how long you hold it, and that is determined entirely by what you put inside the critical section. A mutex around a map update is free. The same mutex around a network call makes every concurrent request wait for that call.

Ours wrapped a token refresh: acquire the lock, call the auth server, store the token, release. Under normal conditions the call took 40ms and nobody noticed. When the auth server degraded to 20 seconds, throughput went to one request per 20 seconds — across the whole service, because they all shared the lock.

Hold locks over memory operations only. I/O inside a critical section is the bug, and it looks like correct synchronisation.

The case this post is about#

A token cache, synchronised to avoid refreshing twice:

class TokenProvider(private val auth: AuthClient) {
    private val mutex = Mutex()
    private var token: Token? = null

    suspend fun current(): Token = mutex.withLock {        // ← held across everything below
        val existing = token
        if (existing != null && !existing.isExpired()) return@withLock existing

        val fresh = auth.refresh()                          // ← NETWORK CALL, inside the lock
        token = fresh
        fresh
    }
}

The intent is right: without the lock, 50 concurrent requests on an expired token cause 50 refreshes. The mutex solves that, and introduces a worse problem.

The symptom#

Total throughput collapse during an auth-provider incident:

auth.refresh() p99 latency   40ms → 19.8s   (provider degraded)

our service
  requests/sec     1,240 → 0.05
  p99 latency      85ms  → 112s
  every endpoint affected, including those not using the token

The thread dump, which made it obvious:

"http-worker-1"  RUNNABLE   at AuthClient.refresh  (waiting on socket read)
"http-worker-2"  WAITING    at Mutex.lock           ← and 47 more identical
"http-worker-3"  WAITING    at Mutex.lock
…
47 threads waiting on the same mutex, held by one thread in a socket read

And then an actual deadlock, which is the part that took longest to understand. The auth client had a response interceptor that logged request metadata — and the metadata included the current token:

worker-1  holds tokenMutex, awaiting auth.refresh()
          → interceptor fires on the response
          → interceptor calls TokenProvider.current() for logging
          → tries to acquire tokenMutex
          → deadlock: waiting for a lock it already holds, re-entrantly

Mutex in kotlinx.coroutines is not reentrant, so the same coroutine re-acquiring it blocks forever. The service required a restart.

Why the obvious fix failed#

"Make the mutex reentrant." It would have converted the deadlock back into the throughput collapse, which is an improvement and not a fix. Reentrancy also hides the real problem: a lock whose critical section can call arbitrary code.

"Add a timeout to the lock acquisition." withTimeout(5.seconds) { mutex.withLock { … } } makes waiters give up, so now 47 requests fail fast instead of hanging. Better behaviour, same serialisation, and the failures are not actually necessary.

"Add a timeout to the auth call." Necessary and insufficient: a 2-second timeout bounds the hold to 2 seconds, so throughput is 0.5 req/s instead of 0.05. The lock still serialises everything.

"Use a read-write lock." The refresh is a write and all callers go through the same path. There is no read-mostly pattern to exploit while the token is expired.

The actual mechanism#

Two things to separate.

A lock protects an invariant, and the invariant here is "do not refresh concurrently". That is a legitimate thing to want.

A lock's hold duration is whatever is inside it. Hold time determines how much concurrency you destroy, and the relationship is brutal: if a lock is held for duration d, maximum throughput through that section is 1/d, regardless of how many cores or threads you have.

That relationship is the part worth internalising, because it means a lock's cost is set by its slowest occupant rather than its typical one. A section holding the lock for 2ms on the happy path and 2 seconds when the token endpoint is slow does not average out to something acceptable — during those 2 seconds the ceiling is 0.5 requests/second, and every arrival queues. The tail latency of whatever is inside the lock becomes the throughput of everything that needs it.

d = 40ms   →   25 req/s through the section
d = 20s    →   0.05 req/s

So putting I/O inside a critical section makes your concurrency limit a function of someone else's latency. The auth provider's p99 became our throughput ceiling.

And the deadlock has a specific cause worth naming: the critical section called out to code it did not control. Once an interceptor, callback, listener or plugin can run inside your lock, the set of things that might try to acquire it is unbounded.

A lock's cost is its hold time, and I/O inside a critical section hands your concurrency limit to another system. Hold locks over memory only, and never call out to code you do not control while holding one.

The fix#

Keep the I/O outside the lock, and use the lock only to publish the result — with single-flight semantics so the original "do not refresh twice" invariant survives.

class TokenProvider(private val auth: AuthClient, private val scope: CoroutineScope) {

    private val mutex = Mutex()                    // guards ONLY the two fields below
    private var token: Token? = null
    private var inFlight: Deferred<Token>? = null

    suspend fun current(): Token {
        // 1. fast path: a short, memory-only critical section
        mutex.withLock {
            token?.let { if (!it.isExpired()) return it }
            // 2. single-flight: if a refresh is running, join it — do NOT start another
            inFlight?.let { return@withLock }
                ?: run { inFlight = scope.async { auth.refresh() } }      // started, not awaited
        }

        // 3. the network wait happens with NO lock held
        val existing = mutex.withLock { inFlight }!!
        val fresh = try {
            existing.await()
        } finally {
            mutex.withLock { if (inFlight === existing) inFlight = null }
        }

        mutex.withLock { token = fresh }
        return fresh
    }
}

Three properties:

Every critical section is memory-only — a field read, a field write, an assignment. Hold time is measured in microseconds, so the lock stops being a throughput limit.

Single-flight preserves the original invariant. Fifty concurrent callers on an expired token start one refresh and all await the same Deferred. That is what the lock was for, achieved without holding it across the call.

And the interceptor deadlock is structurally impossible, because no external code runs inside a critical section.

Two more things that belong with this fix:

Refresh proactively, so the expired-token path is rare:

// refresh at 80% of the lifetime, in the background — callers never wait at all
private fun scheduleRefresh(t: Token) = scope.launch {
    delay(t.lifetime * 0.8)
    runCatching { current() }
}

And a bounded wait, so a degraded provider fails rather than queues:

val fresh = withTimeout(5.seconds) { existing.await() }

Which composes with a request deadline rather than inventing its own number, if you have one.

How we proved it#

Throughput under a slow dependency, which is the claim:

auth.refresh() forced to 20s, 50 concurrent requests
                              throughput   p99      refresh calls
lock held across I/O             0.05/s     112s          1
+ lock timeout 5s                0.05/s     5s (fail)     1
+ auth timeout 2s                 0.5/s     2s            1
single-flight, no lock over I/O    980/s     20s          1

That last row needs explaining, because it looks paradoxical: throughput is high because requests not needing a token proceed freely, and the 50 that do all wait on one shared refresh. One refresh call, no serialisation of unrelated work.

Normal-conditions throughput, to confirm no regression:

auth.refresh() at 40ms
  before:  1,240 req/s
  after:   1,290 req/s

That single-flight holds — the invariant the lock originally existed for:

@Test
fun fiftyConcurrentCallersCauseOneRefresh() = runTest {
    val auth = CountingAuthClient(latency = 500.milliseconds)
    val provider = TokenProvider(auth, backgroundScope)
    (1..50).map { async { provider.current() } }.awaitAll()
    assertEquals(1, auth.refreshCount)
}

no lock at all:        FAILED — 50 refreshes
lock across I/O:       PASSED — 1
single-flight:         PASSED — 1

And a deterministic reproduction of the deadlock, before and after:

@Test
fun anInterceptorCallingBackDoesNotDeadlock() = runTest {
    val auth = AuthClient(interceptor = { provider.current() })   // calls back in
    withTimeout(2.seconds) { provider.current() }                 // fails by timing out if deadlocked
}

lock across I/O:   FAILED — TimeoutCancellationException (deadlock)
single-flight:     PASSED

Plus an audit for the shape, since this is a pattern rather than an instance:

# find critical sections containing a suspend call that is not a pure computation
$ grep -rn -A12 'withLock\s*{\|synchronized\s*(' --include='*.kt' . \
  | grep -E '\.(refresh|fetch|get|post|query|execute|await)\('

critical sections containing I/O:   5
  TokenProvider           (this one)
  ConfigCache             (HTTP fetch inside a lock)
  MetricsFlusher          (network flush inside a lock)
  SchemaRegistry          (DB query inside a lock)
  AvatarCache             (file read inside a lock — bounded, left alone)

Four more, three of which were fixed the same way.

One limit: the single-flight implementation above has a subtle sequence — the first critical section starts the async without awaiting it, which means a failure in auth.refresh() propagates to every joined caller. That is correct behaviour and worth being explicit about, since it means one provider failure fails all 50 waiters rather than letting them retry independently. We accepted it; a retry belongs around current(), not inside it.

What this generalizes to#

A lock's cost is its hold time, and hold time is a design choice. Maximum throughput through a critical section is 1/hold_time. Shortening the section is almost always more effective than switching lock implementations.

Never do I/O while holding a lock. Network calls, disk reads, database queries, and anything with an unbounded wait. The hold becomes a function of another system's latency, which means your concurrency limit is set by someone else's bad day.

Never call out to unknown code inside a critical section. Interceptors, callbacks, listeners, plugins, user-supplied comparators. The moment arbitrary code runs under your lock, re-entry and deadlock are reachable.

Separate "do the work" from "publish the result". Single-flight is the general pattern: one actor performs the operation outside any lock, and a short critical section coordinates who waits and who starts. It preserves the invariant without buying the serialisation.

Related#

• Every timeout you set independently adds up to one that is wrong — bounding the wait this exposes
• The N+1 query that looked fast in development — another pool-saturating pattern
• One failed child cancelled the whole screen, and the scope was the reason — why the shared failure propagates to all waiters

Keep reading#

This is part of 100 practices of senior engineers — the habits that separate five years of experience from one year five times. Follow MobileByteSensei for the rest.