Synchronization of non-final field

A warning is showing every time I synchronize on a non-final class field. Here is the code:

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

so I changed the coding in the following way:

 public class X  
 {  
   
   private final Object o;       
   public X()
   {  
     o = new Object();  
   }  
    
   public void x()  
   {  
     synchronized (o)
     {  
     }  
   }  
 }  

I am not sure the above code is the proper way to synchronize on a non-final class field. How can I synchronize a non final field?

First of all, I encourage you to really try hard to deal with concurrency issues on a higher level of abstraction, i.e. solving it using classes from java.util.concurrent such as ExecutorServices, Callables, Futures etc.

That being said, there's nothing wrong with synchronizing on a non-final field per se. You just need to keep in mind that if the object reference changes, the same section of code may be run in parallel. I.e., if one thread runs the code in the synchronized block and someone calls setO(...), another thread can run the same synchronized block on the same instance concurrently.

Synchronize on the object which you need exclusive access to (or, better yet, an object dedicated to guarding it).

It's really not a good idea - because your synchronized blocks are no longer really synchronized in a consistent way.

Assuming the synchronized blocks are meant to be ensuring that only one thread accesses some shared data at a time, consider:

• Thread 1 enters the synchronized block. Yay - it has exclusive access to the shared data...
• Thread 2 calls setO()
• Thread 3 (or still 2...) enters the synchronized block. Eek! It think it has exclusive access to the shared data, but thread 1 is still furtling with it...

Why would you want this to happen? Maybe there are some very specialized situations where it makes sense... but you'd have to present me with a specific use case (along with ways of mitigating the sort of scenario I've given above) before I'd be happy with it.

I agree with one of John's comment: You must always use a final lock dummy while accessing a non-final variable to prevent inconsistencies in case of the variable's reference changes. So in any cases and as a first rule of thumb:

Rule#1: If a field is non-final, always use a (private) final lock dummy.

Reason #1: You hold the lock and change the variable's reference by yourself. Another thread waiting outside the synchronized lock will be able to enter the guarded block.

Reason #2: You hold the lock and another thread changes the variable's reference. The result is the same: Another thread can enter the guarded block.

But when using a final lock dummy, there is another problem: You might get wrong data, because your non-final object will only be synchronized with RAM when calling synchronize(object). So, as a second rule of thumb:

Rule#2: When locking a non-final object you always need to do both: Using a final lock dummy and the lock of the non-final object for the sake of RAM synchronisation. (The only alternative will be declaring all fields of the object as volatile!)

These locks are also called "nested locks". Note that you must call them always in the same order, otherwise you will get a dead lock:

public class X {
	private final LOCK;
	private Object o;

	public void setO(Object o){
		this.o = o;  
	}  

	public void x() {
		synchronized (LOCK) {
		synchronized(o){
			//do something with o...
		}
		}  
	}  
} 

As you can see I write the two locks directly on the same line, because they always belong together. Like this, you could even do 10 nesting locks:

synchronized (LOCK1) {
synchronized (LOCK2) {
synchronized (LOCK3) {
synchronized (LOCK4) {
	//entering the locked space
}
}
}
}

Note that this code won't break if you just acquire an inner lock like synchronized (LOCK3) by another threads. But it will break if you call in another thread something like this:

synchronized (LOCK4) {
synchronized (LOCK1) {  //dead lock!
synchronized (LOCK3) {
synchronized (LOCK2) {
	//will never enter here...
}
}
}
}

There is only one workaround around such nested locks while handling non-final fields:

Rule #2 - Alternative: Declare all fields of the object as volatile. (I won't talk here about the disadvantages of doing this, e.g. preventing any storage in x-level caches even for reads, aso.)

So therefore aioobe is quite right: Just use java.util.concurrent. Or begin to understand everything about synchronisation and do it by yourself with nested locks. ;)

For more details why synchronisation on non-final fields breaks, have a look into my test case: https://stackoverflow.com/a/21460055/2012947

And for more details why you need synchronized at all due to RAM and caches have a look here: https://stackoverflow.com/a/21409975/2012947

I'm not really seeing the correct answer here, that is, It's perfectly alright to do it.

I'm not even sure why it's a warning, there is nothing wrong with it. The JVM makes sure that you get some valid object back (or null) when you read a value, and you can synchronize on any object.

If you plan on actually changing the lock while it's in use (as opposed to e.g. changing it from an init method, before you start using it), you have to make the variable that you plan to change volatile. Then all you need to do is to synchronize on both the old and the new object, and you can safely change the value

public volatile Object lock;

...

synchronized (lock) {
    synchronized (newObject) {
        lock = newObject;
    }
}

There. It's not complicated, writing code with locks (mutexes) is actally quite easy. Writing code without them (lock free code) is what's hard.

EDIT: So this solution (as suggested by Jon Skeet) might have an issue with atomicity of implementation of "synchronized(object){}" while object reference is changing. I asked separately and according to Mr. erickson it is not thread safe - see: https://stackoverflow.com/questions/29217266/is-synchornized-block-atomic. So take it as example how to NOT do it - with links why ;)

See the code how it would work if synchronised() would be atomic:

public class Main {
    static class Config{
        char a='0';
        char b='0';
        public void log(){
            synchronized(this){
                System.out.println(""+a+","+b);
            }
        }
    }

    static Config cfg = new Config();

    static class Doer extends Thread {
        char id;

        Doer(char id) {
            this.id = id;
        }

        public void mySleep(long ms){
            try{Thread.sleep(ms);}catch(Exception ex){ex.printStackTrace();}
        }

        public void run() {
            System.out.println("Doer "+id+" beg");
            if(id == 'X'){
                synchronized (cfg){
                    cfg.a=id;
                    mySleep(1000);
                    // do not forget to put synchronize(cfg) over setting new cfg - otherwise following will happend
                    // here it would be modifying different cfg (cos Y will change it).
                    // Another problem would be that new cfg would be in parallel modified by Z cos synchronized is applied on new object
                    cfg.b=id;
                }
            }
            if(id == 'Y'){
                mySleep(333);
                synchronized(cfg) // comment this and you will see inconsistency in log - if you keep it I think all is ok
                {
                    cfg = new Config();  // introduce new configuration
                    // be aware - don't expect here to be synchronized on new cfg!
                    // Z might already get a lock
                }
            }
            if(id == 'Z'){
                mySleep(666);
                synchronized (cfg){
                    cfg.a=id;
                    mySleep(100);
                    cfg.b=id;
                }
            }
            System.out.println("Doer "+id+" end");
            cfg.log();
        }
    }

    public static void main(String[] args) throws InterruptedException {
        Doer X = new Doer('X');
        Doer Y = new Doer('Y');
        Doer Z = new Doer('Z');
        X.start();
        Y.start();
        Z.start();
    }

}

AtomicReference suits for your requirement.

From java documentation about atomic package:

> A small toolkit of classes that support lock-free thread-safe programming on single variables. In essence, the classes in this package extend the notion of volatile values, fields, and array elements to those that also provide an atomic conditional update operation of the form:

boolean compareAndSet(expectedValue, updateValue);

Sample code:

String initialReference = "value 1";

AtomicReference<String> someRef =
    new AtomicReference<String>(initialReference);

String newReference = "value 2";
boolean exchanged = someRef.compareAndSet(initialReference, newReference);
System.out.println("exchanged: " + exchanged);

In above example, you replace String with your own Object

Related SE question:

https://stackoverflow.com/questions/3964211/when-to-use-atomicreference-in-java/

If o never changes for the lifetime of an instance of X, the second version is better style irrespective of whether synchronization is involved.

Now, whether there's anything wrong with the first version is impossible to answer without knowing what else is going on in that class. I would tend to agree with the compiler that it does look error-prone (I won't repeat what the others have said).

Just adding my two cents: I had this warning when I used component that is instantiated through designer, so it's field cannot really be final, because constructor cannot takes parameters. In other words, I had quasi-final field without the final keyword.

I think that's why it is just warning: you are probably doing something wrong, but it might be right as well.