java synchronized on method Not working?

Question

I'm experimenting Java Multi-Threading using synchronization on method comparing with Atomic variables (java.util.concurrent.atomic package).

Below are the classes:

// Interface ICounter.java
        public interface ICounter {
            public void increment();
            public void decrement();
            public int value();
        }

// Class Counter.java
    public class Counter implements ICounter {
        private int c = 0;

        @Override
        public void increment() {
            c++;
        }

        @Override
        public void decrement() {
            c--;
        }

        @Override
        public int value() {
            return c;
        }
    }

// Class AtomicCounter.java
    import java.util.concurrent.atomic.AtomicInteger;

    public class AtomicCounter implements ICounter {
        private AtomicInteger c = new AtomicInteger(0);

        @Override
        public void increment() {
            c.incrementAndGet();
        }

        @Override
        public void decrement() {
            c.decrementAndGet();
        }

        @Override
        public int value() {
            return c.get();
        }

        public long getIncrement() {
            return c.incrementAndGet();
        }
    }

// Class MainProg.java
    public class MainProg {
        public static void main(String args[]) {
            ICounter counter = new AtomicCounter();
                    //ICounter counter = new SynchronizedCounter();
            Thread thread1 = new Thread(new CountRunner(counter));
            Thread thread2 = new Thread(new CountRunner(counter));

            thread1.start();
            thread2.start();
        }
    }

    class CountRunner implements Runnable {
        private ICounter counter;
        public CountRunner(ICounter counter) {
            this.counter = counter;
        }

        public void run() {
            while (true) {
                counter.increment();
                System.out.println(Thread.currentThread().getName() + " count=" + counter.value());
                System.out.println("-------------------");
                try {
                    Thread.sleep(2000L);
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
        }
    }

The results from running either the Atomic or Synchronized do not show that the variable integer is thread-safe, e.g.:

Thread-0 count=1
-------------------
Thread-1 count=2
-------------------
Thread-0 count=3
-------------------
Thread-1 count=4
-------------------
Thread-0 count=5
-------------------
Thread-1 count=6
-------------------
Thread-0 count=7
-------------------
Thread-1 count=8
-------------------
Thread-0 count=10
-------------------
Thread-1 count=10
-------------------

From the results, the last 2 lines show the 2 threads were accessing the same value of integer variable of the counter class. Perhaps I am missing something here?

Thanks!

Answer 1

What you missed is that your AtomicCounter class does work correctly, and the observed behaviour occurs becase the threads switched between the call to .increment() and .value():

-------------------
Thread-0 increment -> value = 9
-------------------
Thread-1 increment -> value = 10
-------------------
Thread-0 print value <- 10
-------------------
Thread-1 print value <- 10
-------------------

Answer 2

because your counter.increment() and System.out.println is not one atomic action.

Thread 1               Thread2

increment

                       increment

                       System.out.println  // print 10

System.out.println  
// print 10 too

Answer 3

You're incrementing the value in one step, then getting the value in another step. While each of these individual steps is guaranteed to be atomic by the underling AtomicInteger class, the fact that you make two separate operations leaves the value you see in your print statement at the mercy of the order of execution of the threads.

To be able to accurately display the value updated by a given thread, you need to both update and get the resulting value in a single operation, which your getIncrement() method does. The code which would give you the expected results would look like this:

int changedValue = counter.getIncrement();
System.out.println(Thread.currentThread().getName() + " count=" + changedValue);