5

Let's say I have a method, that is accessed by two or more threads and I want to make it thread safe.

public int getVal(int x, int y, MyClass myObj)  
{
   int z;

   z = getInt(myObj);

   return x + y + z;  
}

In this, I think we don't have to synchronize for x + y as they are primitives.

Let's assume that getInt(myObj) modifies the state of myObj and that influences the value of z.

Therefore, I will have to provide synchronization for the line z = getInt(myObj); but only when both threads to pass same instance in 'myObj' reference. As the API's coder I would not know whether both threads would pass same instance for 'myObj' or not. In some case these threads might pass same MyClass instance in 'myObj' reference and in other cases they might pass different MyClass-instances in 'myObj' reference.

So, how would one ensure thread-safety for the line z = getInt(myObj)? (Surely, we do not want to synchronize when instances passed are different and only have to synchronize when instances passed are same. I it is clear that this cannot be determined).

Assuming that MyClass cannot be made immutable, I am thinking that the following could be a solution.

synchronized(myObj)
{
  z = getInt(myObj);
}

Is it a correct solution? And, in what other ways can we ensure thread safety for 

z = getInt(myObj); (but only in case of different instances)?
Real Red.
  • 4,991
  • 8
  • 32
  • 44

6 Answers6

3

What you have is correct. When you synchronize on an object its locking on that instance not on that class. So if I pass the same *instance* of an object to two different methods, it will lock on that object correctly. However, if I pass two different instance, there won't be any locking because the two instance have each their own lock.

Amir Raminfar
  • 33,777
  • 7
  • 93
  • 123
  • 3
    A better way is to synchronize the getInt method on your MyClass. – Amir Raminfar Mar 09 '11 at 19:36
  • What he has is correct but bad practice. By locking on myObj he could be introducing pitfalls to client code that also wants to synchronize on that object. http://stackoverflow.com/questions/442564/avoid-synchronizedthis-in-java/442601#442601 – Matt Wonlaw Mar 09 '11 at 20:49
  • True, but if he delegates the work to getInt() and uses an internal Object to do the locking I think he will be set. But yes, I agree this has a pitfall. – Amir Raminfar Mar 09 '11 at 20:52
2

If getInt doesn't modify the state of this, then the method is thread-safe. The thread-safeness of the myObj object is the responsability of its class : MyClass, or of the object that holds it. Not the responsibility of all the methods which might take it as an argument, IMHO.

Your solution (synchronized(myObj)) is right, though : two threads won't be able to execute the getInt method concurrently if the same myObj is used in both threads. They will execute concurrently if the two myObjs are different.

JB Nizet
  • 678,734
  • 91
  • 1,224
  • 1,255
  • What if someone is simultaneously calling `setInt`? You can still have problems reading if someone else might be writing. – Mark Peters Mar 09 '11 at 20:06
  • That isn't a getter (accessor), I would assume it extracts or derives something from the input, so it is not likely there is a setInt. – Robin Mar 09 '11 at 21:57
  • "The thread-safeness of the myObj object is the responsibility of its class": Is it common for Java libraries to be inherently thread-safe? Or, like the Collections library, is it assumed that the user will add locking where appropriate? – Karmastan Mar 10 '11 at 02:45
  • @Karmastan: yes, it's common for Java libraries to be thread-safe. When an object is not, then I think it should be encapsulated in another one which will handle synchronization. This object should avoid escaping the object to uncontrolled external methods which might keep a reference to this object. And the external methods should not be responsible for the synchronization. – JB Nizet Mar 10 '11 at 14:00
1
synchronized(myObj) {   z = getInt(myObj); }

will do what you intend, but synchronizing on a paremeter creates many other problems. For example, some other thread may already be syncrhonizing on that object (e.g., maybe that object has a synchronized method which is being called on it) and you could get into a deadlock case.

Synchronization should be encapsulated just like anything else. The best solution would be to add the getInt method to MyClass and synchronize on some private member within that method. This way nobody else can muck with what you are using to implement your sycnrhonization.

E.g.:

public class MyClass {
  private Object lockObject = new Object();
  public int getInt() {
    synchronized(lockObject) {
      // do your thing
    }
  }
}

See this: Avoid synchronized(this) in Java? in reference to the importance of encapsulating your synchronization.

Community
  • 1
  • 1
Matt Wonlaw
  • 12,146
  • 5
  • 42
  • 44
0

To answer "And, in what are the ways can we ensure thread safety for...but only in case of different instances", synchronize the entire method or create another, common object to act as a lock for all threads and synchronize on it instead of myObj.

Brent Worden
  • 10,624
  • 7
  • 52
  • 57
0

If the only changes on myObject in question come from this getInt method, then your synchronization is enough. If there are other modificators out there, make sure they synchronize on the same object.

Paŭlo Ebermann
  • 73,284
  • 20
  • 146
  • 210
0

I disagree with all "your synchronized is correct" answers. What if you user has 2 threads, and one of them is holding a lock on the object already? Deadlocks ensues.

Also, x + y + z is not atomic. At CPU level, it'll become

int temp = x + y;
int res = temp + z;

I'll tell you more: long1 + long2 is not atomic on 32-bit machines.

I think your only option is to synchronize the entire method.

iluxa
  • 6,941
  • 18
  • 36
  • 1
    the adding of primitives in his method is fine since primitives are pass by value. Primitive formal parameters to a method can't be shared among threads since they are pass by value. – Matt Wonlaw Mar 09 '11 at 20:37