Stop service(s) if not in use - Fixes Issue 2777

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

@@ -17,15 +17,67 @@ import com.fsck.k9.controller.MessagingController;
 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.
+ *       This documentation is without warranty and may not be accurate nor reflect the author's original intent.
+ *      
+ * <CK:RE>
+ * 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 could also be a pure service, without
+ * any UI; this is not the case of K9). By declaring a service and starting it, the OS knows that the application has
+ * work to do and should avoid killing the process.
+ * 
+ * A service's main purpose is to do some task (usually in the background) which requires one of more threads. The
+ * thread that starts the service is the same as the UI thread of the process. It should thus not be used to run
+ * the tasks.
+ * 
+ * CoreService is providing the execution plumbing for background tasks including the required thread and task queuing
+ * for all K9 services to use.
+ * 
+ * A service is supposed to run only as long as it has some work to do whether that work is active processing or some
+ * just some monitoring, like listening on a network port for incoming connections or listing on an open network
+ * connection for incoming data (push mechanism).
+ * 
+ * To make sure the service is running only when required, is must be shutdown after tasks are done. As the
+ * execution of tasks is abstracted away in this class, it also proper shutdown handling if approriate. If
+ * the Service requires this is should call enableAutoShutdown(true) in it's onCreate() method. 
+ *
+ * While a service is running it's tasks, it is usually not a good idea to let the device go to sleep more.
+ * WakeLocks are used to avoid this. CoreService provides a central registry (singleton) that can be used
+ * application-wide to store WakeLocks.
+ * 
+ * In short, CoreService provides the following features to K9 Services:
+ *  - task execution and queuing
+ *  - Service life cycle management (insures the service is stopped when not needed anymore); disabled by default
+ *  - WakeLock registry and management
+ *  
+ * </CK:RE>      
+ */
 public abstract class CoreService extends Service {
 
-    public static String WAKE_LOCK_ID = "com.fsck.k9.service.CoreService.wakeLockId";
-    private static ConcurrentHashMap<Integer, TracingWakeLock> wakeLocks = new ConcurrentHashMap<Integer, TracingWakeLock>();
-    private static AtomicInteger wakeLockSeq = new AtomicInteger(0);
-    private ExecutorService threadPool = null;
+    public static String WAKE_LOCK_ID = "com.fsck.k9.service.CoreService.wakeLockId"; // CK:Intent attribute ID
+    private static ConcurrentHashMap<Integer, TracingWakeLock> wakeLocks = new ConcurrentHashMap<Integer, TracingWakeLock>(); // CK:WakeLocks registry
+    private static AtomicInteger wakeLockSeq = new AtomicInteger(0); // CK:WakeLock registry
+    private ExecutorService threadPool = null; // CK:Threadpool with a single thread; used to execute and queue background actions inside the service
     private final String className = getClass().getName();
-    private volatile boolean mShutdown = false;
+    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.
 
+    /**
+     * CK:BF:2777
+     * Controls the auto-shutdown mechanism of the service. 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.
+     */
+    private boolean mAutoShutdown = true;
+    
+    /**
+     * CK:BF:2777
+     * This variable is part of the auto-shutdown feature and determines whether the service has to be shutdown at the
+     * end of the onStart() method or not.
+     */
+    protected boolean mImmediateShutdown = true; // 
+    
     @Override
     public void onCreate() {
         if (K9.DEBUG)
@@ -35,32 +87,70 @@ public abstract class CoreService extends Service {
 
     }
 
-    protected static void addWakeLockId(Intent i, Integer wakeLockId) {
+    /**
+     * CK:DocAdded,Refactored without change of logic
+     * Adds an existing WakeLock identified by it's WakeLock-ID to the specified Intent.
+     * @param i
+     * @param wakeLockId
+     */
+    protected static void addWakeLockId(Context context, Intent i, Integer wakeLockId, boolean createIfNotExists) {
         if (wakeLockId != null) {
             i.putExtra(BootReceiver.WAKE_LOCK_ID, wakeLockId);
+            return;
         }
+        if (createIfNotExists)
+          addWakeLock(context,i);
     }
 
+    /**
+     * CK:DocAdded
+     * Adds a new WakeLock to the 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.
+     * @param i Intent to which to add the WakeLock (CK:Q:still unclear why we need to link Intents and WakeLocks)
+     */
     protected static void addWakeLock(Context context, Intent i) {
-        TracingPowerManager pm = TracingPowerManager.getPowerManager(context);
-        TracingWakeLock wakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "CoreService addWakeLock");
-        wakeLock.setReferenceCounted(false);
-        wakeLock.acquire(K9.MAIL_SERVICE_WAKE_LOCK_TIMEOUT);
-
-        Integer tmpWakeLockId = wakeLockSeq.getAndIncrement();
-        wakeLocks.put(tmpWakeLockId, wakeLock);
-
+        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)
+        Integer tmpWakeLockId = registerWakeLock(wakeLock);
         i.putExtra(WAKE_LOCK_ID, tmpWakeLockId);
     }
 
+    /**
+     * CK:Added as result of refactoring; logic is unchanged
+     * Register WakeLock and returns its registry-entry-ID
+     * @param wakeLock
+     * @return
+     * AUTHOR chrisk
+     */
+    protected static Integer registerWakeLock(TracingWakeLock wakeLock) {
+        Integer tmpWakeLockId = wakeLockSeq.getAndIncrement();
+        wakeLocks.put(tmpWakeLockId, wakeLock);
+        return tmpWakeLockId;
+    }
 
+    /**
+     * CK:Added as result of refactoring; logic is unchanged
+     * Acquires a WakeLock in a K9 standard way
+     * @param context
+     * @return
+     * AUTHOR chrisk
+     */
+    protected static TracingWakeLock acquireWakeLock(Context context, String tag, long timeout) {
+        TracingPowerManager pm = TracingPowerManager.getPowerManager(context);
+        TracingWakeLock wakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, tag);
+        wakeLock.setReferenceCounted(false);
+        wakeLock.acquire(timeout);
+        return wakeLock;
+    }
 
     @Override
     public void onStart(Intent intent, int startId) {
-        TracingPowerManager pm = TracingPowerManager.getPowerManager(this);
-        TracingWakeLock wakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "CoreService onStart");
-        wakeLock.setReferenceCounted(false);
-        wakeLock.acquire(K9.MAIL_SERVICE_WAKE_LOCK_TIMEOUT);
+        // CK:DocAdded: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.
+        //           Also, because we create a new wakelock, we re-initialize the wakelock timeout and give
+        //           the service-start code a protection of up to MAIL_SERVICE_WAKE_LOCK_TIMEOUT (currently 30s).
+        TracingWakeLock wakeLock = acquireWakeLock(this,"CoreService onStart",K9.MAIL_SERVICE_WAKE_LOCK_TIMEOUT);
 
         if (K9.DEBUG)
             Log.i(K9.LOG_TAG, "CoreService: " + className + ".onStart(" + intent + ", " + startId);
@@ -81,20 +171,30 @@ public abstract class CoreService extends Service {
             }
         }
 
+        // CK:DocAdded: Run the actual start-code of the service
+        mImmediateShutdown = true;
         try {
             super.onStart(intent, startId);
             startService(intent, startId);
         } finally {
-            wakeLock.release();
+            try{wakeLock.release();} catch (Exception e) {/* ignore */}
+            try{if (mAutoShutdown && mImmediateShutdown && startId != -1) stopSelf(startId);} catch (Exception e) {/* ignore */} 
         }
     }
 
-    public void execute(Context context, final Runnable runner, int wakeLockTime, final Integer startId) {
+    /**
+     * 
+     * @param context
+     * @param runner
+     * @param wakeLockTime
+     * @param startId
+     * @return CK:BF:2777: returns whether service-shutdown will actually happen after the task has been executed (or has already been done).
+     */
+    public boolean execute(Context context, final Runnable runner, int wakeLockTime, final Integer startId) {
 
-        TracingPowerManager pm = TracingPowerManager.getPowerManager(context);
-        final TracingWakeLock wakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "CoreService execute");
-        wakeLock.setReferenceCounted(false);
-        wakeLock.acquire(wakeLockTime);
+        boolean serviceShutdownScheduled = false; // CK:BF:2777
+        final TracingWakeLock wakeLock = acquireWakeLock(context,"CoreService execute",wakeLockTime);
+        final boolean autoShutdown = mAutoShutdown;
 
         Runnable myRunner = new Runnable() {
             public void run() {
@@ -107,11 +207,14 @@ public abstract class CoreService extends Service {
                         MessagingController.getInstance(getApplication()).systemStatusChanged();
                     }
                 } finally {
-                    if (K9.DEBUG)
-                        Log.d(K9.LOG_TAG, "CoreService (" + className + ") completed Runnable " + runner.hashCode() + " with startId " + startId);
-                    wakeLock.release();
-                    if (startId != null) {
-                        stopSelf(startId);
+                    try { // CK:BF:2777:Making absolutely sure the service stopping command will be executed
+                        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); // CK:BF:2777<-- this is what is meant with "serviceShutdownScheduled"; execution of this line assures proper shutdown of the service once finished
+                        }
                     }
                 }
             }
@@ -121,12 +224,14 @@ public abstract class CoreService extends Service {
             Log.e(K9.LOG_TAG, "CoreService.execute (" + className + ") called with no threadPool available; running Runnable " + runner.hashCode() + " in calling thread", new Throwable());
             synchronized (this) {
                 myRunner.run();
+                serviceShutdownScheduled = startId != null; // CK:BF:2777; In this case it's not actually scheduled, it's already done, but that should never happen anyway
             }
         } else {
             if (K9.DEBUG)
                 Log.d(K9.LOG_TAG, "CoreService (" + className + ") queueing Runnable " + runner.hashCode() + " with startId " + startId);
             try {
                 threadPool.execute(myRunner);
+                serviceShutdownScheduled = startId != null; // CK:BF:2777
             } catch (RejectedExecutionException e) {
                 if (!mShutdown) {
                     throw e;
@@ -134,12 +239,23 @@ public abstract class CoreService extends Service {
                 Log.i(K9.LOG_TAG, "CoreService: " + className + " is shutting down, ignoring rejected execution exception: " + e.getMessage());
             }
         }
+        mImmediateShutdown = !serviceShutdownScheduled;  // CK:BF:2777
+        return serviceShutdownScheduled; // CK:BF:2777
     }
 
+    /**
+     * CK:Added
+     * 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.
+     * 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.
+     * 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.
+     * @param intent
+     * @param startId
+     */
     public abstract void startService(Intent intent, int startId);
 
     @Override
-    public IBinder onBind(Intent arg0) {
+    public IBinder onBind(@SuppressWarnings("unused") Intent intent) {
         // TODO Auto-generated method stub
         return null;
     }
@@ -158,4 +274,22 @@ public abstract class CoreService extends Service {
         super.onDestroy();
         //     MessagingController.getInstance(getApplication()).removeListener(mListener);
     }
+
+    /**
+     * CK:BF:2777
+     * @return True if auto-shutdown is enabled
+     */
+    protected boolean isAutoShutdown() {
+        return mAutoShutdown;
+    }
+
+    /**
+     * CK:BF:2777
+     * Enable of disable auto-shutdown (enabled by default).
+     * See {@#mAutoShutdown} for more information.
+     * @param autoShutdown
+     */
+    protected void setAutoShutdown(boolean autoShutdown) {
+        mAutoShutdown = autoShutdown;
+    }
 }

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

@@ -22,6 +22,10 @@ import com.fsck.k9.helper.AutoSyncHelper;
 import com.fsck.k9.mail.Pusher;
 
 /**
+ * Bug-fix 2777:
+ *   MailService was not properly shutting down when not needed anymore.
+ *   Flawed shutdown logic has been removed and consolidated into CoreService
+ *   as the auto-shutdown feature.
  */
 public class MailService extends CoreService {
     private static final String ACTION_CHECK_MAIL = "com.fsck.k9.intent.action.MAIL_SERVICE_WAKEUP";
@@ -42,10 +46,7 @@ public class MailService extends CoreService {
         Intent i = new Intent();
         i.setClass(context, MailService.class);
         i.setAction(MailService.ACTION_RESET);
-        addWakeLockId(i, wakeLockId);
-        if (wakeLockId == null) {
-            addWakeLock(context, i);
-        }
+        addWakeLockId(context, i, wakeLockId, true);
         context.startService(i);
     }
 
@@ -53,10 +54,7 @@ public class MailService extends CoreService {
         Intent i = new Intent();
         i.setClass(context, MailService.class);
         i.setAction(MailService.ACTION_RESTART_PUSHERS);
-        addWakeLockId(i, wakeLockId);
-        if (wakeLockId == null) {
-            addWakeLock(context, i);
-        }
+        addWakeLockId(context, i, wakeLockId, true);
         context.startService(i);
     }
 
@@ -64,10 +62,7 @@ public class MailService extends CoreService {
         Intent i = new Intent();
         i.setClass(context, MailService.class);
         i.setAction(MailService.ACTION_RESCHEDULE_POLL);
-        addWakeLockId(i, wakeLockId);
-        if (wakeLockId == null) {
-            addWakeLock(context, i);
-        }
+        addWakeLockId(context, i, wakeLockId, true);
         context.startService(i);
     }
 
@@ -75,7 +70,7 @@ public class MailService extends CoreService {
         Intent i = new Intent();
         i.setClass(context, MailService.class);
         i.setAction(MailService.ACTION_CANCEL);
-        addWakeLockId(i, wakeLockId);
+        addWakeLockId(context, i, wakeLockId, false); // CK:Q: why should we not create a wake lock if one is not already existing like for example in actionReschedulePoll?
         context.startService(i);
     }
 
@@ -83,7 +78,7 @@ public class MailService extends CoreService {
         Intent i = new Intent();
         i.setClass(context, MailService.class);
         i.setAction(MailService.CONNECTIVITY_CHANGE);
-        addWakeLockId(i, wakeLockId);
+        addWakeLockId(context, i, wakeLockId, false); // CK:Q: why should we not create a wake lock if one is not already existing like for example in actionReschedulePoll?
         context.startService(i);
     }
 
@@ -96,7 +91,6 @@ public class MailService extends CoreService {
 
     @Override
     public void startService(Intent intent, int startId) {
-        Integer startIdObj = startId;
         long startTime = System.currentTimeMillis();
         try {
             boolean oldIsSyncDisabled = isSyncDisabled();
@@ -147,45 +141,33 @@ public class MailService extends CoreService {
             if (ACTION_CHECK_MAIL.equals(intent.getAction())) {
                 if (K9.DEBUG)
                     Log.i(K9.LOG_TAG, "***** MailService *****: checking mail");
-
                 if (hasConnectivity && doBackground) {
                     PollService.startService(this);
                 }
-                reschedulePoll(hasConnectivity, doBackground, startIdObj, false);
-                startIdObj = null;
+                reschedulePoll(hasConnectivity, doBackground, startId, false); // CK:BF:2777
             } else if (ACTION_CANCEL.equals(intent.getAction())) {
                 if (K9.DEBUG)
                     Log.v(K9.LOG_TAG, "***** MailService *****: cancel");
-
                 cancel();
             } else if (ACTION_RESET.equals(intent.getAction())) {
                 if (K9.DEBUG)
                     Log.v(K9.LOG_TAG, "***** MailService *****: reschedule");
-
-                rescheduleAll(hasConnectivity, doBackground, startIdObj);
-                startIdObj = null;
-
+                rescheduleAll(hasConnectivity, doBackground, startId);  // CK:BF:2777
             } else if (ACTION_RESTART_PUSHERS.equals(intent.getAction())) {
                 if (K9.DEBUG)
                     Log.v(K9.LOG_TAG, "***** MailService *****: restarting pushers");
-                reschedulePushers(hasConnectivity, doBackground, startIdObj);
-                startIdObj = null;
-
+                reschedulePushers(hasConnectivity, doBackground, startId); // CK:BF:2777
             } else if (ACTION_RESCHEDULE_POLL.equals(intent.getAction())) {
                 if (K9.DEBUG)
                     Log.v(K9.LOG_TAG, "***** MailService *****: rescheduling poll");
-                reschedulePoll(hasConnectivity, doBackground, startIdObj, true);
-                startIdObj = null;
-
+                reschedulePoll(hasConnectivity, doBackground, startId, true); // CK:BF:2777
             } else if (ACTION_REFRESH_PUSHERS.equals(intent.getAction())) {
                 if (hasConnectivity && doBackground) {
                     refreshPushers(null);
-                    schedulePushers(startIdObj);
-                    startIdObj = null;
+                    schedulePushers(startId); // CK:BF:2777
                 }
             } else if (CONNECTIVITY_CHANGE.equals(intent.getAction())) {
-                rescheduleAll(hasConnectivity, doBackground, startIdObj);
-                startIdObj = null;
+                rescheduleAll(hasConnectivity, doBackground, startId); // CK:BF:2777
                 if (K9.DEBUG)
                     Log.i(K9.LOG_TAG, "Got connectivity action with hasConnectivity = " + hasConnectivity + ", doBackground = " + doBackground);
             } else if (CANCEL_CONNECTIVITY_NOTICE.equals(intent.getAction())) {
@@ -194,9 +176,7 @@ public class MailService extends CoreService {
                 MessagingController.getInstance(getApplication()).systemStatusChanged();
             }
         } finally {
-            if (startIdObj != null) {
-                stopSelf(startId);
-            }
+          /* nothing to do */
         }
         if (K9.DEBUG)
             Log.i(K9.LOG_TAG, "MailService.onStart took " + (System.currentTimeMillis() - startTime) + "ms");
@@ -204,8 +184,7 @@ public class MailService extends CoreService {
 
     private void rescheduleAll(final boolean hasConnectivity, final boolean doBackground, final Integer startId) {
         reschedulePoll(hasConnectivity, doBackground, null, true);
-        reschedulePushers(hasConnectivity, doBackground, startId);
-
+        reschedulePushers(hasConnectivity, doBackground, startId); // CK:BF:2777
     }
 
 
@@ -228,7 +207,6 @@ public class MailService extends CoreService {
     private final static String LAST_CHECK_END = "MailService.lastCheckEnd";
 
     public static void saveLastCheckEnd(Context context) {
-
         long lastCheckEnd = System.currentTimeMillis();
         if (K9.DEBUG)
             Log.i(K9.LOG_TAG, "Saving lastCheckEnd = " + new Date(lastCheckEnd));
@@ -320,7 +298,6 @@ public class MailService extends CoreService {
     private void reschedulePushers(final boolean hasConnectivity, final boolean doBackground, final Integer startId) {
         execute(getApplication(), new Runnable() {
             public void run() {
-
                 if (K9.DEBUG)
                     Log.i(K9.LOG_TAG, "Rescheduling pushers");
                 stopPushers(null);
@@ -330,7 +307,6 @@ public class MailService extends CoreService {
                 } else {
                     if (K9.DEBUG) {
                         Log.i(K9.LOG_TAG, "Not scheduling pushers:  connectivity? " + hasConnectivity + " -- doBackground? " + doBackground);
-
                     }
                 }
 
@@ -433,13 +409,11 @@ public class MailService extends CoreService {
 
 
     @Override
-    public IBinder onBind(Intent intent) {
+    public IBinder onBind(@SuppressWarnings("unused") Intent intent) {
         return null;
     }
 
     public static long getNextPollTime() {
         return nextCheck;
     }
-
-
 }

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

@@ -27,10 +27,7 @@ public class RemoteControlService extends CoreService {
         //  Intent i = new Intent();
         i.setClass(context, RemoteControlService.class);
         i.setAction(RemoteControlService.SET_ACTION);
-        addWakeLockId(i, wakeLockId);
-        if (wakeLockId == null) {
-            addWakeLock(context, i);
-        }
+        addWakeLockId(context, i, wakeLockId, true);
         context.startService(i);
     }
 

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

@@ -114,11 +114,15 @@ public class SleepService extends CoreService {
 
     @Override
     public void startService(Intent intent, int startId) {
-        if (intent.getAction().startsWith(ALARM_FIRED)) {
-            Integer id = intent.getIntExtra(LATCH_ID, -1);
-            endSleep(id);
+        try {
+          if (intent.getAction().startsWith(ALARM_FIRED)) {
+              Integer id = intent.getIntExtra(LATCH_ID, -1);
+              endSleep(id);
+          }
+        }
+        finally {
+          stopSelf(startId);
         }
-        stopSelf(startId);
     }
 
     private static class SleepDatum {