Code Review: This Lock-Free Counter
A candidate's 'lock-free counter' looks slick, atomic operations, no synchronized blocks. But the get-then-set pattern isn't atomic, the read isn't ordered with the write, and overflow handling is missing. Walk through what an interviewer wants reviewers to find.
The setup
An interviewer hands the candidate a snippet labeled LockFreeCounter and says: "A candidate wrote this. They claim it's thread-safe. Walk me through what to say to them in code review."
This is the Code Review Round, a format that's increasingly common at senior+ interviews. The interviewer wants to see an eye for concurrency bugs and how the candidate communicates corrections without being condescending. Thirty minutes. The bugs are real.
The interviewer is testing two things
- Can the candidate spot the bugs? Specifically the "looks atomic but isn't" patterns.
- Can the candidate explain why they're bugs? Not just "this is wrong", articulate the interleaving that produces the wrong outcome. The interviewer wants to know whether the candidate would be useful in their team's code reviews.
How to read the broken code
Pick the relevant language tab. Read the entire class first, get the shape. Then for each method, ask:
- Does this method do exactly one read or write? If yes, atomic or volatile is enough.
- Does it do check-then-act (read X, decide based on X, modify X)? If yes, it must be a CAS retry OR fully locked. Get-then-set is broken.
- Does it release the lock anywhere between a read and the dependent write? If yes, the check is stale by the time the write runs.
The mental sentence "If this method gets interrupted between line N and line N+1, what state can another thread leave the counter in?" If the answer makes the post-condition wrong, it's a bug.
The bugs in this snippet
All three methods have the same family of bug, check-then-act without atomicity.
| Method | Bug | Fix |
|---|---|---|
incrementIfBelow(max) | Two threads can both pass the check at max-1; both increment; value becomes max+1. | CAS retry on (current, current+1) |
resetIfAbove(threshold) | Reset can fire even when value has dropped below threshold by the time set(0) runs. | CAS retry, or accumulateAndGet |
tryDecrement() | Two threads can both read 1, both decrement, value becomes -1. | CAS retry on (current, current-1) |
How to communicate this in the interview
The structure interviewers want
"Looking at this, I see three bugs of the same shape, check-then-act races. Let me walk through incrementIfBelow specifically. Suppose two threads call it concurrently when value is max - 1..."
Then trace the interleaving step by step. Then propose the fix, CAS retry, and explain why CAS is atomic where get+set isn't. Then say "the same fix applies to the other two methods."
What kills candidates: jumping to the fix without showing the reasoning. The interviewer wants to see whether the candidate can teach a teammate, not just hand them code.
After the review
Once the bugs are fixed, ask: does this even need to be lock-free? For low-contention counters, a synchronized method or a Lock is simpler, more readable, and fast enough. Lock-free is a tool for extreme contention, using it as a default is over-engineering.
The senior signal Junior engineers spot the bugs and propose fixes. Senior engineers spot the bugs, propose fixes, AND ask "should this be lock-free at all?" The latter shows command of the tradeoffs, not just the techniques.
Implementations
A candidate claims this counter is thread-safe and lock-free. Read carefully and identify the bugs before reading the analysis. There are at least 3.
1 class LockFreeCounter {
2 private final AtomicInteger value = new AtomicInteger(0);
3
4 public int incrementIfBelow(int max) {
5 if (value.get() < max) {
6 return value.incrementAndGet();
7 }
8 return -1; // already at max
9 }
10
11 public void resetIfAbove(int threshold) {
12 if (value.get() > threshold) {
13 value.set(0);
14 }
15 }
16
17 public boolean tryDecrement() {
18 int current = value.get();
19 if (current > 0) {
20 value.set(current - 1);
21 return true;
22 }
23 return false;
24 }
25 }Bug 1 (incrementIfBelow): classic check-then-act race. Two threads can both read value.get() == max - 1, both call incrementAndGet(), value becomes max + 1. Fix: CAS retry loop. Bug 2 (resetIfAbove): race between get() and set(). Between them, another thread can write a value below threshold; reset still fires. Fix: CAS to install 0 only if value still matches the original above-threshold value, though semantics may need clarification. Bug 3 (tryDecrement): same get-then-set race as decrement; can decrement past 0. Fix: CAS retry. Also missing: handling overflow, no documentation of contract, no accumulateAndGet for higher-level reads.
1 class LockFreeCounter {
2 private final AtomicInteger value = new AtomicInteger(0);
3
4 // Fix: CAS retry, atomically check AND increment
5 public int incrementIfBelow(int max) {
6 while (true) {
7 int current = value.get();
8 if (current >= max) return -1;
9 if (value.compareAndSet(current, current + 1)) {
10 return current + 1;
11 }
12 // CAS failed, another thread won; retry
13 }
14 }
15
16 // Fix: accumulateAndGet handles the predicate atomically
17 public void resetIfAbove(int threshold) {
18 value.accumulateAndGet(0, (curr, zero) -> curr > threshold ? 0 : curr);
19 }
20
21 // Fix: CAS retry
22 public boolean tryDecrement() {
23 while (true) {
24 int current = value.get();
25 if (current <= 0) return false;
26 if (value.compareAndSet(current, current - 1)) {
27 return true;
28 }
29 }
30 }
31 }Key points
- •An atomic.get() followed by atomic.set() is NOT atomic, same race as ++ on volatile
- •Compound operations need compareAndSet (CAS) or accumulateAndGet, not get+set
- •Reading 'unconditionally' before checking is a read-then-act race
- •Overflow on counters that monotonically increase is a real production bug
Follow-up questions
▸Why is 'check-then-act' such a common bug?
▸When is atomic.get() followed by atomic.set() acceptable?
▸Why does the candidate's code call itself 'lock-free'?
▸Should this code be lock-free at all?
Gotchas
- !atomic.get() then atomic.set() is two operations, not atomic
- !atomic.set(atomic.get() + 1) is the textbook racy increment
- !Reading outside a lock then acting inside it, if the read informed the action, it's a race
- !Naming a class 'LockFree' doesn't make it correct
- !Java's accumulateAndGet/updateAndGet build CAS retry loops automatically, prefer them when applicable
Code review interviews at Google, Stripe, Cloudflare. Detecting check-then-act races is one of the most reliable signals of senior-level concurrency understanding. Lots of production code has these bugs because tests don't catch them.