Is my class thread-safe with multiple threads accessing its variables?

Question

As I am trying to get to grips with thread safety, I would like to know if this class is threadsafe? If two threads try to access its variables at any time it seems to me that it is thread safe because:

• final variable is threadsafe by virtue of being immutable.
• getter and setter are synchronized, so mObject can only be gotten, or set by one thread at a time. So there is no chance of two threads reading different values.
• The method changeObj() is not synchronized but any block in it which deals with class variables (i.e. mObject) is synchronized.

Please tell me if I am wrong or if this class is not threadsafe.

public class MyClass{

   private final String = "mystring"; //thread safe because final
   private AnObject mObject;

   public MyClass(){
       //initialize
   }

   //only one class can set the object at a time.
   public synchronized void setObj(AnObject a){
      mObject = a;
   }

    //two threads trying to get the same object will not read different values.
   public synchronized AnObject getObj(){
      return mObject;
   }

   //better to synchronize the smallest possible block than the whole method, for performance.
   public void changeObj(){
       //just using local variables (on stack) so no need to worry about thread safety
       int i = 1;
       //i and j are just to simulate some local variable manipulations.
       int j =3;
       j+=i;
       synchronized(this){
          //change values is NOT synchronized. Does this matter? I don't think so.
          mObject.changeValues();
      }
   }
}

Answer 1

No, it's not thread safe. You make sure only one thread can change the values in your AnObject at a time if it uses the changeObj() method, but you also provide a getter for this object, so any other thread could call changeValues() concurrently.

Answer 2

Your class in itself is thread safe in its current state (assuming any methods not shown here are), however you have one likely "bug" in your thinking;

mObject isn't 100% encapsulated, it is passed in through a setter and can be fetched through a getter. That means that any thread could get a reference to and call methods on the object referenced by mObject simultaneously without MyClass knowing about it.

In other words, AnObject's methods may need to be thread safe too.

At the very least, the synchronizing in MyClass doesn't in any way make mObject safe from threading problems.