java - do I have to declare my shared listener member variable as volatile?

Question

I have a simple class which does some calculations in its own thread and reports the results to the listener.

class Calculator extends Thread {
    protected Listener listener;

    public void setListener(Listener l) {
        listener = l;
    }

    public void run() {
        while (running) {
            ... do something ...

            Listener l = listener;

            if (l != null) {
                l.onEvent(...);
            }
        }
    }
}

At any time, the user can call setListener(null) if he doesn't want any events for a certain time period. So, in the run() function, I create a copy of the listener, so I can't run into a NullPointerException which might happen if the listener is set to null after the != null condition check succeeded. In my case, I believe this is a correct alternative for synchronizing it.

My question is: should I declare here the listener member variable as volatile? I have been reading a lot about volatile, but all examples seem to target the basic data types (boolean, int, ...), and not Objects. So therefore, I am not sure whether Objects should/could be declared volatile as well. I believe I have to declare it as volatile, so the thread always has the latest version of the member variable, but I am not sure.

Thanks!

Just as a suggestion -- not really answering your question I admit -- but would it work better to put an 'enabled' type flag on your Listener, and then rather than having the client set the Listener null or not-null, have it setEnabled(true) or setEnabled(false)? That gets you around the problem of handling an inadvertent NPE, and also prevents you from having to periodically instantiate new Listener objects. — Jim Kiley, Nov 22 '11 at 17:39
@Japer D., That's a really, really, really ugly solution you've got there. — mre, Nov 22 '11 at 17:40
@JimKiley I agree with you. However, I was simplifying my question, hence it looks a bit strange. Apologies for that. — Japer D., Nov 22 '11 at 17:40
@JaperD., Instead of allowing another object to set a member variable of another class to `null`, you should provide a state that can be queried (i.e. `isEventAvailable()`). If that evaluates to true, execute `onEvent`, otherwise do nothing. Also, if you ever want that thread to stop, you'll have to declare `running` to be `volatile`. — mre, Nov 22 '11 at 17:48
@Крысa I don't agree. There is nothing wrong with having no listener at a certain time, imho. It is also the default and initial scenario. — Japer D., Nov 22 '11 at 17:57
@Крысa How would your code look like given that the listener can be null? I am interested to see what would be different. No offense by the way, just asking! — Japer D., Nov 22 '11 at 18:07

score 5 · Accepted Answer · answered Nov 22 '11 at 17:38

5

Yes. To guarantee that the Calculator thread sees a new value, set by another thread, you would have to make the variable volatile.

However, volatile is a quite low-level mechanism and seldom used in client code. I suggest you consider using java.util.concurrent.AtomicReference in this scenario, which makes sure these things works as expected.

answered Nov 22 '11 at 17:38

aioobe

413,195
112
811
826

Alright, thanks for the confirmation. I'll look into AtomicReference, I have to admit i ignored the new concurrent package for way too long. – Japer D. Nov 22 '11 at 17:43
Hehe.. me too, but that's because I try to avoid concurrency altogether as long as possible :-) – aioobe Nov 22 '11 at 17:43
Even with an atomic reference, things can execute in such an order that a listener would receive an event call after it had been deregistered. See my updated answer. – Ted Hopp Nov 22 '11 at 17:59
1

I'm no expert, but I don't see any advantage to using an AtomicReference (over volatile) in this application. What am I missing? What does it make "work as expected" in this application that doesn't "work as expected" anyway? – Ed Staub Nov 22 '11 at 18:06
@EdStaub - I agree. I don't see any benefit of AtomicReference over volatile here. – Ted Hopp Nov 22 '11 at 22:28

Ted Hopp · Answer 2 · 2011-11-22T17:48:22.150

3

If you use this approach, you can no longer guarantee that a listener will not receive an event notification after setListener(null) returns. Execution could proceed as follows:

Listener l = listener; // listener != null at this point

// setListener(null) executes here

if (l != null) {
    l.onEvent(...);
}

If you need to be guaranteed that no events will be posted to a listener after it has been unregistered, then you need to use synchronized blocks. Declaring listener to be volatile won't help. The code should instead be:

public synchronized void setListener(Listener l) {
    listener = l;
}

public void run() {
    while (running) {
        ... do something ...

        synchronized (this) {
            if (listener != null) {
                listener.onEvent(...);
            }
        }
    }
}

If you want to avoid the expense of synchronized all the time, you can do this:

if (listener != null) {
    synchronized (this) {
        if (listener != null) {
            listener.onEvent(...);
        }
    }
}

This runs the slight risk that you will miss an event after setting a non-null listener. Declaring listener to be volatile would probably fix that.

edited Nov 22 '11 at 17:48

answered Nov 22 '11 at 17:38

Ted Hopp

232,168
48
399
521

That's exactly the reason why I copy the member variable. If setListener(null) is called, the local variable l in run() isn't changed. This way, I avoid the synchronization block. No? – Japer D. Nov 22 '11 at 17:39
@TedHopp, no, since he copies the content to a local variable. – aioobe Nov 22 '11 at 17:40
@JaperD. - I realized that and completely revised my answer. – Ted Hopp Nov 22 '11 at 17:49
Thanks for your update. Now I understand your concern. However, in my case, it isn't a problem that the listener still receives events (for some time) after it unregistered itself. However, in the general case this might be a problem. It's good that you added this for clarity. – Japer D. Nov 22 '11 at 17:53
FWIW, regarding your last code snippet, I remember that double-checked locking isn't working as expected in Java. I might be wrong here. I'll have to google it up. See for example http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html and http://stackoverflow.com/questions/1625118/java-double-checked-locking. – Japer D. Nov 22 '11 at 17:56
@JaperD. - The double-check locking problem has to do with constructing a singleton. Here, where the issue is just setting or unsetting a reference to a presumably completely constructed listener, those problems don't arise. – Ted Hopp Nov 22 '11 at 18:05
1

The listener may be deconstructed after unsetting. Double-checking is fixed as of JDK5 - _if_ listener is volatile. – Ed Staub Nov 22 '11 at 18:30
1

+1 `volatile` usually introduces a false sense of security without fixing the fixing the problem. However, `volatile` has some use here in that the listener wont be unsafely published (although it probably wont be thread safe anyway...). – Tom Hawtin - tackline Nov 22 '11 at 19:33

java - do I have to declare my shared listener member variable as volatile?

2 Answers2