Are java getters thread-safe?

Question

Is is okay to synchronize all methods which mutate the state of an object, but not synchronize anything which is atomic? In this case, just returning a field?

Consider:

public class A
{
   private int a = 5;

   private static final Object lock = new Object();

   public void incrementA()
   {
       synchronized(lock)
       {
           a += 1;
       }
   }

   public int getA()
   {
       return a;
   }
}

I've heard people argue that it's possible for getA() and incrementA() to be called at roughly the same time and have getA() return to wrong thing. However it seems like, in the case that they're called at the same time, even if the getter is synchronized you can get the wrong thing. In fact the "right thing" doesn't even seem defined if these are called concurrently. The big thing for me is that the state remains consistent.

I've also heard talk about JIT optimizations. Given an instance of the above class and the following code(the code would be depending on a to be set in another thread):

while(myA.getA() < 10)
{
    //incrementA is not called here
}

it is apparently a legal JIT optimization to change this to:

int temp = myA.getA();
while(temp < 10)
{
    //incrementA is not called here
}

which can obviously result in an infinite loop. Why is this a legal optimization? Would this be illegal if a was volatile?

Update

I did a little bit of testing into this.

public class Test
{
   private int a = 5;

   private static final Object lock = new Object();

   public void incrementA()
   {
       synchronized(lock)
       {
           a += 1;
       }
   }

   public int getA()
   {
       return a;
   }

   public static void main(String[] args)
   {
       final Test myA = new Test();
       Thread t = new Thread(new Runnable(){
        public void run() {
            while(true)
            {
                try {
                    Thread.sleep(100);
                } catch (InterruptedException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
                myA.incrementA();
            }
        }});
       t.start();
       while(myA.getA() < 15)
       {
           System.out.println(myA.getA());
       }
   }
}

Using several different sleep times, this worked even when a is not volatile. This of course isn't conclusive, it still may be legal. Does anyone have some examples that could trigger such JIT behaviour?

Answer 1

Is is okay to synchronize all methods which mutate the state of an object, but not synchronize anything which is atomic? In this case, just returning a field?

Depends on the particulars. It is important to realize that synchronization does two important things. It is not just about atomicity but it is also required because of memory synchronization. If one thread updates the a field, then other threads may not see the update because of memory caching on the local processor. Making the int a field be volatile solves this problem. Making both the get and the set method be synchronized will as well but it is more expensive.

If you want to be able to change and read a from multiple threads, the best mechanism is to use an AtomicInteger.

private AtomicInteger a = new AtomicInteger(5);
public void setA(int a) {
   // no need to synchronize because of the magic of the `AtomicInteger` code
   this.a.set(a);
}
public int getA() {
    // AtomicInteger also takes care of the memory synchronization
    return a.get();
}

I've heard people argue that it's possible for getA() and setA() to be called at roughly the same time and have getA() return to wrong thing.

This is true but you can get the wrong value if getA() is called after setA() as well. A bad cache value can stick forever.

which can obviously result in an infinite loop. Why is this a legal optimization?

It is a legal optimization because threads running with their own memory cache asynchronously is one of the important reasons why you see performance improvements with them. If all memory accesses where synchronized with main memory then the per-CPU memory caches would not be used and threaded programs would run a lot slower.

Would this be illegal if a was volatile?

It is not legal if there is some way for a to be altered – by another thread possibly. If a was final then the JIT could make that optimization. If a was volatile or the get method marked as synchronized then it would certainly not be a legal optimization.

Answer 2

It's not thread safe because that getter does not ensure that a thread will see the latest value, as the value may be stale. Having the getter be synchronized ensures that any thread calling the getter will see the latest value instead of a possible stale one.

Answer 3

You basically have two options:

1) Make your int volatile 2) Use an atomic type like AtomicInt

using a normal int without synchronization is not thread safe at all.

Answer 4

Your best solution is to use an AtomicInteger, they were basically designed for exactly this use case.

If this is more of a theoretical "could this be done question", I think something like the following would be safe (but still not perform as well as an AtomicInteger):

public class A
{
   private volatile int a = 5;

   private static final Object lock = new Object();

   public void incrementA()
   {
       synchronized(lock)
       {
           final int tmp = a + 1;
           a = tmp;
       }
   }

   public int getA()
   {
       return a;
   }
}

Answer 5

The short answer is your example will be thread-safe, if

• the variable is declared as volatile, or
• the getter is declared as synchronized.

The reason that your example class A is not thread-safe is that one can create a program using it that doesn't have a "well-formed execution" (see JLS 17.4.7).

For instance, consider

  // in thread #1
  int a1 = A.getA();

  Thread.sleep(...);

  int a2 = A.getA();

  if (a1 == a2) {
      System.out.println("no increment");

  // in thread #2
  A.incrementA();

in the scenario that the increment happens during the sleep.

For this execution to be well-formed, there must be a "happens before" (HB) chain between the assignment to a in incrementA called by thread #2, and the subsequent read of a in getA called by thread #1.

• If the two threads synchronize using the same lock object, then there is a HB between one thread releasing the lock and a second thread acquiring the lock. So we get this:

   thread #2 acquires lock --HB-->
   thread #2 reads a --HB-->
   thread #2 writes a --HB-->
   thread #2 releases lock --HB-->
   thread #1 acquires lock --HB-->
   thread #1 reads a
  

• If two threads share a a volatile variable, there is a HB between any write and any subsequent read (without an intervening write). So we typically get this:

   thread #2 acquires lock --HB-->
   thread #2 reads a --HB-->
   thread #2 writes a --HB-->
   thread #1 reads a
  

  Note that incrementA needs to be synchronized to avoid race conditions with other threads calling incrementA.

• If neither of the above is true, we get this:

   thread #2 acquires lock --HB-->
   thread #2 reads a --HB-->
   thread #2 writes a                  // No HB!!
   thread #1 reads a
  

  Since there is no HB between the write by thread #2 and the subsequent read by thread #1, the JLS does not guarantee that the latter will see the value written by the former.

Note that this is a simplified version of the rules. For the complete version, you need to read all of JLS Chapter 17.