Reading a Closure Out of a Generic Lock Box Causes Stack Overflow

After a few crash reports from users, Fable 5.1 has suggested that the Swift compiler has a bug. Now, a surefire way to look stupid is to tell a bunch of compiler experts that they have a bug. So before I file an actual report, I'd appreciate a second set of eyes on this analysis to see if it's accurate:

The Bug

Reading a function value out of a generic "locked box" (the ubiquitous withLockedValue pattern — e.g. NIOLockedValueBox in swift-nio, or any hand-rolled equivalent) using box.withLockedValue({ $0 }) mutates the stored value: the inout write-back re-stores the closure wrapped in a fresh pair of reabstraction thunks, even though the access only reads.

Every read therefore adds two stack frames to every future invocation of the stored closure. On a hot path, the stored closure's call depth grows without bound until a single call of it overflows the calling thread's stack (EXC_BAD_ACCESS / "Thread stack size exceeded due to excessive recursion").

Code to Reproduce:

// Demonstrates: reading a closure out of a generic lock box with `withLockedValue({ $0 })` under -O
// wraps the *stored* closure in a fresh pair of reabstraction thunks on every access — including a
// pure read. The stored value ratchets: each read adds two stack frames to every future call of the
// stored closure, until one more call overflows the thread's stack.
//
//   swiftc -O repro.swift -o repro && ./repro        # crashes (stack overflow) around batch 10
//   swiftc -Onone repro.swift -o repro && ./repro    # same crash — the ratchet is not optimizer-dependent
//
// The per-batch timings make the accumulation visible before the crash: each batch of identical
// work gets slower than the last, because calling the stored closure walks every thunk pair added
// by all previous reads.
//
// Extracted from a production crash in a SwiftNIO-based server, where the box was NIOLockedValueBox
// and the closure was a websocket message handler: every delivered message performed one read, so
// the handler gained two frames per message and the event-loop thread's stack overflowed after a
// day of traffic. This file reproduces it with no dependencies.

import Foundation

typealias Handler = (Int) -> Void

/// A minimal stand-in for NIOConcurrencyHelpers.NIOLockedValueBox.
final class LockedBox<Value>: @unchecked Sendable {
    private let lock = NSLock()
    private var value: Value

    init(_ value: Value) {
        self.value = value
    }

    func withLockedValue<T>(_ mutate: (inout Value) throws -> T) rethrows -> T {
        lock.lock()
        defer { lock.unlock() }
        return try mutate(&value)
    }
}

var sink: Int = 0

@inline(never)
func makeHandler() -> Handler {
    return { value in sink &+= value }
}

let box = LockedBox<Handler?>(nil)
box.withLockedValue { $0 = makeHandler() }

let iterationsPerBatch: Int = 10_000
let clock = ContinuousClock()

// Unbuffered, so the batch timings survive the crash the -O build ends in.
setvbuf(stdout, nil, _IONBF, 0)

for batch in 1...50 {
    let duration = clock.measure {
        for i in 0..<iterationsPerBatch {
            // A pure read of the stored closure, then a call — the pattern a message-delivery
            // hot path uses. Under -O the inout write-back re-stores the value with two new
            // reabstraction thunks around it, so every iteration deepens the stored closure.
            box.withLockedValue({ $0 })?(i)
        }
    }
    print("batch \(batch): \(iterationsPerBatch) reads took \(duration)")
}

print("survived — no ratchet (sink = \(sink))")

Output:

Running the above will generate output similar to this:

batch 1: 10000 reads took 0.590973625 seconds
batch 2: 10000 reads took 1.710135 seconds
batch 3: 10000 reads took 2.867418583 seconds
batch 4: 10000 reads took 4.034200791 seconds
batch 5: 10000 reads took 5.16922575 seconds
batch 6: 10000 reads took 6.31596125 seconds
batch 7: 10000 reads took 7.515117958 seconds
batch 8: 10000 reads took 8.653866292 seconds
batch 9: 10000 reads took 9.852811625 seconds
batch 10: 10000 reads took 11.039056 seconds

Fable's Analysis:

The box stores Value = Optional<(Int) -> Void> in generic storage, where the function value sits at its maximally-abstracted calling convention. withLockedValue passes the value inout to the caller's closure. Servicing that inout access materializes the value at the concrete convention (wrapping it in an abstract→direct thunk) and, on write-back, re-abstracts it (wrapping again in a direct→abstract thunk) — instead of recognizing the round trip as the identity. The stored value after one read is reabstract(deabstract(original)), and the pairs nest without limit.

Disassembly of the affected production binary shows the read path performing three swift_allocObject calls and storing the doubly-wrapped closure back into the box's storage before invoking the handler — on every read.

Real-world impact

Found via a production crash in a macOS app whose SwiftNIO websocket server stored its message handler in a NIOLockedValueBox<Handler?> and read it once per delivered message. Each message deepened the stored handler by two frames; after a day of message traffic (~100k+ messages), delivery of one more message overflowed a NIO event-loop thread's stack. The crash is arbitrarily far from the cause and the intermediate frames symbolicate as anonymous reabstraction thunks, which makes it exceptionally hard to diagnose from a crash report.

Workaround

Wrap the box's value in a class. The class reference crosses the inout boundary as a plain pointer retain, so nothing accumulates:

// workaround.swift
//
// The same access pattern as repro.swift, with one change: the box's value is a final class that
// holds the closure in a concrete stored property, instead of the bare function value. A class
// reference crosses the inout boundary as a plain pointer retain, so no reabstraction thunks can
// accumulate. Runs all 50 batches with flat timings at any optimization level.
//
//   swiftc -O workaround.swift -o workaround && ./workaround

import Foundation

typealias Handler = (Int) -> Void

final class HandlerHolder {
    let handle: Handler

    init(_ handler: @escaping Handler) {
        self.handle = handler
    }
}

final class LockedBox<Value>: @unchecked Sendable {
    private let lock = NSLock()
    private var value: Value

    init(_ value: Value) {
        self.value = value
    }

    func withLockedValue<T>(_ mutate: (inout Value) throws -> T) rethrows -> T {
        lock.lock()
        defer { lock.unlock() }
        return try mutate(&value)
    }
}

var sink: Int = 0

@inline(never)
func makeHandler() -> Handler {
    return { value in sink &+= value }
}

let box = LockedBox<HandlerHolder?>(nil)
box.withLockedValue { $0 = HandlerHolder(makeHandler()) }

let iterationsPerBatch: Int = 10_000
let clock = ContinuousClock()

setvbuf(stdout, nil, _IONBF, 0)

for batch in 1...50 {
    let duration = clock.measure {
        for i in 0..<iterationsPerBatch {
            box.withLockedValue({ $0 })?.handle(i)
        }
    }
    print("batch \(batch): \(iterationsPerBatch) reads took \(duration)")
}

print("survived — no ratchet (sink = \(sink))")
2 Likes

This is probably #54724 and/or #49727. Effectively, every assignment (including implicitly through the writeback of an inout parameter) wraps a closure in a new level of abstraction. Calling that closure will get slower and slower, and eventually calling that closure requires more stack than you have.

Wrapping the closure in anything that avoids that actual explicit closure assignment (e.g. a struct) should be enough to avoid the problem.

There's a discussion in the swift-open-source slack, too: Slack

2 Likes

Oh, there's been motion since then; 91348 explicitly mentions Mutex as a cause, and there's an open PR with a workaround for the particular case of closure literals that accept inout closure parameters: [SILGen] Don't write back reabstracted inout parameters that are never mutated by yuyouur · Pull Request #91770 · swiftlang/swift · GitHub

2 Likes

@KeithBauerANZ Ah, great! Glad to see it’s already being addressed. Thanks.

Yep. Also see Value wrapped in a thunk recursively(?) - #2 by Slava_Pestov.

3 Likes