Understanding Java Synchronization: The Hidden Race Condition

I recently posted a Java puzzle on LinkedIn that got some responses. Most folks were able to spot the issue right away, which was awesome to see! Here's a quick breakdown of what the problem was all about.

The Code in Question

I shared this Counter class and asked if anyone could spot the problem:

public class Counter {
    private Integer count = 0;
    
    public void increment() {
        synchronized (count) {
            count++;
        }
    }
}        

Why It's Broken

This code is broken because synchronization requires all threads to lock on the same object, but here each thread ends up locking on a different object.

Here's why:

1. Initial state: count = 0 (Integer object #1)

2. Thread A enters increment():
   - Acquires lock on Integer object #1 (value 0)
   - Executes count++ which:
     * Unboxes to primitive: int temp = count.intValue() → 0
     * Increments: temp = temp + 1 → 1  
     * Autoboxes back: count = Integer.valueOf(1) → creates NEW Integer object #2
   - Releases lock on Integer object #1

3. Thread B enters increment():
   - Tries to acquire lock on current count which is now Integer object #2 (not #1!)
   - Successfully acquires lock because no other thread is holding it
   - Both threads can now execute "synchronized" code simultaneously        

The fundamental problem: Using a mutable reference as a lock. The lock object keeps changing, so there's no mutual exclusion.

Vladimir Marjanovic nailed it in the comments, explaining how this creates a race condition because of the changing lock object.

If you want to see the issue, run the following code:

package ca.bazlur.concurrency;

import java.util.concurrent.CountDownLatch;


public class Counter {
  private Integer count = 0;

  public void increment() {
    synchronized (count) {
      count++;
    }
  }

  public Integer getCount() {
    return count;
  }

  private static final int THREADS = 100;
  private static final int INCREMENTS_PER_THREAD = 10_000;

  public static void main(String[] args) throws InterruptedException {
    Counter counter = new Counter();
    CountDownLatch start = new CountDownLatch(1);
    CountDownLatch done = new CountDownLatch(THREADS);
    System.out.println(counter.count);

    Runnable task = () -> {
      try {
        start.await();          // start all threads at once
        for (int i = 0; i < INCREMENTS_PER_THREAD; i++) {
          counter.increment();
        }
      } catch (InterruptedException ignored) {
      } finally {
        done.countDown();
      }
    };

    for (int i = 0; i < THREADS; i++) Thread.ofPlatform().start(task);

    start.countDown();              // go!
    done.await();                   // wait for completion

    int expected = THREADS * INCREMENTS_PER_THREAD;
    System.out.printf("expected = %,d  actual = %,d%n", expected, counter.getCount());
  }
}        

You will get something like this:

expected = 1,000,000 actual = 625,186

The Fix

The solution is simple—just synchronize on the Counter instance instead:

public class Counter {
    private Integer count = 0;
    
    public void increment() {
        synchronized (this) {
            count++;
        }
    }
}        

Now all threads lock on the same object, and we get proper thread safety.

Even Better: Use AtomicInteger

For this specific case, AtomicInteger is the way to go:

public class Counter {
    private AtomicInteger count = new AtomicInteger(0);
    
    public void increment() {
        count.incrementAndGet();
    }
}        

No explicit synchronization needed!

The Takeaway

Always make sure you're synchronizing on an object that won't change during execution. It's one of those bugs that can slip past code reviews because the synchronized block looks correct at first glance.