Safe Thread Synchronization

Question

I think the following is thread safe. Am I right? How would you improve this code? Are there new APIs or methods I can use?

Data:

public class Data {
  private int value;
  public Data(int value) {
    this.value = value;
  }

  public synchronized int getValue(int mTrhead) {
    System.out.println("MyTrhead " + mTrhead + " gets value " + value);
    return value;
  }

  public synchronized void setValue(int value, int mTrhead) {
    System.out.println("MyTrhead " + mTrhead + " sets value " + value);
    this.value = value;
  }
}

MyTrhead:

public class MyTrhead extends Thread {
  private Data mData;
  private int id;

  public MyTrhead(Data mData, int id) {
    this.mData= mData;
    this.id = id;
  }

  @Override
  public synchronized void run() {
    try {
      for (int a = 1; a <= 10; a++) 
      {
        mData.setValue(mData.getValue(id) + 1, id);
      }
      Thread.sleep(1000);
    } catch (InterruptedException e) {
      e.printStackTrace();
    }
  }
}

Answer 1

Is it thread-safe? Yes

Is it going to do what you expect? Probably not

Is there a better way? Yes.

Safe

Your code will correctly lock, and access the mData value each time, and this makes it thread safe.... but, you will not get the results you expect.

Expectations

I suspect that your plan is to start, say, 10 threads, each of them incrementing from 0 through 9, adding 1 each time. You would expect the end result for the value to be 100.

It may not be. This is because, while your code is thread-safe, it is not atomic.

Atomic means that an operation starts and completes as a single logical step. You have the code:

mData.setValue(mData.getValue(id) + 1, id);

The apparent expectation is that you get the value, add one, and then set the new value. You would expect the value to increment from 0 through 10.

But, in a multi-threaded system, it may not. This is because two threads may each call mData.getValue(...) before the other has then reset the value... For example, consider the following:

Time      mData value  Thread 1  Thread 2
========  ===========  ========  ========

Initial             0

t1 get              0         0
t1 add              0         1
t1 set              1         1

t1 get              1         1
t1 add              1         2
*t2* get            1                   1
t1 set              2         2
t2 add              2                   2
t2 set              2                   2

Notice how the thread-2 can get in after the thread-1 has got the value, and before it has set the value?

What you need is an atomic operation, one which gets, adds, and sets the values all in one locked process. You can do it with your code using:

  for (int a = 1; a <= 10; a++) {
      synchronized(mData) {
          mData.setValue(mData.getValue(id) + 1, id);
      }
  }

That way all the get, add, and set operations are in a single synchronized block.

Better way

Note, in the above code, that we are synchronizing the block on the mData:

synchronized(mData) {
    ....

This illustrates a potential bug in your usage.... anyone can synchronize on your class. This may, or may not be a good thing. Your get and set methods are already synchronized on the mData instance, so now you have multiple classes all using the same synchronization point ("monitor").

The bottom line is that Java already identifies atomic operations to be a problem, and there is a subset of the standard library dedicated to ensuring that Atomic operations are safe. In your case, use AtomicInteger