Is it possible to use volatile variables to allow consistent write-only synchronisation?

Question

This is the code:

public class Controller {

    Container cont = new Container();
    ...

    public Object get(){
        if(!cont.isLocked()){
            return cont.getObject(); //many threads can read at the same time
        }
        else synchronized(cont){
            return cont.getObject(); //threads wait until write is finished
        }
    }

    public void set(Object o){
        synchronized(cont){
            cont.lock();
            cont.setObject(o);
            cont.unlock();
        }
    }
}

public class Container {
    private volatile boolean lock = false;
    private Object data;

    public Container(){
            }

    public void lock() {
        lock = true;
    }

    public void unlock() {
        lock = false;
    }

    public boolean isLocked() {
        return lock;
    }

    public void setObject(Object o) {
        data = o;
    }

    public Object getObject() {
        return data;
    }
}

Will it work, or will it break, what do you think?

It looks good to me. But if you wanted to be sure, you could actually test this by introducing delays into `setObject` and `getObject`; plus a few judicious debugging messages. +1 for producing an SSCCE. — Dawood ibn Kareem, Feb 13 '14 at 18:01

score 5 · Accepted Answer · answered Feb 13 '14 at 18:02

5

It may/will break. Example of execution:

T1: if(!cont.isLocked()) returns true
T2: set(someObject)
T1: return cont.getObject(); returns an old version

Even worse, since you don't have a proper happens-before relationship between T2 and T1, cont.getObject() may return the a reference to the new object created by T2, but pointing to an inconsistent/not-fully-constructed object.

You have a typical check-then-act problem.

Why don't you just use a volatile reference to your object?

public class Controller {

    private volatile Object obj = new Object();

    public Object get(){ return obj; }
    public void set(Object o){ obj = o; }
}

That would be thread safe.

answered Feb 13 '14 at 18:02

assylias

321,522
82
660
783

1

That would probably be sufficient - but you should explain your real goal because your example is unnecessarily complex - so either your actual use case has additional constraints that you have not mentioned or you should make yoru class a lot simpler. – assylias Feb 13 '14 at 18:09
Do you really think it can be done simpler? I mean, I can't just synchronize on `Object data`, that would defeat the purpose of non-synchronized reads... – Marcus Feb 13 '14 at 18:12
@Marcus What I meant is that the code snippet in my answer does what you intended to do but in a much simpler way. Is there a reason why you had a complex design in the first place? – assylias Feb 13 '14 at 18:16
Well, as I said I can't just synchronize on `Object data` because I want to read without slow synchronisation...and IMO also `synchronized(cont)` must be called when writing. Or do you think that using just a volatile object will also synchronize it consistently? – Marcus Feb 13 '14 at 18:26
2

@Marcus volatile will ensure visibility and reference assignment is atomic so yes it should be fine. But then your `Controller` just becomes a simple version of an `AtomicReference` so you might as well just use that instead... – assylias Feb 13 '14 at 18:34
Ah, so I will use volatile fields to enable fast reads and just synchronize the writes. Thanks a lot! – Marcus Feb 13 '14 at 18:39
@Marcus you don't need to synchronize the writes if the field is volatile. – assylias Feb 13 '14 at 18:43
But here the answerer says it's needed: http://stackoverflow.com/a/11385864/2012947. What do you think? – Marcus Feb 13 '14 at 18:50
@Marcus in that situation synchronized is required because there is more than one operation (`if (a == null) { a = new A(); return a; }`) - so because there is a check then an assignment, volatile guarantees are not enough. In my example it is a simple assignment which is atomic so no need for synchronized. – assylias Feb 13 '14 at 18:53
@Marcus Same thing with the example on `x++` - that statement effectively represents a read, increment and write. So three operations which need to be enclosed in a synchronized block to be atomic. There again volatile is not enough. – assylias Feb 13 '14 at 18:55
Alright, so when I have 2 volatile objects within the `Container` and want to change them consistently, then I need synchronization (to make a block "atomic" so to speak). Okey, one could say that in this case I also need to synchronize the reads (to get atomic reads). However in my case I will only change 1 field, the other remains constant, so I'll set the 1 field as volatile and the other as final. Thx :) – Marcus Feb 13 '14 at 19:01
@Marcus If you need to update the two fields atomically then volatile won't be enough. Have you considered using a ReadWriteLock? It is designed to deal with scenarios like yours (from what I understand). – assylias Feb 13 '14 at 19:22

Is it possible to use volatile variables to allow consistent write-only synchronisation?

1 Answers1