Multithreading - sont des accesseurs get java thread-safe ? -Débordement de pile

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?

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 it is called later as well. A bad cache value can stick for ever.

which can obviously result in an infinite loop. Why is this a legal optimization? 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.

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.

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.

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;
   }
}

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?

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 it is called later as well. A bad cache value can stick for ever.

which can obviously result in an infinite loop. Why is this a legal optimization? 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.

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.

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.

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;
   }
}

Source