Implementing a thread pool inside a Service

Question

I'm currently working on implementing a Service that when requested will perform some work on several parallel threads.

My implementation is based on the ThreadPoolExecutor class along with a LinkedBlockingQueue.

As a ground rule, once all tasks are finished and the are no pending tasks in the queue, I would like to stop the service (though the service can later on be started again and follow the same logic).

I have been able to reach the desired outcome using the code below, yet I'm not sure if this approach is correct.

public class TestService extends Service {
    // Sets the initial threadpool size to 3
    private static final int CORE_POOL_SIZE = 3;

    // Sets the maximum threadpool size to 3
    private static final int MAXIMUM_POOL_SIZE = 3;

    // Sets the amount of time an idle thread will wait for a task before terminating
    private static final int KEEP_ALIVE_TIME = 1;

    // Sets the Time Unit to seconds
    private static final TimeUnit KEEP_ALIVE_TIME_UNIT = TimeUnit.SECONDS;

    // A queue of Runnables for the uploading pool
    private final LinkedBlockingQueue<Runnable> uploadQueue = new LinkedBlockingQueue<Runnable>();

    // A managed pool of background upload threads
    private final ThreadPoolExecutor uploadThreadPool = new ThreadPoolExecutor(
            CORE_POOL_SIZE, MAXIMUM_POOL_SIZE, KEEP_ALIVE_TIME, KEEP_ALIVE_TIME_UNIT,
            uploadQueue) {

        @Override
        protected void afterExecute(Runnable r, Throwable t) {
            super.afterExecute(r, t);

            if (getActiveCount() == 1 && getQueue().size() == 0) {
                // we're the last Runnable around + queue is empty, service can be
                // safely stopped.
                TestService.this.stopSelf();
            }
        }
    };

    @Override
    public IBinder onBind(Intent intent) {
        return null;
    }

    @Override
    public int onStartCommand(Intent intent, int flags, int startId) {
        // execute a new Runnable
        uploadThreadPool.execute(new TestRunnable());

        /**
         * Indicating that if Android has to kill off this service (i.e. low memory),
         * it should not restart it once conditions improve.
         */
        return START_NOT_STICKY;
    }

    @Override
    public void onDestroy() {
        uploadThreadPool.shutdownNow();
        uploadQueue.clear();

        super.onDestroy();
    }
}

So I've got a few things I'm not yet sure about.

1. Assuming onDestroy was called, is it safe to assume that my implementation will interrupt ALL running threads and will safely clear the pending tasks without somehow interrupting with the ThreadPoolExecutor class implementation? the reason I'm asking is because the queue is associated with the executor, and perhaps shutdownNow is async and depending on the state of the queue. Is there a better way of doing this?

2. Am I correct implementing this logic inside onDestroy? from my experience there are some cases where the service is killed (i.e. low memory) and this callback is not called. Should I perform a similar approach in other places as well?

3. Would it be better to declare my queue & executor class members as static? -- As stated by @TheTwo "Excecutor cannot be re-used once shutdown is called".

4. ThreadPoolExecutor class expects a BlockingQueue, what are the pros/cons of using other types of BlockingQueue implementations (i.e. ArrayBlockingQueue)?

5. Regarding the way I currently detect when the queue is empty & there are no more pending tasks (specifically inside afterExecute callback) -- Is this the best way of doing this? Or can I get an indication that the queue is empty and tasks are finished in another way?

Appreciate any help!

Answer 1

I think you are trying to implement a service, which introduces many problems, but solves none. Effectively you reduce the calling code by one line - the creation of the Executor - but take away the ability to fine-control it. There isn't any gain in scheduling many tasks, as this is already solved by the OS' thread scheduler. In addition a malicious caller can break several other programs by just adding enough while(true) sleep(100); loops.

On your questions:

1. You cannot make sure that all threads are properly interrupted ever, because there is no way to interrupt a thread that is not properly watching the interrupt flag. A while(true) ; cannot be interrupted except by System.exit(). You could theoretically stop a thread, but this feature is deprecated for a reason, because it can leave the actual task in an incomplete/unfinished state (i.E. leaving TCP-connections half-open).

2. No, you are not correctly implementing that. For once tasks left int he queue would simply vanish in the void, and then an Excecutor cannot be re-used once shutdown is called. So you at least need to create a new Excecutor instance on service start, and you really should figure out what to do with the remaining tasks.

3. No, because of 2.

4. The pros/cons of list types depend on your use-case. An ArrayList has a higher cost when growing/shrinking but a lower cost when indexing a specific element (indexOf), while a linked-list is about the opposite. Since your queue always adds to the tail, doesn't care about any other element but the first, and it is growing/shrinking frequently, a linked-list is the best choice.

5. You should not stop the task this way at all, because the order of execution with threads is undefined. In worst case your calling program is interrupted each time till the service is done executing, which will cause the service to constantly start & stop for no particular reason, while wasting a lot of processing time for nothing. Why would you even want to stop the service? If it has nothing to do, it won't do anything but to use a few bytes of memory.

Answer 2

1. No. shutdownNow attempts to interrupt the currently actively tasks. There are no guarantees that it will able to do it
2. Yes it is. From the documentation:

   Once neither of these situations hold, the service's onDestroy() method is called and the service is effectively terminated. All cleanup (stopping threads, unregistering receivers) should be complete upon returning from onDestroy().

3. there is no reason to have those members declared as static. static member are associated withe class rather than with any object, and the common use is to share the same static member between different (services in your case) instances.
4. You should read carefully the documentation about all the possible implementations of the BlockingQueue<E> interface, but I doubt that, for normal cases use, you will see differences from the performances perspective.