Question about thread safe code

Question

I have a class as follows

public MyClass{

Boolean flag = false;

    public Boolean getflag(){
       synchronized(flag){
          //BLOCK 1
          return flag;
       }
    }

    public Boolean setflag(){
       synchronized(flag){
           //BLOCK 2
           this.flag = flag;
       }
    }
}

Both methods are synchronized on object flag.Now my doubt is can two different threads can simultaniously executed the synchronized blocks (1&2). Can the following situation arise? 1)Thread 1 is setting flag value and thread 2 getting its value at same time?

The signature of setFlag probably isn't what you intended. Did you mean `public void setflag(Boolean flag)`? — Atreys, Aug 03 '11 at 12:32

Péter Török · Accepted Answer · 2011-08-03T12:38:26.633

Yes it can. Note that you are synchronizing on the same object you are setting. So the setter can change the object reference to something different, then the getter can synchronize on the new object while the setter is still inside the synchronized block.

But there is more: flag is (usually) a reference to one of the system-wide singletons Boolean.TRUE and Boolean.FALSE, so it is (at least theoretically) possible to lock on these outside of your class, even without referring to your class in any way. In which case you can end up with a deadlock and you can have a hard time figuring out why.

(Note also that the code in its current form is wrong, since the setter has no parameter, thus this.flag = flag assigns the reference to itself - but above I assume that you meant it to behave like a normal setter :)

The fix is to use a dedicated, private final lock object (in case you want to absolutely ensure that noone outside can synchronize on the same lock you are using within your class - which I suppose your original intention was):

public MyClass{

    private final Object lock = new Object();
    private Boolean flag = false;

    public Boolean getflag(){
       synchronized(lock){
          //BLOCK 1
          return flag;
       }
    }

    public void setflag(Boolean flag){
       synchronized(lock){
           //BLOCK 2
           this.flag = flag;
       }
    }
}

If you aren't worried so much about other synchronizing on the same lock you internally use, you can simply make your methods synchronized (in which case they lock on this).

Making flag volatile would probably be the simplest approach in this situation and remove all the synchronized blocks. — Robin, Aug 03 '11 at 13:14
@Robin, depends on the context and what the OP wants to achieve. — Péter Török, Aug 03 '11 at 14:58

score 2 · Answer 2 · edited May 23 '17 at 12:11

I'm assuming your setFlag method should actually have a parameter and no return value?

This looks like a bad idea to me.

Flag is set to reference r1
Thread 1 calls setFlag(r2)
Thread 1 acquires lock on r1
Thread 1 sets flag to reference r2
Thread 2 acquires lock on r2
Both threads are actually executing concurrently, both in a synchronized block, but locking on different objects...

Basically, I think it's a bad idea to synchronize on a mutable field. There's a recent question about this which you may find interesting.

I would use this design instead:

private final Object lock = new Object();
private boolean flag;

public void setFlag(boolean flag) {
    synchronized (lock) {
        this.flag = flag;
    }
}

public boolean getFlag() {
    synchronized (lock) {
        return flag;
    }
}

(Or just use a volatile field, potentially. It really depends on what else is in the class.)

score 0 · Answer 3 · answered Aug 03 '11 at 12:21

0

Problem can be in method setFlag - it will change "lock object" for synchronization. You must be sure that you synchronize on same object.
private Object lock = new Object(); And synchronize on object lock.

answered Aug 03 '11 at 12:21

bugs_

3,544
4
34
39

score 0 · Answer 4 · answered Aug 03 '11 at 12:21

Thats not going to work. You're blocking on the monitor of the object referenced by flag. Imagine if a thread enters the setter, locks on the current flag object, and then flag is pointed at a new Flag. Then a second thread enters the getter, and is free to obtain a lock on flag, since it now points to a different object.

Hence you can have two threads both seemingly locked on 'flag', but on different objects. Thats why objects used for locking should generally be declared final, to avoid this situation arising.

Question about thread safe code

4 Answers4