Synchronization of non-final field
Asked Answered
N

8

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?

Newmann answered 2/8, 2011 at 10:44 Comment(0)
M
146

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

Mcdougald answered 2/8, 2011 at 10:46 Comment(6)
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.Mcdougald
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.Accomplish
That's another good point, and I agree; locking on a non-final variable definitely needs careful justification.Mcdougald
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.Eastlake
@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.Mcdougald
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).Panta
A
51

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.

Accomplish answered 2/8, 2011 at 10:49 Comment(23)
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.)Mcdougald
@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.Accomplish
True. It messes up the reasoning. I agree.Mcdougald
The important part is that the lock object should be somehow associated with the data which you will protect by it, I think.Changeup
@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...Accomplish
I never said it is easy ... Yes, synchronizing on some object which can't be switched against another one is usually better.Changeup
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?Coroneted
@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.Accomplish
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?Winwaloe
@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.Accomplish
@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)Winwaloe
@JonSkeet what if I synchronize with the 'Runnable' itself?Emlynne
In short, it's safer if we always declare such lock objects final, correct?Baghdad
@St.Antario: Well, it's the variable that's final rather than the object, but yes, that seems reasonable advice.Accomplish
@JonSkeet Of course, I meant the variable. Got it, thanks much.Baghdad
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.Maupassant
@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.Accomplish
@JonSkeet ah, thank you for correcting me. I don't work much with synchronization.Maupassant
@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.Maupassant
@JonSkeet why not just synchronize the set(o) method?Salt
@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.Accomplish
@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?Salt
@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.Accomplish
U
13

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://mcmap.net/q/101291/-when-a-lock-holds-a-non-final-object-can-the-object-39-s-reference-still-be-changed-by-another-thread

And for more details why you need synchronized at all due to RAM and caches have a look here: https://mcmap.net/q/99181/-why-must-wait-always-be-in-synchronized-block

Umbilicus answered 30/1, 2014 at 16:54 Comment(4)
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: #32852964Cosmonautics
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?Asymmetry
The code won't compile, as a type of LOCK is missing and is not initialized.Eventuate
@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. ;-)Umbilicus
P
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.

Pilch answered 7/10, 2014 at 9:45 Comment(2)
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.Schnook
even after adding this mutex, you will still get the warning "Synchronization on a non-final lock".Eventuate
W
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();
    }

}
Winwaloe answered 23/3, 2015 at 16:57 Comment(2)
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.)Accomplish
@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 #29217766 of synchronized() atomicity - so we will see if it can be used (and is someone reply)Winwaloe
S
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?

Shipwright answered 23/6, 2016 at 15:4 Comment(0)
W
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).

Wickham answered 2/8, 2011 at 10:49 Comment(0)
G
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.

Gastrulation answered 22/7, 2012 at 17:3 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.