code Review/concurrency/Race Conditions and Thread Safety: Catching Concurrency Defects in Code Review

concurrency

Race Conditions and Thread Safety: Catching Concurrency Defects in Code Review

Spot data races, check-then-act bugs, unjoined background work, and missing synchronisation in code review, with worked Python, Java, Go, and C# examples.

Reviewer Detection Checklist

0 of 12 verified (0%)

Defect Patterns & Fixes

Unsynchronised global queue mutated by a worker pool

python

Scenario: A payment retry worker is started with four threads. Each thread walks the shared retry buffer and removes entries it has completed.

RETRY_QUEUE: List[Dict[str, Any]] = []


class PaymentRetryWorker:
    def enqueue_retry(self, transaction_id: str, amount: float) -> None:
        RETRY_QUEUE.append({"transaction_id": transaction_id,
                            "amount": amount, "attempts": 0})

    def process_queue(self) -> None:
        # Runs in 4 threads concurrently.
        for item in RETRY_QUEUE:
            if item["attempts"] >= 3:
                RETRY_QUEUE.remove(item)
                continue
            item["attempts"] += 1
            if self._execute_payment(item):
                RETRY_QUEUE.remove(item)

Why this is defective: Two independent defects share one root cause. list.append and list.remove from four threads race, so entries are dropped or processed twice — and a duplicate _execute_payment on a payment is a double charge. Separately, RETRY_QUEUE.remove(item) shrinks the list while for item in RETRY_QUEUE is iterating it by index, so the iterator skips the element after every removal even in a single thread. The GIL does not help: it guarantees that individual bytecodes do not interleave, not that your read-modify-write is atomic.

How to Spot It in Reviews:

•A module-level mutable collection is a shared-state marker — check the PR description for a worker or thread count.
•remove, pop, or del inside a for loop over the same collection is a bug even before you consider threads.
•item['attempts'] += 1 is a read and a write on an object two threads may hold at once.
•Ask what happens if the same item is executed twice — if the answer involves money or email, the severity is critical, not medium.

Check-then-act on a thread-safe map

java

Scenario: A rate limiter keeps per-tenant request counts in a ConcurrentHashMap shared by every request thread.

private final ConcurrentHashMap<String, Integer> counts = new ConcurrentHashMap<>();

public boolean allow(String tenantId, int limit) {
    Integer current = counts.get(tenantId);
    if (current == null) {
        counts.put(tenantId, 1);
        return true;
    }
    counts.put(tenantId, current + 1);
    return current + 1 <= limit;
}

Why this is defective: ConcurrentHashMap makes get atomic and put atomic. It says nothing about the gap between them. Two threads can both read 41 and both write 42, so one request is never counted and a tenant sails past the limit. The map being thread-safe is exactly what makes this defect easy to miss in review — the type name reassures the reader. computeIfAbsent plus an atomic value type collapses the read and the write into one operation.

How to Spot It in Reviews:

•Any get and put on the same key within one method is check-then-act, whatever the map type.
•Look for the pattern map.put(key, map.get(key) + 1) — it is the canonical lost update.
•A concurrent collection in the diff is a signal the author knew about threads, so the compound operations deserve more scrutiny, not less.
•The correct fix is a one-liner (merge, compute, computeIfAbsent); if the author reaches for synchronized around the whole method, that is a throughput problem in waiting.

Goroutines launched and never waited for

Scenario: A batch importer fans out per-file parsing across goroutines and then returns the accumulated result map.

func (p *Importer) ImportAll(paths []string) map[string]int {
    stats := make(map[string]int)

    for _, path := range paths {
        go func() {
            n := p.parse(path)          // captures the loop variable
            stats[path] = n             // concurrent map write
        }()
    }

    return stats                        // returns before any goroutine finishes
}

Why this is defective: Three defects stack up. The function returns immediately, so callers almost always receive an empty or partial map and treat it as a real answer — a silent wrong result, not a crash. Writes to a plain map from several goroutines are a data race that the Go runtime may abort the process for (fatal error: concurrent map writes). And on Go versions before 1.22 the closure captures the loop variable itself, so every goroutine parses the last path. The empty-result defect is the one to lead with in a review comment, because it is wrong even when the race does not trigger.

How to Spot It in Reviews:

•go func() with no sync.WaitGroup, channel receive, or errgroup in the same function.
•A shared map or slice written from inside a goroutine without a mutex.
•A closure with no parameters that references the loop variable — pass it in explicitly or rely on Go 1.22+ semantics deliberately, not accidentally.
•Trace the return value: if the caller reads data the goroutines are still producing, the function has an ordering bug regardless of locking.

Fire-and-forget task with a discarded result

csharp

Scenario: An order controller kicks off inventory reservation asynchronously so the HTTP response is not delayed.

[HttpPost("orders")]
public async Task<IActionResult> Create(OrderRequest request)
{
    var order = await _orders.CreateAsync(request);

    // Do not block the response on inventory.
    _ = _inventory.ReserveAsync(order.Id);

    return Ok(order);
}

Why this is defective: A discarded Task has no owner. If ReserveAsync throws, the exception surfaces on a finaliser thread long after the request is gone, or is swallowed entirely — the order exists and the stock was never reserved, with nothing in the logs joining the two facts. Worse, ASP.NET may tear down the request scope (and its DbContext) while the task is still using it, producing ObjectDisposedException under load only. async void handlers have the same shape and the same failure mode. The fix is not to await it inline (that reintroduces the latency the author was avoiding) but to give the work a durable owner.

How to Spot It in Reviews:

•Grep for _ = in front of a call returning Task, and for async void on anything that is not an event handler.
•Check whether the discarded task touches a scoped dependency such as a DbContext or an HttpContext.
•Ask where a failure would be observed. If the answer is 'nowhere', it is a defect even when nothing races.
•Watch for the same pattern spelled Task.Run(() => ...) with no stored task and no ContinueWith.

Why concurrency defects are the most valuable thing you can catch in review

A race condition is the only class of defect that is cheaper to find in review than in any other phase. It survives unit tests, because tests run one thread at a time. It survives staging, because staging has one user. It survives the first week of production, because it needs a specific interleaving of two threads at a specific microsecond. Then traffic doubles, the scheduler makes a different choice, and you get a corrupted balance that nobody can reproduce.

Every other defect class has a second line of defence. A null dereference gets caught by a smoke test. A slow query gets caught by an APM dashboard. A data race gets caught by nothing, so the reviewer is the last checkpoint before it becomes a permanent, intermittent, unreproducible production mystery.

What it costs in production

Concurrency bugs do not fail loudly. They fail quietly and wrongly, which is worse:

Lost updates. Two workers read a counter as 41, both write 42. One increment vanishes. The ledger is off by one cent, then by one thousand.
Double side effects. Two threads pass the same if not already_sent check and the customer gets charged twice, or gets two "your order shipped" emails.
Torn state. A list is resized while another thread is indexing into it. In Python you get skipped items, in C++ you get a segfault, in Java you get an ArrayIndexOutOfBoundsException on a line that "cannot" throw.
Deadlock under load only. Two locks taken in opposite orders in two code paths. The paths only overlap at peak traffic, so the service wedges at 9am on Monday and recovers when you restart it.
Silent empty results. Work is dispatched asynchronously and the result is read before the work finishes, so the caller gets an empty aggregate and treats it as a real answer.

The operational signature is always the same: low reproduction rate, no stack trace pointing at the real culprit, and an incident review that ends with "we added a retry".

How to spot it in review

You do not need to simulate the interleaving in your head. You need to find shared mutable state and then ask two questions about it.

Step 1 — find the shared state. Scan the diff for anything that outlives a single request or task:

module-level or static collections, counters, caches, and maps
fields on a singleton, a Spring @Service, a Django app-level object, or anything registered once and reused
objects handed to a thread pool, goroutine, Task, or ExecutorService and also retained by the caller
rows in a database read and written without a transaction, a version column, or a conditional update

Step 2 — ask "is this operation one step or two?" Most concurrency bugs are a read and a write masquerading as a single line. count += 1, cache[k] = cache.get(k) + 1, if k not in d: d[k] = [], if not exists(): create() — all of these are check-then-act. A thread-safe container does not save you here: ConcurrentHashMap guarantees that each individual call is atomic, never that your get and your put happen together.

Step 3 — ask "does anyone wait for this?" Any submit, apply_async, go func(), Task.Run, or _ = doWork() that is not joined, awaited, or collected is a defect the moment the caller uses a result that depends on it.

Language-specific tells worth grepping for:

Python: a mutable module-level list/dict, apply_async without close()/join(), mutation of a list while iterating it, and the assumption that the GIL makes += atomic (it does not).
Java: non-final mutable fields on a shared bean, HashMap in a @Service, check-then-act on ConcurrentHashMap, SimpleDateFormat as a shared field, lazy init without volatile.
Go: a closure capturing a loop variable, writes to a plain map from multiple goroutines, a WaitGroup that is never waited on, sending on a channel nobody reads.
C#: async void, a discarded Task, .Result/.Wait() on a captured context, and a static field mutated per-request.
Rust: the compiler stops data races, so the bugs move up a level — a MutexGuard held across an .await, or lock ordering between two Mutex values.

Tip

If the PR description contains the words "worker", "async", "batch", "pool", "cache", or "background", assume shared state exists and go find it. That one heuristic catches most of them.

Fixing it without breaking something else

A concurrency fix changes the timing of the system, so it can trade a rare correctness bug for a constant performance bug. Before you endorse a fix, check these:

A lock around too much code becomes a throughput ceiling. If the critical section contains an HTTP call or a database round-trip, you have serialised the whole service on that call. The fix is to shrink the critical section, not to remove the lock.
A second lock introduces lock ordering. Any change that adds a lock to a path that already holds one needs a stated ordering, or you have shipped a deadlock.
Adding join()/await makes a fast path slow. Code that returned immediately now blocks for the duration of the work. Check the caller's timeout and whether it is on a request thread.
Swapping a plain dict for a synchronized/manager dict adds cost per access. In a hot loop, an IPC-proxied dictionary can be orders of magnitude slower than the local one it replaced.
Moving to a per-thread copy changes semantics. A threading.local counter fixes the race by making the number wrong in a different way — each thread now counts only its own work.
Database-level fixes must match the isolation level. SELECT ... FOR UPDATE, optimistic version columns, and UPDATE ... WHERE version = ? all behave differently under read-committed versus repeatable-read.

How not to over-engineer the fix

This is where reviews go wrong in the opposite direction. The reviewer is right that there is a race, and then asks for a distributed lock, an actor framework, or an event-sourced rewrite. The bar for a concurrency fix is: the smallest construct that makes the operation atomic.

The ladder, cheapest first:

Delete the shared state. Most shared state is accidental. If each worker can accumulate locally and merge once at the end, there is no race to fix and no lock to contend on.
Use an atomic primitive. AtomicLong, Interlocked.Increment, sync/atomic, itertools.count — one line, no lock, no deadlock risk.
Use the concurrent collection's compound operation. computeIfAbsent, merge, putIfAbsent, GetOrAdd, sync.Map.LoadOrStore exist precisely so you do not write check-then-act.
Use one narrow lock. A single Lock/Mutex around three lines is a completely respectable fix.
Only then consider queues, single-writer designs, or a database constraint.

Things that are almost never the right ask in a code review comment: a distributed lock (Redis/ZooKeeper) for state that lives in one process; a full actor library to protect one counter; a rewrite to immutable data structures because one field was mutated. If the state is in one process, the fix is in one process.

Important

A unique database constraint is often a better "lock" than a lock. If the real invariant is "one payment per order", enforce it where it is true for every instance of the service, not in the memory of one of them.

Comment templates that an agent can act on

The exercise you are practising for scores comments on whether an AI coding agent could act on them without asking a follow-up question. For concurrency, that means naming the shared object, the two operations that are not atomic, and the smallest fix.

"RETRY_QUEUE is a plain list mutated from 4 worker threads — append and remove race. Replace with queue.Queue."
"get then put on counters is check-then-act; two threads lose an increment. Use counters.merge(key, 1L, Long::sum)."
"pool.apply_async is never joined, so dict(self.shared_stats) returns before the workers finish. Add pool.close(); pool.join() before the return."
"This goroutine captures the loop variable item; all iterations see the last value. Pass it as an argument to the closure."

Compare with a comment that will not score: "possible race condition here, might want to add locking". It names no object, no pair of operations, and no fix.

Next Guide

Logic and State Mutation Bugs: Finding Correctness Defects in a Pull Request

Interactive Practice

Review Defective Code in the Workbench

Test your ability to spot this defect in our interactive Monaco-powered PR code editor.

Open Workbench