Simple Background Thread Worker Class (follow-up)

Question

This is a follow up of Simple, generic background thread worker class

What changed?

1. Moved from synchronized lazily evaluated static instances to true Singletons per the on-demand holder idiom https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
2. Added a constructor to auto-start the Thread.

Why is this being resubmitted?

Some of the changes introduce a potential anti-pattern that I'd appreciate feedback on. Additionally, I believe the feedback I had originally was fair, thoughtful and valuable, but did not necessarily address some of the points I was concerned about. This was my fault since I did not provide specifics. I'll do that now.

The original review included some feedback about static versus instance method conventions which were not addressed in the rewrite - this is intentional - I'm using static members for the Handler and ThreadPoolExecutor so that they're common to each instance of the class, but can also be overridden by subclasses to provide different implementations.

What specifically should be evaluated?

1. Overall ease of use, and usefulness: would this suffice as a replacement (not a drop-in) for most of what AsyncTask is being used for? A design goal was to delegate most responsibilities to the user (to test and react if dependencies exist - e.g., did the Activity that was going to display the finished work get paused/destroyed? if so, e.g., let's stop decoding the bitmap) and provide them with a simple mechanism to do that: the quitIfCancelledOrInterrupted method, which should be tested as frequently as possible within the body of a Runnable passed to runOnUiThread, and tests if cancel has been called or if the Thread has been interrupted otherwise - either way, a new interrupt request is sent, and a true value is returned, to which the user code should reply by immediately exiting (return from the run method).

2. Is there any controversy about the specific Singleton pattern used here? I know that some people in the community consider any Singleton an anti-pattern, but IIUC this specific pattern (https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom) gets around most of the concerns opponents express about it.

3. Thread safety features (include appropriate-ness of my use of volatile versus other features).

4. Thread pool construction. I start with 1 and max it at Runtime.getRuntime().availableProcessors() - are there reasons to do otherwise? I understand that a single processor might not be used if a Thread is idle, but I'd imagine that'd be unlikely here.

Per this comment, usage would be roughly what AsyncTask is used for, putting work in a background thread and posting the results back to the UI thread. E.g.,

new AsynchronousOperation(){
  @Override public void performWorkInBackgroundThread(){
    someMethodThatDoesALotOfWorkOrTakesTime();
    // test as often as possible for cancel or interruption; bail if so
    if(quitIfCancelledOrInterrupted(true)){
      return;
    }
    // if you need to report back to UI thread, there's a convenience method that uses typical Android convention
    runOnUiThread(someRunnableToUpdateUIOrNotifyUser);
  }
};

(Comments removed for length)

package example.os;

import android.os.Handler;
import android.os.Looper;
import android.os.Process;

import java.util.concurrent.BlockingDeque;
import java.util.concurrent.LinkedBlockingDeque;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

public abstract class AsynchronousOperation implements Runnable {

  protected static final int INITIAL_POOL_SIZE = 1;
  protected static final int MAXIMUM_POOL_SIZE = Runtime.getRuntime().availableProcessors();
  protected static final int KEEP_ALIVE_TIME = 2;
  protected static final TimeUnit KEEP_ALIVE_TIME_UNIT = TimeUnit.SECONDS;
  protected static final BlockingDeque<Runnable> BLOCKING_DEQUE = new LinkedBlockingDeque<>();

  protected volatile Thread mThread;

  private volatile boolean mCancelled;

  private static class ThreadPoolExecutorHolder {
    public static final ThreadPoolExecutor sThreadPoolExecutor = new ThreadPoolExecutor(
      INITIAL_POOL_SIZE,
      MAXIMUM_POOL_SIZE,
      KEEP_ALIVE_TIME,
      KEEP_ALIVE_TIME_UNIT,
      BLOCKING_DEQUE
    );
  }

  private static class HandlerHolder {
    public static final Handler sHandler = new Handler(Looper.getMainLooper());
  }

  public AsynchronousOperation(boolean shouldStart) {
    if(shouldStart) {
      start();
    }
  }

  public AsynchronousOperation() {
    this(true);
  }

  protected ThreadPoolExecutor getThreadPoolExecutor() {
    return ThreadPoolExecutorHolder.sThreadPoolExecutor;
  }

  protected Handler getHandler() {
    return HandlerHolder.sHandler;
  }

  public Thread getThread() {
    return mThread;
  }

  public boolean cancel(boolean mayInterrupt) {
    if(mayInterrupt && mThread != null) {
      mThread.interrupt();
    }
    boolean alreadyCancelled = mCancelled;
    mCancelled = true;
    return !alreadyCancelled;
  }

  public boolean isCancelled() {
    return mCancelled;
  }

  public boolean isInterrupted() {
    return mThread != null && mThread.isInterrupted();
  }

  public boolean isCancelledOrInterrupted() {
    return isCancelled() || isInterrupted();
  }

  public boolean quitIfCancelledOrInterrupted(boolean mayInterrupt) {
    boolean shouldQuit = isCancelledOrInterrupted();
    if(shouldQuit) {
      cancel(mayInterrupt);
    }
    return shouldQuit;
  }

  public void runOnUiThread(Runnable runnable) {
    getHandler().post(runnable);
  }

  public void run() {
    mThread = Thread.currentThread();
    Process.setThreadPriority(Process.THREAD_PRIORITY_BACKGROUND);
    performWorkInBackgroundThread();
  }

  public void start() {
    getThreadPoolExecutor().execute(this);
  }

  public abstract void performWorkInBackgroundThread();

}

(With comments)

package example.os;

import android.os.Handler;
import android.os.Looper;
import android.os.Process;

import java.util.concurrent.BlockingDeque;
import java.util.concurrent.LinkedBlockingDeque;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

/**
 *
 * Usage:
 * new AsynchronousOperation(){
 *   @Override public void performWorkInBackgroundThread(){
 *     Do stuff...
 *     Be as non-atomic as possible.
 *     If you can break things into small steps, do so -
 *     e.g., maybe download by looping through chunks instead of a library method
 *     at every opportunity, check #shouldQuit, if true, bail out
 *     (or use the convenience #quitIfCancelledOrInterrupted).
 *     For any operation you want to publish, use runOnUiThread and a Runnable.
 *     This would be for things like AsyncTask.onProgressUpdate or AsyncTask.onPostExecute.
 *     Remember to keep references to cancel if the operation depends on the lifecycle of a View or Activity.
 *   }
 * };
 */
public abstract class AsynchronousOperation implements Runnable {

  protected static final int INITIAL_POOL_SIZE = 1;
  protected static final int MAXIMUM_POOL_SIZE = Runtime.getRuntime().availableProcessors();
  protected static final int KEEP_ALIVE_TIME = 2;
  protected static final TimeUnit KEEP_ALIVE_TIME_UNIT = TimeUnit.SECONDS;
  protected static final BlockingDeque<Runnable> BLOCKING_DEQUE = new LinkedBlockingDeque<>();

  protected volatile Thread mThread;

  private volatile boolean mCancelled;

  /**
   * Single static instance of the ThreadPoolExecutor that will manage each Thread.
   *
   * Moving from double-check locking: https://en.wikipedia.org/wiki/Double-checked_locking
   * to on-demand https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
   */
  private static class ThreadPoolExecutorHolder {
    public static final ThreadPoolExecutor sThreadPoolExecutor = new ThreadPoolExecutor(
      INITIAL_POOL_SIZE,
      MAXIMUM_POOL_SIZE,
      KEEP_ALIVE_TIME,
      KEEP_ALIVE_TIME_UNIT,
      BLOCKING_DEQUE
    );
  }

  /**
   * Single static instance of Handler (on the main thread) shared by all AsynchronousOperation instances.
   *
   * Moving from double-check locking: https://en.wikipedia.org/wiki/Double-checked_locking
   * to on-demand https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom
   */
  private static class HandlerHolder {
    public static final Handler sHandler = new Handler(Looper.getMainLooper());
  }

  /**
   * Constructor for AsynchronousOperation with the option of submitting to the ThreadPoolExecutor
   * immediately.
   *
   * @param shouldStart True if the AsynchronousOperation should start immediately.
   */
  public AsynchronousOperation(boolean shouldStart) {
    if(shouldStart) {
      start();
    }
  }

  /**
   * Constructor for AsynchronousOperation.  Using this signature will immediately call #start.
   */
  public AsynchronousOperation() {
    this(true);
  }

  /**
   * Lazily instantiate the ThreadPoolExecutor, constructed with default values. If customization of these values is
   * required, override this getter method in the implementation subclass.
   *
   * @return A ThreadPoolExecutor instance used by all AsynchronousOperation instances.
   */
  protected ThreadPoolExecutor getThreadPoolExecutor() {
    return ThreadPoolExecutorHolder.sThreadPoolExecutor;
  }

  /**
   * Lazily instantiate a new Handler on the main thread.  This Handler instance is common to and shared between
   * all AsynchronousOperation instances, and is only accessible to those instances.
   *
   * @return A Handler instance used by all AsynchronousOperation instances to communicate with the main thread.
   */
  protected Handler getHandler() {
    return HandlerHolder.sHandler;
  }

  /**
   * This will usually be the Thread provided by the ThreadPoolExecutor when submitted to it, but since #run
   * is a public method, it might be the main thread (or any thread) if used inappropriately.  Assuming this does
   * not happen, you can be rely on this referencing the background Thread provided to it.
   *
   * This will be null until #run is invoked.
   *
   * @return The Thread that owned this instance the moment #run was invoked.
   */
  public Thread getThread() {
    return mThread;
  }

  /**
   * Cancels an operation.
   *
   * This is neither synchronized nor an AtomicBoolean because the boolean primitive for the cancelled flag is
   * volatile and only ever set to true (never set back to false), which should be thread-safe here.
   *
   * Cancellation by itself will attempt to interrupt the background thread this worker is on, but by itself will
   * not interrupt any work being performed - the user should test for cancellation frequently within the
   * #performWorkInBackgroundThread method.
   *
   * @param mayInterrupt True if cancelling this operation should also interrupt its owner Thread.
   * @return True if the operation was cancelled (and had not previously been cancelled).
   */
  public boolean cancel(boolean mayInterrupt) {
    if(mayInterrupt && mThread != null) {
      mThread.interrupt();
    }
    boolean alreadyCancelled = mCancelled;
    mCancelled = true;
    return !alreadyCancelled;
  }

  /**
   * @return True if this AsynchronousOperation has been explicitly cancelled.
   */
  public boolean isCancelled() {
    return mCancelled;
  }

  /**
   * @return True if this AsynchronousOperation instance's owner thread has been interrupted.
   */
  public boolean isInterrupted() {
    return mThread != null && mThread.isInterrupted();
  }

  /**
   * @return True if this AsynchronousOperation has been explicitly cancelled or its owner thread has been interrupted.
   */
  public boolean isCancelledOrInterrupted() {
    return isCancelled() || isInterrupted();
  }

  /**
   * Tests for explicit cancellation or thread interruption - if either are true, it cancels and offers another
   * opportunity to interrupt the owner thread.
   *
   * @param mayInterrupt True if cancelling this operation should also interrupt its owner Thread.
   * @return True if this AsynchronousOperation has been explicitly cancelled or its owner thread has been interrupted.
   */
  public boolean quitIfCancelledOrInterrupted(boolean mayInterrupt) {
    boolean shouldQuit = isCancelledOrInterrupted();
    if(shouldQuit) {
      cancel(mayInterrupt);
    }
    return shouldQuit;
  }

  /**
   * Executes a Runnable instance's #run method on the main thread.
   *
   * @param runnable The Runnable instance whose #run method should be invoked on the main thread.
   */
  public void runOnUiThread(Runnable runnable) {
    getHandler().post(runnable);
  }

  /**
   * Creates a reference to the current thread, sets that thread's priority, and initiates the
   * #performWorkInBackgroundThread method.
   *
   * Unlike most Runnable implementations, this method should not be commonly overridden.  It is not
   * marked as final in case a subclasses wants to hook into this process, but in almost all cases the
   * subclass should do its work in #performWorkInBackgroundThread rather than #run.
   */
  public void run() {
    mThread = Thread.currentThread();
    Process.setThreadPriority(Process.THREAD_PRIORITY_BACKGROUND);
    performWorkInBackgroundThread();
  }

  /**
   * Passes this instance to the common ThreadPoolExecutor, which will provide a worker thread and call this
   * instance's #run method.
   */
  public void start() {
    getThreadPoolExecutor().execute(this);
  }

  /**
   * Subclasses should override this method to perform work in the background thread provided by this class when
   * #start is called.
   *
   * Any time work needs to be published to the main thread from within the method body, use #runOnUiThread.
   *
   * Work within this method should tend to be non-atomic and test for #quitIfCancelledOrInterrupted as often as
   * possible, returning immediately if that method returns true.
   */
  public abstract void performWorkInBackgroundThread();

}

Answer 1

Escape of this on the constructor

When you call start the thread may receive the current object (this) and start execution immediately. This means that your constructor may not have run fully and your object is not ready for use yet. The only thing you can do to avoid this is to call start after the constructor.

This issue is more common on event listeners implementations.

Concurrency

You are using volatile correctly and it really suffices your currency needs. However people tend to prefer to user AtomicBoolean and AtomicReference.

Code that you do not control

The programmer implementing performWorkInBackgroundThread may throw an exception on is implementation there is no really a need for your worker thread to propagate that exception to nowhere: catch it.

All the cancelling stuff

(Again) The programmer only needs to implement performWorkInBackgroundThread. In this implementation he usually does two things:

• He implements a heavy processing/IO task that takes some time
• He updates the user interface

What you really want to do here is just to have a way to cancel the heavy processing task. There is no really a need for him to know about cancelled or interrupted status. Thus isCancelled, isInterrupted, isCancelledOrInterrupted and quitIfCancelledOrInterrupted are all meaningless. And cancel doesn't need any arguments at all, after all it's being explicitly called so it should cancel no matter what.

If you really want to provide a way to him implement something when the heavy work is cancelled then provide another method like onWorkCanceled.

Number of threads

The number of threads is one of those things that you have to profile to make sure to see which number is the best one. But I would say that you are using a fair approach.

There is no harm of having one thread idle lying around. If your API is being used it's because someone needs it, thus that thread will work at some point (Or so we hope at least).

At maximum you have as many threads as your parallelism allows meaning that you can take advantage of all CPUs on the device.

There is one particular scenario where the maximum threads may need to increase. If you keep doing operations that take loads of time and those operations blocks the thread the BLOCKING_DEQUE keeps increasing with stuff to do. If maximum threads would be larger a new thread could come into the scene and work while one of those threads is blocked.

See here a reference implementation of AsyncTask