4
\$\begingroup\$

I've written a small Service class that contains a thread pool so that it can process multiple tasks in parallel. The kicker is that is also allows clients to bind to the service and automatically exits when there are no more tasks to run and there are no more bound clients.

Some questions I have are:

  • Am I abusing the Service's lifecycle in a way that may come back to bite me in the future?
  • Am I handling all my variables in a thread-safe way?
  • Is the service guaranteed to exit? (at the right time?)
  • Is the service exception-safe? (would a Exception or RuntimeException thrown a task running in the thread pool kill everything?)
  • Is the entire design a bad practice according to Android conventions/standards?

This is how the service is used: I have another class (say TestService) that extends ThreadPoolService. TestService will have some public methods that the client can call after binding which will execute some tasks. The client (usually an Activity or Fragment) will bind to the service and also start it so that the service doesn't automatically exit after unbinding.

There are 3 ways for the service to exit:

  • In onCreate(), the service starts a 10 second timer so that the service will attempt exit if nothing binds to the service. This timer is cancelled when enqueueOperation() is called.
  • When all clients unbind and onUnbind() is called, the service will attempt to exit.
  • Every time a task completes, the service will attempt to exit.

Below are the ThreadPoolService class, a sample service that extends it, and a sample activity that uses it.

ThreadPoolService.java

package com.github.chenxiaolong.dualbootpatcher;

import android.app.AlarmManager;
import android.app.PendingIntent;
import android.app.Service;
import android.content.Context;
import android.content.Intent;
import android.os.Binder;
import android.os.IBinder;
import android.os.SystemClock;
import android.util.Log;

import java.util.concurrent.BlockingQueue;
import java.util.concurrent.LinkedBlockingQueue;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

public class ThreadPoolService extends Service {
    private static final boolean DEBUG = true;
    private final String TAG = getClass().getSimpleName();

    /** Exit action for intent */
    private final String ACTION_EXIT = getClass().getCanonicalName() + ".exit";
    /** Number of milliseconds for delayed exit */
    private final long EXIT_DELAY = 10 * 1000;
    /** Pending intent to exit the service */
    private PendingIntent mExitPendingIntent;
    /** Alarm manager */
    private AlarmManager mAlarmManager;

    /** Number of cores */
    private static final int NUMBER_OF_CORES = Runtime.getRuntime().availableProcessors();
    /** Time an idle thread will wait before it exits */
    private static final int KEEP_ALIVE_TIME = 1;
    /** Units of {@link #KEEP_ALIVE_TIME} */
    private static final TimeUnit KEEP_ALIVE_TIME_UNITS = TimeUnit.SECONDS;

    /** Queue of operations to be processed by {@link #mThreadPool} */
    private final BlockingQueue<Runnable> mWorkQueue = new LinkedBlockingQueue<>();
    /** Thread pool for executing operations */
    private ThreadPoolExecutor mThreadPool;

    /** Number of currently running or pending operations */
    private int mOperations = 0;

    private final Object mLock = new Object();

    /** Whether there are bound clients */
    private boolean mBound = false;

    /** Binder instance */
    private final IBinder mBinder = new ThreadPoolServiceBinder();

    /** Local Binder class */
    public class ThreadPoolServiceBinder extends Binder {
        public ThreadPoolService getService() {
            return ThreadPoolService.this;
        }
    }

    /** Log debug messages if {@link #DEBUG} is true */
    private void log(String message) {
        if (DEBUG) {
            Log.d(TAG, message);
        }
    }

    /** {@inheritDoc} */
    @Override
    public void onCreate() {
        super.onCreate();
        log("onCreate()");

        // Initialize thread pool
        mThreadPool = new ThreadPoolExecutor(NUMBER_OF_CORES, NUMBER_OF_CORES, KEEP_ALIVE_TIME,
                KEEP_ALIVE_TIME_UNITS, mWorkQueue);

        mAlarmManager = (AlarmManager) getSystemService(Context.ALARM_SERVICE);
        Intent exitIntent = new Intent(this, getClass());
        exitIntent.setAction(ACTION_EXIT);
        mExitPendingIntent = PendingIntent.getService(this, 0, exitIntent, 0);

        scheduleDelayedExit();
    }

    /** {@inheritDoc} */
    @Override
    public void onDestroy() {
        super.onDestroy();
        log("onDestroy()");

        // Attempt to stop thread pool. This shouldn't be an issue as there should be no tasks
        // running at this point. The service stops when all clients have unbinded from it and there
        // are no pending tasks.
        mThreadPool.shutdownNow();
        try {
            mThreadPool.awaitTermination(60, TimeUnit.SECONDS);
        } catch (InterruptedException e) {
            Log.e(TAG, "Failed to wait 60 seconds for thread pool termination", e);
        }
    }

    /** {@inheritDoc} */
    @Override
    public int onStartCommand(final Intent intent, final int flags, final int startId) {
        log("onStartCommand(intent=" + intent + ", flags=" + flags + ", startId=" + startId + ")");

        if (intent != null && ACTION_EXIT.equals(intent.getAction())) {
            attemptToStop();
            return START_NOT_STICKY;
        }

        return START_STICKY;
    }

    /** {@inheritDoc} */
    @Override
    public IBinder onBind(final Intent intent) {
        log("onBind(intent=" + intent + ")");
        synchronized (mLock) {
            mBound = true;
        }
        return mBinder;
    }

    /** {@inheritDoc} */
    @Override
    public boolean onUnbind(final Intent intent) {
        log("onUnbind(intent=" + intent + ")");
        synchronized (mLock) {
            mBound = false;
        }
        attemptToStop();
        // We're don't need to differentiate between bind and rebind
        return false;
    }

    /**
     * Enqueue operation
     *
     * @param runnable Task to run in thread pool
     */
    protected void enqueueOperation(final Runnable runnable) {
        cancelDelayedExit();

        synchronized (mLock) {
            mOperations++;
            mThreadPool.execute(new Runnable() {
                @Override
                public void run() {
                    try {
                        runnable.run();
                    } finally {
                        synchronized (mLock) {
                            mOperations--;
                            attemptToStop();
                        }
                    }
                }
            });
        }
    }

    /**
     * Attempt to stop the service
     *
     * This will not call {@link #stopSelf()} unless there are no tasks running and no clients bound
     */
    private void attemptToStop() {
        synchronized (mLock) {
            log("Attempting to stop service");
            if (mOperations > 0 || mBound) {
                log("Not stopping: # of operations: " + mOperations + ", is bound: " + mBound);
                return;
            }

            log("Calling stopSelf(): there are no more operations");
            stopSelf();
        }
    }

    /**
     * Schedule delayed exit.
     */
    private void scheduleDelayedExit() {
        log("Scheduling delayed exit after " + EXIT_DELAY + "ms");
        mAlarmManager.set(AlarmManager.ELAPSED_REALTIME_WAKEUP,
                SystemClock.elapsedRealtime() + EXIT_DELAY, mExitPendingIntent);
    }

    /**
     * Cancel delayed exit
     */
    private void cancelDelayedExit() {
        log("Cancelling delayed exit");
        mAlarmManager.cancel(mExitPendingIntent);
    }
}

TestService.java

package com.github.chenxiaolong.dualbootpatcher;

import android.util.Log;

public class TestService extends ThreadPoolService {
    private static final String TAG = TestService.class.getSimpleName();

    public void testAction() {
        enqueueOperation(new Runnable() {
            @Override
            public void run() {
                Log.d(TAG, "Sleeping for 10 seconds");
                try {
                    Thread.sleep(10000);
                } catch (InterruptedException e) {
                    Log.e(TAG, "Interrupted", e);
                }
            }
        });
    }
}

TestActivity.java

package com.github.chenxiaolong.dualbootpatcher;

import android.content.ComponentName;
import android.content.Context;
import android.content.Intent;
import android.content.ServiceConnection;
import android.os.Bundle;
import android.os.IBinder;
import android.support.v7.app.AppCompatActivity;

import com.github.chenxiaolong.dualbootpatcher.ThreadPoolService.ThreadPoolServiceBinder;

public class TestActivity extends AppCompatActivity implements ServiceConnection {
    private TestService mService;

    @Override
    protected void onCreate(final Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_test);
    }

    @Override
    protected void onStart() {
        super.onStart();

        Intent intent = new Intent(this, TestService.class);
        startService(intent);
        bindService(intent, this, Context.BIND_AUTO_CREATE);
    }

    @Override
    protected void onStop() {
        super.onStop();

        unbindService(this);
    }

    @Override
    public void onServiceConnected(final ComponentName name, final IBinder service) {
        ThreadPoolServiceBinder binder = (ThreadPoolServiceBinder) service;
        mService = (TestService) binder.getService();

        // Run test task
        mService.testAction();
    }

    @Override
    public void onServiceDisconnected(final ComponentName name) {
        mService = null;
    }
}
\$\endgroup\$
1
\$\begingroup\$

I'm not sure you're using the right solution to your problem. It might be much less complicated to just have Activities that need it create their own thread-pools... This removes the dependency on your code and it clarifies the responsibility of cleaning up.

The way I see it you're solving a problem that isn't really there. I'll still review your code as is :)


private static final boolean DEBUG = true;

This isn't a nice thing to do, because it makes you change this constant before deploying your code to production. This may change the behaviour of your app. It should work for both values of this flag, and it should be tested for both. Currently you're assuming that DEBUG will only be used the way you intended in your class. In this case it's not a wrong assumption, but usually assumptions are bad.

The code should make the least amount of assumptions possible.

private final String TAG = getClass().getSimpleName();

this could be static if you initialized it as ThreadPoolService.class.getSimpleName(); Then it would even earn the all-caps name. Usually all-caps is reserved for "actual constants", meaning those that are static and final.

private final String ACTION_EXIT = getClass().getCanonicalName() + ".exit"

The code relying on this is making assumptions again. It shouldn't. I'm not even sure why exactly this is needed. If you want to allow "forcible termination" of this service, it'd be easier to expose a method to do that and call it instead of spinning up a specially named action in an intent...

private final long EXIT_DELAY = 10 * 1000;

Can be initialized much clearer as TimeUnit.SECONDS.toMillis(10);. This documents everything from the preceding comment as code.

Now something "interesting" happens. You declare two fields. after those you proceed to declare constants again. I personally dislike this mixing of types of members. Usually my classes begin with public constants, followed by private ones. Then there's immutable fields and then mutable ones. This gives me a nice overview and I know where to find things in relation to others....

Since I'm not usually programming android, I'll just quickly mention that I have a personal dislike against the convention to prefix members with m and I'd like to point you to this excellent blog post on hungarian notation.
... moving swiftly on:

It might be beneficial to give your workQueue some limit as to how many tasks you allow to be enqueued before blocking the calling thread... I'm guessing something like NUMBER_OF_CORES * 15 should be okay for most purposes.
Then again it may be completely superfluous....

private int mOperations = 0;

You can get the number of currently pending operations from your workQueue. Then you'll just have to check for pending operations when you want to try to shutdown the executor. If you call shutdown() the Executor finishes the currently running tasks. Only with shutdownNow it cancels them. (You can pass a flag to shutdown that allows the executor to interrupt running tasks, but eh).

The next line again messes with what I perceive as the "natural" ordering of members ... Alas, it's not a convention so I'll just keep being twitchy about it

That's enough for starters, I hope someone comes and reviews the functionality.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.