Cosmetic changes and documentation updates/fixes

src/com/fsck/k9/service/CoreService.java

@@ -18,116 +18,167 @@ import com.fsck.k9.helper.power.TracingPowerManager;
 import com.fsck.k9.helper.power.TracingPowerManager.TracingWakeLock;
 
 /**
- * Note: All documentation in this file market <CK:RE> is documentation written by Christian Knecht by reverse engineering.
+ * {@code CoreService} is the base class for all K-9 Services.
- *       This documentation is without warranty and may not be accurate nor reflect the author's original intent.
  *
- * <CK:RE>
+ * <p>
- * CoreService is the base class for all K9 Services.
+ * An Android service is a way to model a part of an application that needs to accomplish certain
- *
+ * tasks without the UI part of the application being necessarily active (of course an application
- * An Android service is a way to model a part of an application that needs to accomplish certain tasks without the
+ * could also be a pure service, without any UI; this is not the case of K-9). By declaring a
- * UI part of the application being necessarily active (of course an application could also be a pure service, without
+ * service and starting it, the OS knows that the application has work to do and should avoid
- * any UI; this is not the case of K9). By declaring a service and starting it, the OS knows that the application has
+ * killing the process.
- * work to do and should avoid killing the process.
+ * </p><p>
- *
+ * A service's main purpose is to do some task (usually in the background) which requires one or
- * A service's main purpose is to do some task (usually in the background) which requires one of more threads. The
+ * more threads. The thread that starts the service is the same as the UI thread of the process. It
- * thread that starts the service is the same as the UI thread of the process. It should thus not be used to run
+ * should thus not be used to run the tasks.
- * the tasks.
+ * </p><p>
- *
+ * CoreService is providing the execution plumbing for background tasks including the required
- * CoreService is providing the execution plumbing for background tasks including the required thread and task queuing
+ * thread and task queuing for all K9 services to use.
- * for all K9 services to use.
+ * </p><p>
- *
+ * A service is supposed to run only as long as it has some work to do whether that work is active
- * A service is supposed to run only as long as it has some work to do whether that work is active processing or some
+ * processing or some just some monitoring, like listening on a network port for incoming connections
- * just some monitoring, like listening on a network port for incoming connections or listing on an open network
+ * or listing on an open network connection for incoming data (push mechanism).
- * connection for incoming data (push mechanism).
+ * </p><p>
- *
+ * To make sure the service is running only when required, is must be shutdown after tasks are
- * To make sure the service is running only when required, is must be shutdown after tasks are done. As the
+ * done. As the execution of tasks is abstracted away in this class, it also handles proper
- * execution of tasks is abstracted away in this class, it also proper shutdown handling if approriate. If
+ * shutdown. If a Service doesn't want this, it needs to call {@code enableAutoShutdown(true)} in
- * the Service requires this is should call enableAutoShutdown(true) in it's onCreate() method.
+ * its {@link Service#onCreate()} method.
- *
+ * </p><p>
- * While a service is running it's tasks, it is usually not a good idea to let the device go to sleep more.
+ * While a service is running its tasks, it is usually not a good idea to let the device go to
- * WakeLocks are used to avoid this. CoreService provides a central registry (singleton) that can be used
+ * sleep mode. Wake locks are used to avoid this. CoreService provides a central registry
- * application-wide to store WakeLocks.
+ * (singleton) that can be used application-wide to store wake locks.
- *
+ * </p><p>
- * In short, CoreService provides the following features to K9 Services:
+ * In short, CoreService provides the following features to K-9 Services:
- *  - task execution and queuing
+ * <ul>
- *  - Service life cycle management (insures the service is stopped when not needed anymore); disabled by default
+ *   <li>task execution and queuing</li>
- *  - WakeLock registry and management
+ *   <li>Service life cycle management (ensures the service is stopped when not needed anymore);
- *
+ *       enabled by default</li>
- * </CK:RE>
+ *   <li>wake lock registry and management</li>
+ * </ul>
  */
 public abstract class CoreService extends Service {
 
-    public static String WAKE_LOCK_ID = "com.fsck.k9.service.CoreService.wakeLockId"; // CK:Intent attribute ID
+    public static String WAKE_LOCK_ID = "com.fsck.k9.service.CoreService.wakeLockId";
-    private static ConcurrentHashMap<Integer, TracingWakeLock> wakeLocks = new ConcurrentHashMap<Integer, TracingWakeLock>(); // CK:WakeLocks registry
+
-    private static AtomicInteger wakeLockSeq = new AtomicInteger(0); // CK:WakeLock registry
+    private static ConcurrentHashMap<Integer, TracingWakeLock> sWakeLocks =
-    private ExecutorService threadPool = null; // CK:Threadpool with a single thread; used to execute and queue background actions inside the service
+        new ConcurrentHashMap<Integer, TracingWakeLock>();
-    private final String className = getClass().getName();
+    private static AtomicInteger sWakeLockSeq = new AtomicInteger(0);
-    private volatile boolean mShutdown = false; // CK:A:Seems to be used only when the service is "officially" shutdown to make sure that an exception raise because of the shutdown gets ignored.
 
     /**
-     * Controls the auto-shutdown mechanism of the service. The default service life-cycle model is that the service should run
+     * Threadpool that is used to execute and queue background actions inside the service.
-     * only as long as a task is running. If a service should behave differently, disable auto-shutdown.
+     */
+    private ExecutorService mThreadPool = null;
+
+    /**
+     * String of the class name used in debug messages.
+     */
+    private final String className = getClass().getName();
+
+    /**
+     * {@code true} if the {@code Service}'s {@link #onDestroy()} method was called. {@code false}
+     * otherwise.
+     *
+     * <p>
+     * <strong>Note:</strong>
+     * This is used to ignore (expected) {@link RejectedExecutionException}s thrown by
+     * {@link ExecutorService#execute(Runnable)} after the service (and with that, the thread pool)
+     * was shut down.
+     * </p>
+     */
+    private volatile boolean mShutdown = false;
+
+    /**
+     * Controls the auto shutdown mechanism of the service.
+     *
+     * <p>
+     * The default service life-cycle model is that the service should run only as long as a task
+     * is running. If a service should behave differently, disable auto shutdown using
+     * {@link #setAutoShutdown(boolean)}.
+     * </p>
      */
     private boolean mAutoShutdown = true;
 
     /**
-     * This variable is part of the auto-shutdown feature and determines whether the service has to be shutdown at the
+     * This variable is part of the auto shutdown feature and determines whether the service has to
-     * end of the onStart() method or not.
+     * be shutdown at the end of the {@link #onStart(Intent, int)} method or not.
      */
-    protected boolean mImmediateShutdown = true; //
+    protected boolean mImmediateShutdown = true;
 
-    @Override
-    public void onCreate() {
-        if (K9.DEBUG)
-            Log.i(K9.LOG_TAG, "CoreService: " + className + ".onCreate()");
-        threadPool = Executors.newFixedThreadPool(1);  // Must be single threaded
-        super.onCreate();
-
-    }
 
     /**
-     * Adds an existing WakeLock identified by it's WakeLock-ID to the specified Intent.
+     * Adds an existing wake lock identified by its registry ID to the specified intent.
-     * @param i
+     *
+     * @param context
+     *         A {@link Context} instance. Never {@code null}.
+     * @param intent
+     *         The {@link Intent} to add the wake lock registy ID as extra to. Never {@code null}.
      * @param wakeLockId
+     *         The wake lock registry ID of an existing wake lock or {@code null}.
+     * @param createIfNotExists
+     *         If {@code wakeLockId} is {@code null} and this parameter is {@code true} a new wake
+     *         lock is created, registered, and added to {@code intent}.
      */
-    protected static void addWakeLockId(Context context, Intent i, Integer wakeLockId, boolean createIfNotExists) {
+    protected static void addWakeLockId(Context context, Intent intent, Integer wakeLockId,
+            boolean createIfNotExists) {
+
         if (wakeLockId != null) {
-            i.putExtra(BootReceiver.WAKE_LOCK_ID, wakeLockId);
+            intent.putExtra(BootReceiver.WAKE_LOCK_ID, wakeLockId);
             return;
         }
-        if (createIfNotExists)
+
-          addWakeLock(context,i);
+        if (createIfNotExists) {
+          addWakeLock(context,intent);
+        }
     }
 
     /**
-     * Adds a new WakeLock to the intent.
+     * Adds a new wake lock to the specified intent.
-     * This will add the WakeLock to the central WakeLock registry managed by this class.
+     *
-     * @param context Required to be able to create a new wake-lock.
+     * <p>
-     * @param i Intent to which to add the WakeLock (CK:Q:still unclear why we need to link Intents and WakeLocks)
+     * This will add the wake lock to the central wake lock registry managed by this class.
+     * </p>
+     *
+     * @param context
+     *         A {@link Context} instance. Never {@code null}.
+     * @param intent
+     *         The {@link Intent} to add the wake lock registy ID as extra to. Never {@code null}.
      */
-    protected static void addWakeLock(Context context, Intent i) {
+    protected static void addWakeLock(Context context, Intent intent) {
-        TracingWakeLock wakeLock = acquireWakeLock(context,"CoreService addWakeLock",K9.MAIL_SERVICE_WAKE_LOCK_TIMEOUT); // CK:Q: What this timeout? 30secs seems a bizarre choice. It it's a safeguard it should be longer, if it's to cover the real time required by some operation, it seems too short (though I say this before knowing really what the service is supposed to do)
+        TracingWakeLock wakeLock = acquireWakeLock(context, "CoreService addWakeLock",
+                K9.MAIL_SERVICE_WAKE_LOCK_TIMEOUT);
         Integer tmpWakeLockId = registerWakeLock(wakeLock);
-        i.putExtra(WAKE_LOCK_ID, tmpWakeLockId);
+        intent.putExtra(WAKE_LOCK_ID, tmpWakeLockId);
     }
 
     /**
-     * Register WakeLock and returns its registry-entry-ID
+     * Registers a wake lock with the wake lock registry.
+     *
      * @param wakeLock
-     * @return
+     *         The {@link TracingWakeLock} instance that should be registered with the wake lock
-     * AUTHOR chrisk
+     *         registry. Never {@code null}.
+     *
+     * @return The ID that identifies this wake lock in the registry.
      */
     protected static Integer registerWakeLock(TracingWakeLock wakeLock) {
-        Integer tmpWakeLockId = wakeLockSeq.getAndIncrement();
+        // Get a new wake lock ID
-        wakeLocks.put(tmpWakeLockId, wakeLock);
+        Integer tmpWakeLockId = sWakeLockSeq.getAndIncrement();
+
+        // Store the wake lock in the registry
+        sWakeLocks.put(tmpWakeLockId, wakeLock);
+
         return tmpWakeLockId;
     }
 
     /**
-     * Acquires a WakeLock in a K9 standard way
+     * Acquires a wake lock.
+     *
      * @param context
-     * @return
+     *         A {@link Context} instance. Never {@code null}.
-     * AUTHOR chrisk
+     * @param tag
+     *         The tag to supply to {@link TracingPowerManager}.
+     * @param timeout
+     *         The wake lock timeout.
+     *
+     * @return A new {@link TracingWakeLock} instance.
      */
     protected static TracingWakeLock acquireWakeLock(Context context, String tag, long timeout) {
         TracingPowerManager pm = TracingPowerManager.getPowerManager(context);
@@ -138,29 +189,50 @@ public abstract class CoreService extends Service {
     }
 
     @Override
-    public void onStart(Intent intent, int startId) {
+    public void onCreate() {
+        if (K9.DEBUG) {
+            Log.i(K9.LOG_TAG, "CoreService: " + className + ".onCreate()");
+        }
+
+        mThreadPool = Executors.newFixedThreadPool(1);  // Must be single threaded
+        super.onCreate();
+    }
+
+    @Override
+    public final void onStart(Intent intent, int startId) {
         // deprecated method but still used for backwards compatibility with Android version <2.0
 
-        // CK:DocAdded: Manage wake-locks, especially, release any wake-locks held so far and define a new "local" wake lock.
+        // Acquire new wake lock
-        //           Also, because we create a new wakelock, we re-initialize the wakelock timeout and give
+        TracingWakeLock wakeLock = acquireWakeLock(this, "CoreService onStart",
-        //           the service-start code a protection of up to MAIL_SERVICE_WAKE_LOCK_TIMEOUT (currently 30s).
+                K9.MAIL_SERVICE_WAKE_LOCK_TIMEOUT);
-        TracingWakeLock wakeLock = acquireWakeLock(this,"CoreService onStart",K9.MAIL_SERVICE_WAKE_LOCK_TIMEOUT);
 
-        if (K9.DEBUG)
+        if (K9.DEBUG) {
             Log.i(K9.LOG_TAG, "CoreService: " + className + ".onStart(" + intent + ", " + startId);
+        }
 
+        // If we were started by BootReceiver, release the wake lock acquired there.
         int wakeLockId = intent.getIntExtra(BootReceiver.WAKE_LOCK_ID, -1);
         if (wakeLockId != -1) {
             BootReceiver.releaseWakeLock(this, wakeLockId);
         }
-        Integer coreWakeLockId = intent.getIntExtra(WAKE_LOCK_ID, -1);
+
-        if (coreWakeLockId != null && coreWakeLockId != -1) {
+        // If we were passed an ID from our own wake lock registry, retrieve that wake lock and
-            if (K9.DEBUG)
+        // release it.
+        int coreWakeLockId = intent.getIntExtra(WAKE_LOCK_ID, -1);
+        if (coreWakeLockId != -1) {
+            if (K9.DEBUG) {
                 Log.d(K9.LOG_TAG, "Got core wake lock id " + coreWakeLockId);
-            TracingWakeLock coreWakeLock = wakeLocks.remove(coreWakeLockId);
+            }
+
+            // Remove wake lock from the registry
+            TracingWakeLock coreWakeLock = sWakeLocks.remove(coreWakeLockId);
+
+            // Release wake lock
             if (coreWakeLock != null) {
-                if (K9.DEBUG)
+                if (K9.DEBUG) {
-                    Log.d(K9.LOG_TAG, "Found core wake lock with id " + coreWakeLockId + ", releasing");
+                    Log.d(K9.LOG_TAG, "Found core wake lock with id " + coreWakeLockId +
+                            ", releasing");
+                }
                 coreWakeLock.release();
             }
         }
@@ -171,117 +243,179 @@ public abstract class CoreService extends Service {
             super.onStart(intent, startId);
             startService(intent, startId);
         } finally {
-            try{wakeLock.release();} catch (Exception e) {/* ignore */}
+            try {
-            try{if (mAutoShutdown && mImmediateShutdown && startId != -1) stopSelf(startId);} catch (Exception e) {/* ignore */}
+                // Release the wake lock acquired at the start of this method
+                wakeLock.release();
+            } catch (Exception e) { /* ignore */ }
+
+            try {
+                // If there is no outstanding work to be done in a background thread we can stop
+                // this service.
+                if (mAutoShutdown && mImmediateShutdown && startId != -1) {
+                    stopSelf(startId);
+                }
+            } catch (Exception e) { /* ignore */ }
         }
     }
 
     /**
+     * Execute a task in the background thread.
      *
      * @param context
+     *         A {@link Context} instance. Never {@code null}.
      * @param runner
+     *         The code to be executed in the background thread.
      * @param wakeLockTime
+     *         The timeout for the wake lock that will be acquired by this method.
      * @param startId
-     * @return returns whether service-shutdown will actually happen after the task has been executed (or has already been done).
+     *         The {@code startId} value received in {@link #onStart(Intent, int)} or {@code null}
+     *         if you don't want the service to be shut down after {@code runner} has been executed
+     *         (e.g. because you need to run another background task).<br>
+     *         If this parameter is {@code null} you need to call {@code setAutoShutdown(false)}
+     *         otherwise the auto shutdown code will stop the service.
      */
-    public boolean execute(Context context, final Runnable runner, int wakeLockTime, final Integer startId) {
+    public void execute(Context context, final Runnable runner, int wakeLockTime,
+            final Integer startId) {
 
         boolean serviceShutdownScheduled = false;
-        final TracingWakeLock wakeLock = acquireWakeLock(context,"CoreService execute",wakeLockTime);
         final boolean autoShutdown = mAutoShutdown;
 
+        // Acquire a new wakelock
+        final TracingWakeLock wakeLock = acquireWakeLock(context, "CoreService execute",
+                wakeLockTime);
+
+        // Wrap the supplied runner with code to release the wake lock and stop the service if
+        // appropriate.
         Runnable myRunner = new Runnable() {
             public void run() {
                 try {
+                    // Get the sync status
                     boolean oldIsSyncDisabled = MailService.isSyncDisabled();
-                    if (K9.DEBUG)
+
-                        Log.d(K9.LOG_TAG, "CoreService (" + className + ") running Runnable " + runner.hashCode() + " with startId " + startId);
+                    if (K9.DEBUG) {
+                        Log.d(K9.LOG_TAG, "CoreService (" + className + ") running Runnable " +
+                                runner.hashCode() + " with startId " + startId);
+                    }
+
+                    // Run the supplied code
                     runner.run();
+
+                    // If the sync status changed while runner was executing, notify
+                    // MessagingController
                     if (MailService.isSyncDisabled() != oldIsSyncDisabled) {
                         MessagingController.getInstance(getApplication()).systemStatusChanged();
                     }
                 } finally {
-                    try { // Making absolutely sure the service stopping command will be executed
+                    // Making absolutely sure stopSelf() will be called
-                        if (K9.DEBUG)
+                    try {
-                            Log.d(K9.LOG_TAG, "CoreService (" + className + ") completed Runnable " + runner.hashCode() + " with startId " + startId);
+                        if (K9.DEBUG) {
+                            Log.d(K9.LOG_TAG, "CoreService (" + className + ") completed " +
+                                    "Runnable " + runner.hashCode() + " with startId " + startId);
+                        }
                         wakeLock.release();
                     } finally {
                         if (autoShutdown && startId != null) {
-                            stopSelf(startId); // <-- this is what is meant with "serviceShutdownScheduled"; execution of this line assures proper shutdown of the service once finished
+                            stopSelf(startId);
                         }
                     }
                 }
             }
-
         };
-        if (threadPool == null) {
+
-            Log.e(K9.LOG_TAG, "CoreService.execute (" + className + ") called with no threadPool available; running Runnable " + runner.hashCode() + " in calling thread", new Throwable());
+        // TODO: remove this. we never set mThreadPool to null
+        if (mThreadPool == null) {
+            Log.e(K9.LOG_TAG, "CoreService.execute (" + className + ") called with no thread " +
+                    "pool available; running Runnable " + runner.hashCode() +
+                    " in calling thread");
+
             synchronized (this) {
                 myRunner.run();
-                serviceShutdownScheduled = startId != null; // In this case it's not actually scheduled, it's already done, but that should never happen anyway
+                serviceShutdownScheduled = startId != null;
             }
         } else {
-            if (K9.DEBUG)
+            if (K9.DEBUG) {
-                Log.d(K9.LOG_TAG, "CoreService (" + className + ") queueing Runnable " + runner.hashCode() + " with startId " + startId);
+                Log.d(K9.LOG_TAG, "CoreService (" + className + ") queueing Runnable " +
+                        runner.hashCode() + " with startId " + startId);
+            }
+
             try {
-                threadPool.execute(myRunner);
+                mThreadPool.execute(myRunner);
                 serviceShutdownScheduled = startId != null;
             } catch (RejectedExecutionException e) {
+                // Ignore RejectedExecutionException after we shut down the thread pool in
+                // onDestroy(). Still, this should not happen!
                 if (!mShutdown) {
                     throw e;
                 }
-                Log.i(K9.LOG_TAG, "CoreService: " + className + " is shutting down, ignoring rejected execution exception: " + e.getMessage());
+
+                Log.i(K9.LOG_TAG, "CoreService: " + className + " is shutting down, ignoring " +
+                        "rejected execution exception: " + e.getMessage());
             }
         }
+
         mImmediateShutdown = !serviceShutdownScheduled;
-        return serviceShutdownScheduled;
     }
 
     /**
-     * CK:Added
+     * Subclasses need to implement this instead of overriding {@link #onStart(Intent, int)}.
-     * To implement by sub-class instead of overriding onStart.
+     *
-     * This allows CoreService to do start and end operations around the sub-class's start code.
+     * <p>
-     * Especially, CoreService will protect the start-code with a wake-lock to guarantee the service to have the required resources to do it's work.
+     * This allows {@link CoreService} to manage the service lifecycle, incl. wake lock management.
-     * CK:Q: Is this really useful (the wakelock part)? The real work is happening in the worker-thread anyway. Maybe it is because this makes sure that whatever needs to be started by the service, it can be without being interrupted by the phone going to sleep.
+     * </p>
+
      * @param intent
+     *         The Intent supplied to {@link Context#startService(Intent)}.
      * @param startId
+     *         A unique integer representing this specific request to start. Use with
+     *         {@link #stopSelfResult(int)}.
      */
     public abstract void startService(Intent intent, int startId);
 
-    @Override
-    public IBinder onBind(@SuppressWarnings("unused") Intent intent) {
-        // TODO Auto-generated method stub
-        return null;
-    }
-
     @Override
     public void onLowMemory() {
         Log.w(K9.LOG_TAG, "CoreService: " + className + ".onLowMemory() - Running low on memory");
     }
 
+    /**
+     * Clean up when the service is stopped.
+     */
     @Override
     public void onDestroy() {
-        if (K9.DEBUG)
+        if (K9.DEBUG) {
             Log.i(K9.LOG_TAG, "CoreService: " + className + ".onDestroy()");
+        }
+
+        // Shut down thread pool
         mShutdown = true;
-        threadPool.shutdown();
+        mThreadPool.shutdown();
+
         super.onDestroy();
-        //     MessagingController.getInstance(getApplication()).removeListener(mListener);
     }
 
     /**
-     * @return True if auto-shutdown is enabled
+     * Return whether or not auto shutdown is enabled.
+     *
+     * @return {@code true} iff auto shutdown is enabled.
      */
     protected boolean isAutoShutdown() {
         return mAutoShutdown;
     }
 
     /**
-     * Enable of disable auto-shutdown (enabled by default).
+     * Enable or disable auto shutdown (enabled by default).
-     * See {@#mAutoShutdown} for more information.
+     *
      * @param autoShutdown
+     *         {@code true} to enable auto shutdown. {@code false} to disable.
+     *
+     * @see #mAutoShutdown
      */
     protected void setAutoShutdown(boolean autoShutdown) {
         mAutoShutdown = autoShutdown;
     }
+
+    @Override
+    public IBinder onBind(Intent intent) {
+        // Unused
+        return null;
+    }
 }