Multithreading correctly done?

I rarely write multithreaded code, and am on shaky ground when doing so. I make mistakes. I would like a sanity check of something I am going to introduce in one of my apps.

There will be exactly one running instance of the Thread class below in my application. (It will be stored in a servlet context attribute.) "Clients" will call one of three methods on the instance, getRunList() to get data from it, signalFreshRunListCreation() to tell the thread to generate new data, and stopThread() to signal the thread to exit. On its own, the thread creates new data every 30 seconds.

Have I made any errors?

```
public class ThreadRunListDefault extends Thread {

// This set to true signals that the thread should exit.
private boolean _boStop = false;
// The thread sets this to true just before it sleeps.
private boolean _boIsSleeping = false;
// An external client sets this to true to signal that a new run list should be created.
private boolean _boFreshRunListRequested = false;
// This is a mutex to lock the run list while a fresh one is being created.
private Object _lockRunList = new Object();
// This is the actual data that will be returned.
private List _runList;
// This is the time the thread will sleep between refreshing the data on its own
private long _iSleepMilliseconds = 30000;

@Override
public void run() {

// Loop as long as no client has indicated the thread should stop.
while (!_boStop) {
// Create the data.
this.createFreshRunList();

// Only sleep if a refresh of the data wasn't requested while we were
// creating a fresh run list.
if (!_boFreshRunListRequested) {
try {
// We set the sleeping flag to true so that signalFreshRunListCreation() will
// know whether or not to call interrupt() on the thread and wake it up.
_boIsSleeping

-
The _boStop and the _boIsSleeping fields are accessed by multiple threads, so you should synchronize them. You could use volatile fields or AtomicBooleans too.

Make sure that you do read operations in synchronized blocks too, not just writes:


[...] synchronization has no effect unless both read and write operations are synchronized.

From Effective Java, 2nd Edition, Item 66: Synchronize access to shared mutable data.

-
A possible bug: _boFreshRunListRequested is never set to false.

-
Using _ prefixes (and sometimes this.) to access fields it's not really necessary and it's just noise. Modern IDEs use highlighting to separate local variables from instance variables.

-
You could eliminate some comments, for example:

// Loop as long as no client has indicated the thread should stop.
while (!boStop) {

This could be

while (!threadStop) {

Another example:

// Create the data.
   this.createFreshRunList();

Here you could rename the method to createData or createFreshData.

A good reading in this topic is Clean Code by Robert C. Martin and Code Complete, 2nd Edition by Steve McConnell.

-
It mainly depends on your requirements, but I'd consider renaming the class to DataRefresherImpl and implementing two interfaces: DataRefresherService and DataRefresher. The first would be used only in the servlet/container initializer. The second is for the clients to get their data and to request new data creation. It would prevent clients to call the stopThread method accidentally.

-
OldCurmudgeon has already mentioned the getRunList method is dangerous. You could use Collections.unmodifiableList here. Furthermore, make sure that the BeanRunOutput objects are also immutable or handle the modifications. See also: Effective Java, 2nd Edition, Item 39: Make defensive copies when needed

Finally, here is an refactored version with the queue which was suggested by OldCurmudgeon.

public interface DataRefresherService {
    void shutdown();
}

public interface DataRefresher {
    List getRunList();
    void signalFreshRunListCreation();
}

public class DataRefresherImpl implements Runnable, 
        DataRefresherService, DataRefresher {

    private final AtomicBoolean threadStop = new AtomicBoolean(false);

    private final ReentrantReadWriteLock listLock = new ReentrantReadWriteLock();

    private static final long REFRESH_INTERVAL_IN_MILLIS = 3000;

    private final ArrayBlockingQueue refreshQueue = 
        new ArrayBlockingQueue(1);

    private List runList;

    public DataRefresherImpl() {
    }

    @Override
    public void run() {
        while (!threadStop.get()) {
            createFreshRunList();
            sleepAndWakeOnRequest();
        }
    }

    private void createFreshRunList() {
        listLock.writeLock().lock();
        try {
            runList = new ArrayList();
        } finally {
            listLock.writeLock().unlock();
        }
    }

    private void sleepAndWakeOnRequest() {
        try {
            refreshQueue.poll(REFRESH_INTERVAL_IN_MILLIS, TimeUnit.MILLISECONDS);
        } catch (InterruptedException ignored) {
        }
    }

    @Override
    public List getRunList() {
        listLock.readLock().lock();
        try {
            return runList;
        } finally {
            listLock.readLock().unlock();
        }
    }

    @Override
    public void signalFreshRunListCreation() {
        refreshQueue.offer("refresh");
    }

    @Override
    public void shutdown() {
        threadStop.set(true);
        refreshQueue.offer("shutdown");
    }
}

(I've not tested this too much.)

StackExchange Code Review Q#18740, answer score: 6