Why volatile and synchronized statements cannot avoid thread interference here?

Question

package simple;

public class ThreadInterference {
    public static volatile Integer count = 1000;

    public static class MyThread implements Runnable {
        @Override
        public void run() {
            try {
                Thread.sleep(1000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            for (int i = 0; i < 10000; i++) {
                count++;
                count--;
                count++;
                count--;
            }
        }
    }

    public static void main(String[] args) throws InterruptedException {
        System.out.println(count);

        Thread t1 = new Thread(new MyThread());
        Thread t2 = new Thread(new MyThread());
        Thread t3 = new Thread(new MyThread());
        Thread t4 = new Thread(new MyThread());

        t1.start();
        t2.start();
        t3.start();
        t4.start();

        t1.join();
        t2.join();
        t3.join();
        t4.join();

        System.out.println(count);

    }

}

The count variable is marked as volatile but the output is:

1000
1230

If I change to synchronized statements, thread interference also happens:

package simple;

public class ThreadInterference {
    public static Integer count = 1000;

    public static class MyThread implements Runnable {
        @Override
        public void run() {
            try {
                Thread.sleep(1000);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            for (int i = 0; i < 10000; i++) {
                synchronized(count) {
                    count++;
                    count--;
                    count++;
                    count--;
                }
            }
        }
    }

    public static void main(String[] args) throws InterruptedException {
        System.out.println(count);

        Thread t1 = new Thread(new MyThread());
        Thread t2 = new Thread(new MyThread());
        Thread t3 = new Thread(new MyThread());
        Thread t4 = new Thread(new MyThread());

        t1.start();
        t2.start();
        t3.start();
        t4.start();

        t1.join();
        t2.join();
        t3.join();
        t4.join();

        System.out.println(count);

    }

}

output of this time is:

1000
1008

Why?

Answer 1

When you are doing this, you are doing it wrong.

        public static volatile Integer count = 1000;
        for (int i = 0; i < 10000; i++) {
            synchronized(count) {
                count++;
                count--;
                count++;
                count--;
            }
        }

Integer is immuttable. That means, when you are doing count++, it created a new Integer object, and assigned count with that new object. Meanwhile, synchronized part still reference the old object. So there exist a chance when every thread reached synchronized block with different object. You should use variable that doesn't point to different object when the process happened.

Answer 2

As others have noted, you have two separate problems. The first one is that count++ (and count--) isn't an atomic operation, even for primitive int types. So this won't work without some sort of locking or other concurrency handling. For count++ where count is an int, the compiler generates something like the following byte code instructions:

    getstatic count
    iconst_1
    iadd
    putstatic count

This is not likely to be atomic even if/when compiled to native code.

The second problem is that you are not locking on a consistent object, so operations are not serialised. The code:

   count++; // "count" is an "Integer" object type.

Creates a new object. In essence it does something like the following:

   count = Integer.valueOf(count.intValue() + 1);

So your count object is getting replaced by a new object, and subsequent entry into the synchronized section will be synchronizing against a different object.

As a safety tip, if you are using synchronized (someObject), where someObject is a class or instance field, then it's a good idea to make that field final. That way it can't be inadvertently reassigned to a different value.

There are two straightforward solutions to the problem that I can think of. One with locking against a specific object used for locking like so:

public class ThreadInterference {
  public static final Object COUNT_LOCK = new Object();
  public static int count = 1000; // Either "int" or "Integer" OK, depending on need

  public static class MyThread implements Runnable {
    @Override
    public void run() {
      try {
        Thread.sleep(1000);
      } catch (InterruptedException e) {
        e.printStackTrace();
      }
      for (int i = 0; i < 10000; i++) {
        synchronized(COUNT_LOCK) {
          count++;
          count--;
          count++;
          count--;
        }
      }
    }
  }

  // And so on...
}

Another option is to use an AtomicInteger which may give better concurrent performance, if that matters:

public class ThreadInterference {
  public static AtomicInteger count = new AtomicInteger(1000);

  public static class MyThread implements Runnable {
    @Override
    public void run() {
      try {
        Thread.sleep(1000);
      } catch (InterruptedException e) {
        e.printStackTrace();
      }
      for (int i = 0; i < 10000; i++) {
        count.incrementAndGet();
        count.decrementAndGet();
        count.incrementAndGet();
        count.decrementAndGet();
      }
    }
  }

  // And so on...
}

Answer 3

I believe what is happening is this:

count++ tries to increment an Integer, which is immutable. The compiler must create a new Integer and refer to it as "count". Synchronization locks on the former count will then behave unexpectly. Somebody really smart could probably work it out...

Change count to a primitive int and synchronize on something else to see what happens. Or use an AtomicInteger.

Main takeaway is don't increment Integers! unless you are sure.

Answer 4

Instead of synchronizing count,

Try doing increment/decrement method that are themselves synchronized...

public static synchronized decrementCount()
{
    count--;
}

public static synchronized incrementCount()
{
    count++;
}

And call them instead of directly decrementing/incrementing.

Also, you should use atomic value if you are only doing a counter which prevent thread interference without resorting to synchronization.

Anyway, check this doc which show you how to do a synchronized counter with synchronized methods or atomic values : http://docs.oracle.com/javase/tutorial/essential/concurrency/atomicvars.html