--- a/handoff.swift +++ b/handoff.swift @@ -1,26 +1,39 @@ -// EXERCISE 04 — BROKEN STARTING POINT. Do not edit this file; copy it. +// EXERCISE 04 — FIXED VARIANT. The repair is to treat the shared PREDICATE as +// the state and the wakeup as nothing more than a hint. // -// ============================ UNSAFE CODE WARNING ============================ -// The two consumers below use a condition variable incorrectly, on purpose. -// Both mistakes are bounded here by finite deadlines and a watchdog. In real -// code the second one is a trap on an empty array, i.e. a crash. -// ============================================================================= -// // THE SCENARIO // A settings loader publishes a configuration and announces it. A consumer -// waits for that announcement. In the field: "the app occasionally sits on -// the launch screen for a second and a half, then carries on normally", and -// separately, "a background reader crashes taking an item from an empty -// queue, about once a week". +// waits for that announcement. // -// Build: swiftc -swift-version 6 -O handoff.swift -o handoff_broken -// Run: ./handoff_broken +// THE REPAIR — one rule, applied in two places +// Never wait on a condition variable without a loop around the predicate: // -// Expected: partA reports that the consumer slept for its full deadline even -// though the data was already published, and partB reports a nonzero -// wouldHaveCrashed count. Exit status 0 — neither mistake is a crash HERE, -// only because this fixture counts the failure instead of taking the item. +// while !predicate { cond.wait(until: deadline) } // +// That single line fixes both symptoms at once, and it is worth seeing why +// they are the same bug: +// +// * The check BEFORE the first wait is what makes a lost announcement +// harmless. If the thing already happened, the loop body never runs and the +// consumer proceeds immediately. An NSCondition stores nothing, so an +// announcement delivered before anyone was listening is simply gone; the +// predicate is what survives it. +// +// * The re-check AFTER each wakeup is what makes a wakeup safe to act on. A +// wakeup means "the predicate MAY have changed", never "the item is yours". +// Another consumer may have taken it first, and the kernel is permitted to +// wake you for no reason at all. +// +// `signal()` is also upgraded to `broadcast()` where more than one waiter can +// be satisfied by one state change; with a correct predicate loop, an extra +// wakeup costs a re-check rather than a defect. +// +// Build: swiftc -swift-version 6 -O handoff.swift -o handoff_fixed +// Run: ./handoff_fixed +// +// Expected: partA returns essentially instantly, partB reports +// wouldHaveCrashed=0 and consumes every item, and correct=true. +// // Bounded: every wait has a finite deadline, every loop has an iteration cap, // and a watchdog force-exits after WATCHDOG_S. @@ -42,7 +55,8 @@ // MARK: - Part A — the announcement that arrived before anyone was listening // // The loader publishes the configuration and signals. No consumer has started -// yet. The consumer then begins, and waits. +// yet. The consumer then begins, checks the predicate, and finds it already +// satisfied, so it never waits at all. func partA() -> (waitedMs: Int, dataWasAlreadyPublished: Bool) { let cond = NSCondition() @@ -58,9 +72,11 @@ let start = Date() cond.lock() - // BUG: the consumer waits for an announcement without ever asking whether - // the thing being announced has already happened. - _ = cond.wait(until: deadline) + // The predicate is the state. Ask about it first; wait only while it is + // false; re-ask after every wakeup. + while configuration == nil { + if !cond.wait(until: deadline) { break } // finite deadline + } let published = configuration != nil cond.unlock() @@ -85,7 +101,7 @@ func publish(_ v: Int) { cond.lock(); defer { cond.unlock() } items.append(v) - cond.signal() + cond.broadcast() // an extra wakeup now costs one re-check } func close() { @@ -102,15 +118,18 @@ cond.broadcast() } - /// Returns the next item, nil if the queue closed, or -1 to mean "in real - /// code this line would have been `items.removeFirst()` on an empty array, - /// i.e. a trap". + /// Returns the next item, or nil once the queue has closed and drained. + /// The -1 "would have trapped" path is now unreachable, and is kept only so + /// that the two builds report the same fields. func take(deadline: Date) -> Int? { cond.lock(); defer { cond.unlock() } - if items.isEmpty && !closed { // BUG: checked once, never re-checked - _ = cond.wait(until: deadline) + // `while`, not `if`. Re-evaluate on EVERY wakeup, because a wakeup is a + // hint rather than a promise: another consumer may have taken the item, + // or the kernel may have woken us for no reason. + while items.isEmpty && !closed { + if !cond.wait(until: deadline) { break } + if items.isEmpty && !closed { stats.withLock { $0.spuriousAbsorbed += 1 } } } - // The line below ASSUMES the wakeup meant "an item is ready". guard !items.isEmpty else { if closed { return nil } stats.withLock { $0.wouldHaveCrashed += 1 } @@ -177,14 +196,14 @@ // MARK: - Entry -armWatchdog(WATCHDOG_S, "EX04 build=broken") +armWatchdog(WATCHDOG_S, "EX04 build=fixed") let a = partA() let b = partB() let c = partC() let sleptThroughPublishedData = a.waitedMs >= 1000 && a.dataWasAlreadyPublished -print("EX04 build=broken " +print("EX04 build=fixed " + "sleptThroughPublishedData=\(sleptThroughPublishedData) " + "partAWaitedMs=\(a.waitedMs) " + "wouldHaveCrashed=\(b.wouldHaveCrashed) "