Multithreading correctly done?

Question

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<BeanRunOutput> _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 = true;
                    Thread.sleep(_iSleepMilliseconds);
                } catch (InterruptedException e) { }
                _boIsSleeping = false;
            } 
        }
    }

    /*
     * This is the method that will be called by run() to create new data in the _runList member variable.
     */
    private void createFreshRunList() {
        synchronized(this._lockRunList) {
            // Here we put the code that creates the run list.  
            // Typically it will take 5 to 10 seconds.
            _runList = new ArrayList<BeanRunOutput>();
        }
    }

    /*
     * This is the method a client will call to get the data in _runList.
     */
    public List<BeanRunOutput> getRunList() {
        synchronized(this._lockRunList) {
            return _runList;
        }
    }

    /*
     * This is the method a client will call to signal the thread that a new run list should be created.
     * If run() is currently gathering data (_boIsSleeping is false), a flag will be set to tell run() 
     * to run again immediately after it is finished, without sleeping first.  If the thread is currently
     * sleeping, it will be interrupted. 
     */
    public void signalFreshRunListCreation() {
        if (_boIsSleeping) {
            // Thread is sleeping so interrupt it.
            this.interrupt();
        } else {
            // The thread is running so signal it should just run again when 
            // complete (in other words not sleep).
            _boFreshRunListRequested = true;
        }
    }

    /*
     * This method allows a client to signal the thread to exit.
    */
    public void stopThread() {
        _boStop = true;
    }
}

Answer 1

1. 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.

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

3. 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.

4. 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.

5. 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.

6. 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<BeanRunOutput> 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<String> refreshQueue = 
        new ArrayBlockingQueue<String>(1);

    private List<BeanRunOutput> runList;

    public DataRefresherImpl() {
    }

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

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

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

    @Override
    public List<BeanRunOutput> 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.)

Answer 2

Comments in no particular order but mostly from top to bottom:

1. It is generally considered better to implement Runnable rather than extend Thread.

2. Consequently from 1, calling this.interrupt() is no longer valid and changing it to Thread.currentThread().interrupt() in signalFreshRunListCreation highlights a problem. You will actually then be interrupting the calling thread, not the RunList thread.

   The trick to this is to store a copy of your current thread as one of the first things in your run method. You can then call its interrupt() to interrupt the RunList.

3. I am being reminded by my IDE to only synchronize on final objects. This is good advice so make _lockRunList final.

4. Just returning the list from getRunList is dangerous. You should probably either copy it or do something else to protect yourself from the other thread hacking it around.

5. It's probably not the best idea to use interrupt to control your flow. There are other mechanisms that would be better, I'd probably create a single-item BlockingQueue and use poll(time) on it to implement your pause. You can then prematurely interrupt the pause by posting to the queue.

   This would also eliminate the use of Thread.sleep() which is generally considered worth trying to avoid as it is rarely what you really want to do.

6. stopThread propably should also interrupt it. Once you are not using interrupt to control your flow this will become easy.

7. boFreshRunListRequested should be made volatile because it is accessed from several threads.

Just because I felt like making one, here's a Dozer that will pause for a certain amount of time or a special event. It uses the same mechanism we are suggesting.

/**
 * Use one of these to doze for a certain time.
 *
 * The dozing is fully interruptable.
 *
 * Another thread can stop the caller's doze with either a wakeup call or an abort call.
 *
 * These can be interpreted in any way you like but it is intended that a Wakeup is
 * interpreted as a normal awakening and should probably be treated in exactly the
 * same way as an Alarm. An Abort should probably be interpreted as a suggestion
 * to abandon the proces.
 */
public class Doze {
  // Special alarm messages.
  public enum Alarm {
    // Standard timeout.
    Alarm,
    // Just wake from your doze.
    Wakeup,
    // Abort the whole Doze process.
    Abort;
  }
  // My queue to wait on.
  private final ArrayBlockingQueue<Alarm> doze = new ArrayBlockingQueue<Alarm>(1);
  // How long to wait by default.
  private final long wait;

  public Doze(long wait) {
    this.wait = wait;
  }

  public Doze() {
    this(0);
  }

  public Alarm doze() throws InterruptedException {
    // Wait that long.
    return doze(wait);
  }

  public Alarm doze(long wait) throws InterruptedException {
    // Wait that long.
    Alarm poll = doze.poll(wait, TimeUnit.MILLISECONDS);
    // If we got nothing then it must be a normal wakeup.
    return poll == null ? Alarm.Alarm : poll;
  }

  public void wakeup() {
    // Just post a Wakeup.
    doze.add(Alarm.Wakeup);
  }

  public void abort() {
    // Signal the system to abort.
    doze.add(Alarm.Abort);
  }

  public static void main(String[] args) throws InterruptedException {
    // Doze for 10 seconds.
    final Doze d = new Doze(1 * 1000);

    // Start a dozing thread.
    new Thread(new Runnable() {
      @Override
      public void run() {
        try {
          Alarm a = d.doze();
          // Wait forever until we are aborted.
          while (a != Alarm.Abort) {
            System.out.println("Doze returned " + a);
            a = d.doze();
          }
          System.out.println("Doze returned " + a);
        } catch (InterruptedException ex) {
          // Just exit on interrupt.
        }
      }
    }).start();

    // Wait for a few seconds.
    Thread.sleep(3000);

    // Wake it up.
    d.wakeup();

    // Wait for a few seconds.
    Thread.sleep(3000);

    // Abort it.
    d.abort();


  }
}