109

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?

Draken
  • 3,134
  • 13
  • 34
  • 54
divz
  • 7,847
  • 23
  • 55
  • 78

8 Answers8

144

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).

aioobe
  • 413,195
  • 112
  • 811
  • 826
  • 2
    I'm saying that, *if* you synchronize on a non-final field, you should be aware of the fact that the snippet of code runs with exclusive access to the object `o` referred to at the time the synchronized block was reached. If the object which `o` refers to changes, another thread can come along and execute the synchronized code block. – aioobe Aug 02 '11 at 10:53
  • 11
    As I say in my answer, I think I'd need to have it justified very carefully to me, why you would *want* to do such a thing. And I wouldn't recommend synchronizing on `this`, either - I would recommend creating a final variable in the class *solely for the purposes of locking*, which stops anyone else from locking on the same object. – Jon Skeet Aug 02 '11 at 11:05
  • 1
    That's another good point, and I agree; locking on a non-final variable definitely needs careful justification. – aioobe Aug 02 '11 at 11:08
  • I'm not sure about memory visibility issues around changing an object that is used for sync-ing. I think you would get into big trouble changing an object and then relying on code seeing that change correctly so that "the same section of code may be run in parallel". I'm not sure what, if any, guaratees are extended by the memory model to the memory-visibility of fields that are used to lock, as opposed to the variables accessed within the sync block. My rule of thumb would be, if you synchronize on something it should be final. – Mike Q Aug 02 '11 at 11:30
  • @MikeQ, I'm not sure what you're aiming at here. Obviously two threads can't acquire the same lock, due to visibility issues. Either you (A) *modify* the content of the variable of which you synchronize on, (in which case you should be sure to (1) avoid data races by properly synchronize the accesses to this variable, and (2) understand that the same section of code can be executed by more than one thread) or (B) *don't modify* the content of the variable you synchronize on, in which you of course can program defensively by declaring it final. – aioobe Mar 13 '13 at 15:17
  • So many up-votes on that comment! But for example: **1.** If we wanted to synchronize another thread with `java.io.Closeable` extending object (to ensure is not being used, after disposed). **2.** But we don't want to add `getCloseableMutex()` method. **3.** Then, both `close()` and the thread `run()` should synchronize on the `real-object`. (Still, now we need to ensure `close()` is never called inside of synchronized-block). – Top-Master Mar 26 '21 at 15:27
50

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.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • One use case would be if `o` for instance referred to an ArrayList, and the synchronized block accessed this list methods. It would be unnecessary to lock on a separate "instance-global" lock object. (Assuming here that the class wants to provide a set-method for `o`.) – aioobe Aug 02 '11 at 11:06
  • 2
    @aioobe: But then thread 1 could still be running some code which is mutating the list (and frequently referring to `o`) - and *part way through its execution* start mutating a different list. How would that be a good idea? I think we fundamentally disagree on whether or not it's a good idea to lock on objects you touch in other ways. I would rather be able to reason about my code without any knowledge of what other code does in terms of locking. – Jon Skeet Aug 02 '11 at 11:08
  • True. It messes up the reasoning. I agree. – aioobe Aug 02 '11 at 11:11
  • The important part is that the lock object should be somehow associated with the data which you will protect by it, I think. – Paŭlo Ebermann Aug 02 '11 at 16:52
  • @Paŭlo: Well, it's more than just that - you also need to make sure that a thread *never* starts accessing shared data associated with one lock when it *actually* only owns a different lock. You could potentially achieve that by putting a `synchronized(o)` block *inside setO* - but it's a little odd to say the least... – Jon Skeet Aug 02 '11 at 16:57
  • I never said it is easy ... Yes, synchronizing on some object which can't be switched against another one is usually better. – Paŭlo Ebermann Aug 02 '11 at 17:01
  • Let's say i have Objects A and B in the class C and i want 3 threads to make access to A and B, but access to A should'nt lock access to B. So that locking the whole class C wouldn't be a good practice in this case. Any suggestions? Should I create a final dumb object for each of the objects i wanna lock? – Felype Jun 06 '13 at 11:29
  • 2
    @Felype: It sounds like you should ask a more detailed question as a separate question - but yes, I'd often create separate objects just as locks. – Jon Skeet Jun 06 '13 at 11:48
  • Hello I don't get what is the problem with synchronizing on non-final object. As long as I want to ensure only consistency within that object. I do not have real project example but just top of my head if I would have some configuration object and I want to ensure that if someone is changing configuration other thread won't read partial configuration. If some thread decides to switch to different configuration object then it's only correct that it synchronize to new object and not some global lock unnecessarily locking all threads that are reading configurations which are not modified. Right? – Vit Bernatik Mar 23 '15 at 12:54
  • 3
    @VitBernatik: No. If thread X starts modifying the configuration, thread Y changes the value of the variable being synchronized, then thread Z starts modifying the configuration, then both X and Z will be modifying the configuration at the same time, which is bad. – Jon Skeet Mar 23 '15 at 12:57
  • @Jon - I think I understand your worry. I made an example where it can be demonstrated - by omitting "synchronized(cfg)" before changing the reference. However it would be a bug in code. If all is synchronized as I posted I think there shall be no problem with inconsistency. Can you peer it and approve/disprove it? (It is now posted as the answer to this thread - cos comment space is too small) – Vit Bernatik Mar 23 '15 at 17:06
  • @JonSkeet what if I synchronize with the 'Runnable' itself? – shaithana Aug 20 '15 at 21:27
  • 1
    In short, it's safer if we __always__ declare such lock objects final, correct? – St.Antario Nov 17 '15 at 06:45
  • 1
    @St.Antario: Well, it's the *variable* that's final rather than the object, but yes, that seems reasonable advice. – Jon Skeet Nov 17 '15 at 06:46
  • @JonSkeet Of course, I meant the variable. Got it, thanks much. – St.Antario Nov 17 '15 at 06:53
  • You know, I would think the developers of hotspot might think of using a hidden lock object whenever we synchronize on something. A synchronized method synchronizes every single object in the instance. It could instead just have a common lock object instead of leaving it to us to make global locks. Same for synchronization blocks. – AMDG Nov 26 '15 at 15:25
  • 2
    @LinkTheProgrammer: "A synchronized method synchronizes every single object in the instance" - no it doesn't. That's simply not true, and you should revisit your understanding of synchronization. – Jon Skeet Nov 26 '15 at 15:53
  • @JonSkeet ah, thank you for correcting me. I don't work much with synchronization. – AMDG Jul 01 '16 at 02:12
  • 1
    @JonSkeet ah hah! I see so clearly: synchronized methods auto-acquire the monitor for the object belonging to the invoked method, and synchronized blocks manually acquire a lock. I've been told the polar opposite of the truth. – AMDG Jul 01 '16 at 02:39
  • @JonSkeet why not just synchronize the set(o) method? – Matthew Jan 21 '20 at 21:59
  • 1
    @Matthew: That would be synchronizing on an entirely different object - the "container" object rather than the one being contained. We don't really know what the OP was trying to achieve here (it was over 8 years ago, and I don't want to second-guess here) but it may well be very different from synchronizing on the container. – Jon Skeet Jan 22 '20 at 08:21
  • @JonSkeet Ok, I understand. But, I don't understand what the issue is with what the OP is doing. If I'm not mistaken, he is trying to prevent multiple threads from accessing the contents of object 'o'. He made the object synchronized to ensure this. However, you are saying this doesn't work because the object reference can be changed via the setO() method? Why does that stop synchronization from working the way the OP wanted? – Matthew Jan 22 '20 at 15:42
  • @Matthew: "If I'm not mistaken, he is trying to prevent multiple threads from accessing the contents of object 'o'." Well, we don't really know that. Presumably there's more code within the `synchronized` block, and we don't know what that does. But at the very least, it would seem odd for `x` to be half way through executing, then `setO` be called on a different thread, and then any further uses of `o` within `x` would access a different object. Fundamentally I *suspect* the OP hadn't really decided in a clear way what they were trying to achieve with synchronization. – Jon Skeet Jan 22 '20 at 16:06
14

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

Community
  • 1
  • 1
Marcus
  • 1,222
  • 2
  • 13
  • 22
  • 1
    I think that you have to wrap the setter of `o` with a synchronized(LOCK) to establish a "happens-before" relationship between setting and reading object `o`. I'm discussing this in a similar question of mine: http://stackoverflow.com/questions/32852464/how-to-avoid-synchronization-on-a-non-final-field – Petrakeas Sep 29 '15 at 19:46
  • I use dataObject to synchronise access to dataObject members. How is that wrong? If the dataObject starts to point somewhere different, I want it synchronised on the new data to prevent concurrent threads to modify it. Any problems with that? – Harmen Nov 26 '15 at 14:37
  • The code won't compile, as a type of LOCK is missing and is not initialized. – Tarun Sep 28 '21 at 16:41
  • @Tarun: It was just a basic code example without the requirement to compile. But if you want to make it compileable, feel free to edit the answer accordingly. ;-) – Marcus Sep 18 '22 at 09:53
2

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.

Evan Dark
  • 1,311
  • 7
  • 7
  • This may not work. Say o started as ref to O1, then thread T1 locks o (= O1) and O2 and sets o to O2. At the same time Thread T2 locks O1 and waits for T1 to unlock it. When it receives the lock O1, it will set o to O3. In this scenario, Between T1 releasing O1 and T2 locking O1, O1 became invalid for locking through o. At this time another thread may use o (=O2) for locking and proceed uninterrupted in race with T2. – GPS Jan 13 '17 at 11:11
  • even after adding this mutex, you will still get the warning "Synchronization on a non-final lock". – Tarun Sep 28 '21 at 16:47
2

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: Is entering synchronized 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();
    }

}
Community
  • 1
  • 1
Vit Bernatik
  • 3,566
  • 2
  • 34
  • 40
  • 1
    This *might* be okay - but I don't know whether there's any guarantee in the memory model that the value that you synchronize on is the most recently-written one - I don't think there's any guarantee of atomically "read and synchronize". Personally I try to avoid synchronizing on monitors which have other uses anyway, for simplicity. (By having a separate field, the code becomes *clearly* correct instead of having to reason about it carefully.) – Jon Skeet Mar 23 '15 at 17:11
  • @Jon. Thx for answer! I hear your worry. I agree for this case the external lock would avoid question of "synchronized atomicity". Thus would be preferable. Although there might be cases where you want to introduce in runtime more configuration and share different configuration for different thread groups (although it is not my case). And then this solution might become interesting. I posted question http://stackoverflow.com/questions/29217266/is-synchornized-block-atomic of synchronized() atomicity - so we will see if it can be used (and is someone reply) – Vit Bernatik Mar 23 '15 at 18:09
2

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:

When to use AtomicReference in Java?

Community
  • 1
  • 1
Ravindra babu
  • 37,698
  • 11
  • 250
  • 211
1

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.

peenut
  • 3,366
  • 23
  • 24
1

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).

NPE
  • 486,780
  • 108
  • 951
  • 1,012