diff --git a/src/com/fsck/k9/service/CoreService.java b/src/com/fsck/k9/service/CoreService.java index 8e70e8ed1..d3df82cac 100644 --- a/src/com/fsck/k9/service/CoreService.java +++ b/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 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. + * + * + * 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 + * + * + */ public abstract class CoreService extends Service { - public static String WAKE_LOCK_ID = "com.fsck.k9.service.CoreService.wakeLockId"; - private static ConcurrentHashMap wakeLocks = new ConcurrentHashMap(); - 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 wakeLocks = new ConcurrentHashMap(); // 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; + } } diff --git a/src/com/fsck/k9/service/MailService.java b/src/com/fsck/k9/service/MailService.java index 06f2f193c..9a8a12d14 100644 --- a/src/com/fsck/k9/service/MailService.java +++ b/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; } - - } diff --git a/src/com/fsck/k9/service/RemoteControlService.java b/src/com/fsck/k9/service/RemoteControlService.java index fd088e462..45d1885f5 100644 --- a/src/com/fsck/k9/service/RemoteControlService.java +++ b/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); } diff --git a/src/com/fsck/k9/service/SleepService.java b/src/com/fsck/k9/service/SleepService.java index 425ee70d6..1f365454d 100644 --- a/src/com/fsck/k9/service/SleepService.java +++ b/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 {