Java SE 8 Programmer II: Oversell, a Stuck Shutdown Flag, and a Lock-Order Deadlock in One Reservation Service

Oversell, a Stuck Shutdown Flag, and a Lock-Order Deadlock in One Reservation Service

ShopFront's reservation service runs on an 8-core node behind a 32-thread cached pool. Under a load test of 200 concurrent checkout requests against a single hot SKU with stock 50, the service sold 51 units: a ConcurrentHashMap<String,Integer> stock map is read with get, compared, then written with put inside a helper method that is not synchronized, and an admin restock endpoint writes to the same map directly. A plain boolean running flag, set to false by the shutdown endpoint, is ignored by two worker threads until someone runs jstack against the JVM. Worse, reserve() synchronizes on the stock map first and then on the ledger object, while reconcile() takes them in the opposite order; under load two threads hang and the pool drains. The whole suite passes on a developer's 2-core laptop. The team wants the smallest change set that keeps throughput and removes the oversell, the stuck workers and the hang.

1. Explain exactly how the oversell happens, then evaluate three candidate fixes: (a) adding `synchronized` to the helper method, (b) replacing get/put with `map.compute`, (c) `map.merge`. Which do you recommend, and what residual risks remain?

Read the worked answer

The race is check-then-act: two threads both get(sku) and see 50, both compute 49, both put(49). One update is lost, so the map ends at 49 while 51 units were promised.

(a) synchronized helper - correct only if every writer uses the same monitor. The restock endpoint calls put directly and bypasses it, so the bug can reappear. It also serializes all SKUs through one lock, which hurts throughput on unrelated keys.
(b) compute - atomic for that key (the map locks the bin), so read-modify-write is safe and different SKUs run in parallel. Constraints: the remapping function must be short, must not modify the same map (recursive update throws IllegalStateException or deadlocks), and throwing leaves the mapping unchanged - which is actually useful for a 'not enough stock' rejection.
(c) merge - concise and atomic including the absent-key case, but the function must not return null unless removal is intended, and it cannot express a conditional rejection without throwing.

Recommend compute with a function that throws a domain OutOfStockException when stock < 1, caught at the request boundary, plus atomics for counters. Residual risk: the map is per-JVM. With two or more nodes, in-memory atomicity is useless - you need a conditional UPDATE stock SET qty = qty - ? WHERE sku = ? AND qty >= ?, a row lock, or optimistic versioning in the database.

2. The worker threads do not observe the shutdown flag. Explain why, and give a fix that also works when a worker is blocked waiting for work.

Read the worked answer

A flag written by the shutdown thread and read in a loop by workers has no happens-before edge unless it is volatile, synchronized, or otherwise safely published. The JIT may hoist the read out of the loop, so the worker spins on a stale value - exactly the 'ignored until jstack' symptom, since reaching a safepoint can make the loop re-read it and exit, which misleads debugging.

Fixes in layers:

private volatile boolean running = true; gives visibility; AtomicBoolean gives the same plus a CAS if you ever need compare-and-set. volatile does not make compound actions atomic, so do not extend it to counters.
Check the flag somewhere the loop actually reaches. If workers block in queue.take(), no memory-model change helps - use poll(timeout, unit) so the loop re-checks, or call shutdownNow() to interrupt, and handle InterruptedException by restoring the interrupt status and exiting.
Prefer ExecutorService.shutdown() plus awaitTermination(timeout) over a hand-rolled flag: it already handles interruption, queue drain and rejection.

Also make the flag write the last thing the shutdown path does, after the queue stops accepting new work, so a worker cannot exit while items are still queued.

3. Diagnose the hang and propose a fix, including how you would detect this class of problem in production.

Read the worked answer

Deadlock needs a cycle: reserve() holds stock and wants ledger while reconcile() holds ledger and wants stock. With a 32-thread pool, two blocked threads eventually consume enough requests that the pool stops making progress - the symptom looks like 'pool drained', not like a deadlock.

Fixes, in order of preference:

Global lock order. Define one documented order (always stock, then ledger) and enforce it in every method. Zero runtime cost; the risk is future code reintroducing the reverse order, so encapsulate both locks in one class and never expose them.
Remove the nesting. Read what you need under the first lock, release it, then acquire the second; or collapse the two structures into a single guarded object so only one lock exists.
tryLock(timeout) with backoff. Breaks the cycle but introduces livelock and partial-work rollback complexity; reserve it for cases where ordering is genuinely impossible, such as locks taken in a callback.

Detection: ManagementFactory.getThreadMXBean().findDeadlockedThreads() on a scheduled probe, or jstack, which prints 'found one Java-level deadlock' with both monitors and thread names. Put that command in the runbook. Also never call overridable methods or fire listeners while holding a lock - that is how the second lock sneaks in unnoticed.