Synchronization on a non-final field.

[Maris Orbidans]

Hi

Intellij shows a warning every time I synchronize on a non-final class field. Here is an example:

    
public class X
{
  private Object o;
  
  public void setO(Object o)
  {
    this.o = o;
  }
 
  public void x()
  {
    synchronized (o) // synchronization on a non-final field
    {
    }
  }
}

This inspection reports instances of synchronized statements where the lock expression is a non-final field. Such statements are unlikely to have useful semantics, as different threads may be locking on different objects even when operating on the same object.

I don't fully understand what does it mean. I know that object o will be set only once and before method x() is invoked. Is there still a problem with such code ?

[Stan James]

For a number of threads to synchronize a block of code correctly they need to synchronize on the same object instance. That sample code would allow someone to change "o" to a new object instance between the time Thread 1 synchronizes and the time Thread 2 synchronizes. Then both threads could enter the synchronized block at the "same time". We could probably make up a scenario where this is correct behavior, but it seems a lot more likely that it is not what we would like.

Did that makes sense?

[Maris Orbidans]

Yes, that's how I understand this issue too. But in my case I am sure that nobody will change object reference o ever after it has been set. So I guess I am on the safe side.

[Jim Yingst]

There's a problem with that though - it's still possible for another thread to think that o == null after setO() has been called. That's the sort of weird thing that happens with threads if they're not properly synchronized. And in this case the synchronization only occurs after o has been accessed, which is to late to ensure that o is accessed correctly. You could fix this by making o transient, though that's bit unusual for something that isn't supposed to change. Or you could declare both setO() and x() to be synchronized - that is, synchronized on the current instance of the X class, not the o variable. In that way, you would synchronize before accessing o, and ensure that you got the correct o. For that matter, if you synchronize on the X instance, do you still need to sync on o at all? You may not need to anymore.

All in all, making o final is probably the simplest for you to do, if that's possible. We can't guess what these methods are really doing, so it's hard to offer good suggestions about what sort of redesign would make the most sense here.

[Romeo Son]

Hi Maris,

Yes, that's how I understand this issue too. But in my case I am sure that nobody will change object reference o ever after it has been set. So I guess I am on the safe side.

Maybe you know this, but you don't have to think only to yourself. Since your method setO(Object o) is public, what does prevents another developer that uses your class to call it and change the object you are synchronizing on?

If you would have declared your instance field final, the compiler would have been forced you to initialize it in the constructor, that would have made your IDE happy I guess.

[Mr. C Lamont Gilbert]

Originally posted by Maris Orbidans:
Yes, that's how I understand this issue too. But in my case I am sure that nobody will change object reference o ever after it has been set. So I guess I am on the safe side.

If this is the case then try setting its value in the constructor and/or using a factory method to set it.

[Chris Hurst]

Jim presumably you mean 'transient' in its memory sense of the word rather than as Java defines it, i.e. aren't you describing volatile, or am I amount to learn some new property of transient ;-)

PS

Yes, that's how I understand this issue too. But in my case I am sure that nobody will change object reference o ever after it has been set. So I guess I am on the safe side.

Obviously currently you'ed have the implied condition that you object was created in and your setter called within the same thread.

[Jim Yingst]

[Chris]: Jim presumably you mean 'transient' in its memory sense of the word rather than as Java defines it, i.e. aren't you describing volatile, or am I amount to learn some new property of transient ;-)

Yes, I have an annoying tendency to say transient when I mean volatile, and vice versa.

[Derek White]

Slight dig here, but I found this thread by Googling this exact message, so I'm going to assume others will as well.

I have ran into this same message, and I've replaced the code as such:

private Object o;
private boolean someFlag;

private synchronized Object getO()
{
	return o;
}

public void notifyO()
{
	synchronized(getO())
	{
		someFlag = true;
		getO().notify();
	}
}

I was wondering A, is this okay in general, and B, would this be bad because I am getting the object twice instead of acquiring a lock then using the locked object, IE:

public void notifyO()
{
	Object localO = getO();
	synchronized(localO)
	{
		someFlag = true;
		localO.notify();
	}
}

JSYK, I am also setting o via a synchronized mutator.

Both of these make the warnings go away, and both of them seem to be doing what I want them to, but I am wondering which is proper and why, and whether or not there will be consequences and why.


Thanks!

[sarvesh meens]

In the first case,the object used to lock is private & there is no way for other any other thread to change it (assuming, there is no public setter methods provided). Hence, there are no warnings from the IDE.

In the second case, you are locking on local variable. There is no way for any other thread to access the local variable of another thread.Hence, there are no warnings from IDE.

The best/suggested way to lock in the given scenario is this:

    
public class X
{
  //make the lock object final so that noone can change/reset it
  private final Object o;
  
  //initialize lock object from constructor
  //can also be initialized at declaration
  public X(){
     o = new Object();
  }
 
  public void x()
  {
    //lock on final object
    synchronized (o){
      //do what-ever you want
    }
  }
}

Hope, this helps.