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!

Answer 1

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.

Answer 2

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.