diff --git a/src/com/android/email/service/AttachmentService.java b/src/com/android/email/service/AttachmentService.java index f6514e15b..745a86e57 100644 --- a/src/com/android/email/service/AttachmentService.java +++ b/src/com/android/email/service/AttachmentService.java @@ -53,6 +53,7 @@ import com.google.common.annotations.VisibleForTesting; import java.io.File; import java.io.FileDescriptor; import java.io.PrintWriter; +import java.util.Collection; import java.util.Comparator; import java.util.HashMap; import java.util.Iterator; @@ -75,8 +76,6 @@ public class AttachmentService extends Service implements Runnable { // Our idle time, waiting for notifications; this is something of a failsafe private static final int PROCESS_QUEUE_WAIT_TIME = 30 * ((int)DateUtils.MINUTE_IN_MILLIS); - // How often our watchdog checks for callback timeouts - private static final int WATCHDOG_CHECK_INTERVAL = 20 * ((int)DateUtils.SECOND_IN_MILLIS); // How long we'll wait for a callback before canceling a download and retrying private static final int CALLBACK_TIMEOUT = 30 * ((int)DateUtils.SECOND_IN_MILLIS); // Try to download an attachment in the background this many times before giving up @@ -105,79 +104,45 @@ public class AttachmentService extends Service implements Runnable { // Limit on the number of attachments we'll check for background download private static final int MAX_ATTACHMENTS_TO_CHECK = 25; - private static final String EXTRA_ATTACHMENT = - "com.android.email.AttachmentService.attachment"; + private static final String EXTRA_ATTACHMENT = "com.android.email.AttachmentService.attachment"; + + // This callback is invoked by the various service backends to give us download progress + // since those modules are responsible for the actual download. + private final ServiceCallback mServiceCallback = new ServiceCallback(); // sRunningService is only set in the UI thread; it's visibility elsewhere is guaranteed // by the use of "volatile" /*package*/ static volatile AttachmentService sRunningService = null; + // Signify that we are being shut down & destroyed. + private volatile boolean mStop = false; + /*package*/ Context mContext; /*package*/ EmailConnectivityManager mConnectivityManager; + /*package*/ final AttachmentWatchdog mWatchdog = new AttachmentWatchdog(); - /*package*/ final DownloadSet mDownloadSet = new DownloadSet(new DownloadComparator()); + private final Object mLock = new Object(); - private final HashMap mAccountServiceMap = new HashMap(); - // A map of attachment storage used per account + // A map of attachment storage used per account as we have account based maximums to follow. // NOTE: This map is not kept current in terms of deletions (i.e. it stores the last calculated - // amount plus the size of any new attachments laoded). If and when we reach the per-account + // amount plus the size of any new attachments loaded). If and when we reach the per-account // limit, we recalculate the actual usage - /*package*/ final HashMap mAttachmentStorageMap = new HashMap(); + /*package*/ final ConcurrentHashMap mAttachmentStorageMap = + new ConcurrentHashMap(); + // A map of attachment ids to the number of failed attempts to download the attachment // NOTE: We do not want to persist this. This allows us to retry background downloading // if any transient network errors are fixed & and the app is restarted - /* package */ final HashMap mAttachmentFailureMap = new HashMap(); - private final ServiceCallback mServiceCallback = new ServiceCallback(); + /* package */ final ConcurrentHashMap mAttachmentFailureMap = + new ConcurrentHashMap(); - private final Object mLock = new Object(); - private volatile boolean mStop = false; + // Keeps tracks of downloads in progress based on an attachment ID to DownloadRequest mapping. + /*package*/ final ConcurrentHashMap mDownloadsInProgress = + new ConcurrentHashMap(); - /*package*/ AccountManagerStub mAccountManagerStub; - - /** - * We only use the getAccounts() call from AccountManager, so this class wraps that call and - * allows us to build a mock account manager stub in the unit tests - */ - /*package*/ static class AccountManagerStub { - private int mNumberOfAccounts; - private final AccountManager mAccountManager; - - AccountManagerStub(Context context) { - if (context != null) { - mAccountManager = AccountManager.get(context); - } else { - mAccountManager = null; - } - } - - /*package*/ int getNumberOfAccounts() { - if (mAccountManager != null) { - return mAccountManager.getAccounts().length; - } else { - return mNumberOfAccounts; - } - } - - /*package*/ void setNumberOfAccounts(int numberOfAccounts) { - mNumberOfAccounts = numberOfAccounts; - } - } - - /** - * Watchdog alarm receiver; responsible for making sure that downloads in progress are not - * stalled, as determined by the timing of the most recent service callback - */ - public static class Watchdog extends BroadcastReceiver { - @Override - public void onReceive(final Context context, Intent intent) { - new Thread(new Runnable() { - @Override - public void run() { - watchdogAlarm(); - } - }, "AttachmentService Watchdog").start(); - } - } + // TODO: Remove in favor of the DownloadQueue when we are ready to move over (soon). + /*package*/ final DownloadSet mDownloadSet = new DownloadSet(new DownloadComparator()); + /*package*/ final DownloadQueue mDownloadQueue = new DownloadQueue(); /** * This class is used to contain the details and state of a particular request to download @@ -265,8 +230,10 @@ public class AttachmentService extends Service implements Runnable { /** * Comparator class for the download set; we first compare by priority. Requests with equal * priority are compared by the time the request was created (older requests come first) + * TODO: Move this into the DownloadQueue as a private static class when we finally remove + * the DownloadSet class from the implementation. */ - /*package*/ static class DownloadComparator implements Comparator { + private static class DownloadComparator implements Comparator { @Override public int compare(DownloadRequest req1, DownloadRequest req2) { int res; @@ -300,8 +267,7 @@ public class AttachmentService extends Service implements Runnable { // For prioritization of DownloadRequests. /*package*/ final PriorityQueue mRequestQueue = - new PriorityQueue(DEFAULT_SIZE, - new AttachmentService.DownloadComparator()); + new PriorityQueue(DEFAULT_SIZE, new DownloadComparator()); // Secondary collection to quickly find objects w/o the help of an iterator. // This class should be kept in lock step with the priority queue. @@ -314,12 +280,13 @@ public class AttachmentService extends Service implements Runnable { * @param request The {@link DownloadRequest} that should be added to our queue * @return true if it was added (or already exists), false otherwise */ - public synchronized boolean addRequest(final DownloadRequest request) { + public synchronized boolean addRequest(final DownloadRequest request) + throws NullPointerException { // It is key to keep the map and queue in lock step if (request == null) { - // We can't add a null entry into the queue - LogUtils.wtf(AttachmentService.LOG_TAG, "Adding a null DownloadRequest"); - return false; + // We can't add a null entry into the queue so let's throw what the underlying + // data structure would throw. + throw new NullPointerException(); } final long requestId = request.mAttachmentId; if (requestId < 0) { @@ -399,7 +366,122 @@ public class AttachmentService extends Service implements Runnable { } } - /** + /** + * Watchdog alarm receiver; responsible for making sure that downloads in progress are not + * stalled, as determined by the timing of the most recent service callback + */ + public static class AttachmentWatchdog extends BroadcastReceiver { + // How often our watchdog checks for callback timeouts + private static final int WATCHDOG_CHECK_INTERVAL = 20 * ((int)DateUtils.SECOND_IN_MILLIS); + public static final String EXTRA_CALLBACK_TIMEOUT = "callback_timeout"; + private PendingIntent mWatchdogPendingIntent; + + public void setWatchdogAlarm(final Context context, final long delay, + final int callbackTimeout) { + // Lazily initialize the pending intent + if (mWatchdogPendingIntent == null) { + Intent intent = new Intent(context, AttachmentWatchdog.class); + intent.putExtra(EXTRA_CALLBACK_TIMEOUT, callbackTimeout); + mWatchdogPendingIntent = + PendingIntent.getBroadcast(context, 0, intent, 0); + } + // Set the alarm + final AlarmManager am = (AlarmManager)context.getSystemService(Context.ALARM_SERVICE); + am.set(AlarmManager.RTC_WAKEUP, System.currentTimeMillis() + delay, + mWatchdogPendingIntent); + } + + public void setWatchdogAlarm(final Context context) { + // Call the real function with default values. + setWatchdogAlarm(context, WATCHDOG_CHECK_INTERVAL, CALLBACK_TIMEOUT); + } + + @Override + public void onReceive(final Context context, final Intent intent) { + final int callbackTimeout = intent.getIntExtra(EXTRA_CALLBACK_TIMEOUT, + CALLBACK_TIMEOUT); + new Thread(new Runnable() { + @Override + public void run() { + final AttachmentService service = AttachmentService.sRunningService; + if (service != null) { + // If our service instance is gone, just leave + if (service.mStop) { + return; + } + // Get the timeout time from the intent. + watchdogAlarm(service, callbackTimeout); + } + } + }, "AttachmentService AttachmentWatchdog").start(); + } + + /** + * Watchdog for downloads; we use this in case we are hanging on a download, which might + * have failed silently (the connection dropped, for example) + */ + /*package*/ void watchdogAlarm(final AttachmentService service, final int callbackTimeout) { + final long now = System.currentTimeMillis(); + // We want to iterate on each of the downloads that are currently in progress and + // cancel the ones that seem to be taking too long. + final Collection inProgressRequests = service.getInProgressDownloads(); + for (DownloadRequest req: inProgressRequests) { + // Check how long it's been since receiving a callback + final long timeSinceCallback = now - req.mLastCallbackTime; + if (timeSinceCallback > callbackTimeout) { + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + LogUtils.d(LOG_TAG, "== Download of " + req.mAttachmentId + " timed out"); + } + service.cancelDownload(req); + } + } + // Check whether we can start new downloads... + if (service.isConnected()) { + service.processQueue(); + } + if (service.areDownloadsInProgress()) { + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + LogUtils.d(LOG_TAG, "Reschedule watchdog..."); + } + setWatchdogAlarm(service); + } + } + } + + /** + * Temporary function implemented as a transition between DownloadSet to DownloadQueue. + * Will be property implemented and documented in a subsequent CL. + * @param req The {@link DownloadRequest} to be cancelled. + */ + /*package*/ void cancelDownload(final DownloadRequest req) { + mDownloadSet.cancelDownload(req); + return; + } + + /** + * Temporary function implemented as a transition between DownloadSet to DownloadQueue + */ + /*package*/ void processQueue() { + mDownloadSet.processQueue(); + return; + } + + /*package*/ boolean isConnected() { + if (mConnectivityManager != null) { + return mConnectivityManager.hasConnectivity(); + } + return false; + } + + /*package*/ Collection getInProgressDownloads() { + return mDownloadsInProgress.values(); + } + + /*package*/ boolean areDownloadsInProgress() { + return !mDownloadsInProgress.isEmpty(); + } + + /** * The DownloadSet is a TreeSet sorted by priority class (e.g. low, high, etc.) and the * time of the request. Higher priority requests * are always processed first; among equals, the oldest request is processed first. The @@ -408,18 +490,11 @@ public class AttachmentService extends Service implements Runnable { */ /*package*/ class DownloadSet extends TreeSet { private static final long serialVersionUID = 1L; - private PendingIntent mWatchdogPendingIntent; /*package*/ DownloadSet(Comparator comparator) { super(comparator); } - /** - * Maps attachment id to DownloadRequest - */ - /*package*/ final ConcurrentHashMap mDownloadsInProgress = - new ConcurrentHashMap(); - private void markAttachmentAsFailed(final Attachment att) { final ContentValues cv = new ContentValues(); final int flags = Attachment.FLAG_DOWNLOAD_FORWARD | Attachment.FLAG_DOWNLOAD_USER_REQUEST; @@ -541,7 +616,8 @@ public class AttachmentService extends Service implements Runnable { final long currentTime = SystemClock.elapsedRealtime(); if (req.mRetryCount > 0 && req.mRetryStartTime > currentTime) { LogUtils.d(LOG_TAG, "== waiting to retry attachment %d", req.mAttachmentId); - setWatchdogAlarm(CONNECTION_ERROR_RETRY_MILLIS); + mWatchdog.setWatchdogAlarm(mContext, CONNECTION_ERROR_RETRY_MILLIS, + CALLBACK_TIMEOUT); continue; } // TODO: We try to gate ineligible downloads from entering the queue but its @@ -632,39 +708,6 @@ public class AttachmentService extends Service implements Runnable { return count; } - /** - * Watchdog for downloads; we use this in case we are hanging on a download, which might - * have failed silently (the connection dropped, for example) - */ - private void onWatchdogAlarm() { - // If our service instance is gone, just leave - if (mStop) { - return; - } - long now = System.currentTimeMillis(); - for (DownloadRequest req: mDownloadsInProgress.values()) { - // Check how long it's been since receiving a callback - long timeSinceCallback = now - req.mLastCallbackTime; - if (timeSinceCallback > CALLBACK_TIMEOUT) { - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "== Download of " + req.mAttachmentId + " timed out"); - } - cancelDownload(req); - } - } - // Check whether we can start new downloads... - if (mConnectivityManager != null && mConnectivityManager.hasConnectivity()) { - processQueue(); - } - // If there are downloads in progress, reset alarm - if (!mDownloadsInProgress.isEmpty()) { - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "Reschedule watchdog..."); - } - setWatchdogAlarm(); - } - } - /** * Attempt to execute the DownloadRequest, enforcing the maximum downloads per account * parameter @@ -696,22 +739,6 @@ public class AttachmentService extends Service implements Runnable { return mDownloadsInProgress.get(attachmentId); } - private void setWatchdogAlarm(final long delay) { - // Lazily initialize the pending intent - if (mWatchdogPendingIntent == null) { - Intent intent = new Intent(mContext, Watchdog.class); - mWatchdogPendingIntent = - PendingIntent.getBroadcast(mContext, 0, intent, 0); - } - // Set the alarm - AlarmManager am = (AlarmManager)mContext.getSystemService(Context.ALARM_SERVICE); - am.set(AlarmManager.RTC_WAKEUP, System.currentTimeMillis() + delay, - mWatchdogPendingIntent); - } - - private void setWatchdogAlarm() { - setWatchdogAlarm(WATCHDOG_CHECK_INTERVAL); - } /** * Do the work of starting an attachment download using the EmailService interface, and @@ -728,10 +755,10 @@ public class AttachmentService extends Service implements Runnable { mDownloadsInProgress.put(req.mAttachmentId, req); service.loadAttachment(mServiceCallback, req.mAccountId, req.mAttachmentId, req.mPriority != PRIORITY_FOREGROUND); - setWatchdogAlarm(); + mWatchdog.setWatchdogAlarm(mContext); } - private void cancelDownload(DownloadRequest req) { + /*package*/ synchronized void cancelDownload(DownloadRequest req) { LogUtils.d(LOG_TAG, "cancelDownload #%d", req.mAttachmentId); req.mInProgress = false; mDownloadsInProgress.remove(req.mAttachmentId); @@ -796,7 +823,8 @@ public class AttachmentService extends Service implements Runnable { req.mInProgress = false; req.mRetryStartTime = SystemClock.elapsedRealtime() + CONNECTION_ERROR_RETRY_MILLIS; - setWatchdogAlarm(CONNECTION_ERROR_RETRY_MILLIS); + mWatchdog.setWatchdogAlarm(mContext, CONNECTION_ERROR_RETRY_MILLIS, + CALLBACK_TIMEOUT); } else { LogUtils.d(LOG_TAG, "ConnectionError #%d, retried %d times, adding delay", attachmentId, req.mRetryCount); @@ -962,10 +990,6 @@ public class AttachmentService extends Service implements Runnable { } } - /*package*/ void addServiceIntentForTest(long accountId, Intent intent) { - mAccountServiceMap.put(accountId, intent); - } - /*package*/ void onChange(Attachment att) { mDownloadSet.onChange(this, att); } @@ -1028,13 +1052,6 @@ public class AttachmentService extends Service implements Runnable { return false; } - public static void watchdogAlarm() { - AttachmentService service = sRunningService; - if (service != null) { - service.mDownloadSet.onWatchdogAlarm(); - } - } - // The queue entries here are entries of the form {id, flags}, with the values passed in to // attachmentChanged() private static final Queue sAttachmentChangedQueue = @@ -1236,6 +1253,7 @@ public class AttachmentService extends Service implements Runnable { } } + // For Debugging. @Override public void dump(FileDescriptor fd, PrintWriter pw, String[] args) { pw.println("AttachmentService"); @@ -1285,4 +1303,41 @@ public class AttachmentService extends Service implements Runnable { } } } + + // For Testing + /*package*/ AccountManagerStub mAccountManagerStub; + private final HashMap mAccountServiceMap = new HashMap(); + + /*package*/ void addServiceIntentForTest(long accountId, Intent intent) { + mAccountServiceMap.put(accountId, intent); + } + + /** + * We only use the getAccounts() call from AccountManager, so this class wraps that call and + * allows us to build a mock account manager stub in the unit tests + */ + /*package*/ static class AccountManagerStub { + private int mNumberOfAccounts; + private final AccountManager mAccountManager; + + AccountManagerStub(Context context) { + if (context != null) { + mAccountManager = AccountManager.get(context); + } else { + mAccountManager = null; + } + } + + /*package*/ int getNumberOfAccounts() { + if (mAccountManager != null) { + return mAccountManager.getAccounts().length; + } else { + return mNumberOfAccounts; + } + } + + /*package*/ void setNumberOfAccounts(int numberOfAccounts) { + mNumberOfAccounts = numberOfAccounts; + } + } } diff --git a/tests/src/com/android/email/service/AttachmentServiceTests.java b/tests/src/com/android/email/service/AttachmentServiceTests.java index 044721535..235e3d4c5 100644 --- a/tests/src/com/android/email/service/AttachmentServiceTests.java +++ b/tests/src/com/android/email/service/AttachmentServiceTests.java @@ -58,8 +58,13 @@ public class AttachmentServiceTests extends TestCase { public void testDownloadQueueAddRequestNull() { final AttachmentService.DownloadQueue dq = new AttachmentService.DownloadQueue(); - final boolean result = dq.addRequest(null); - assertFalse(result); + boolean exceptionThrown = false; + try { + dq.addRequest(null); + } catch (NullPointerException ex) { + exceptionThrown = true; + } + assertTrue(exceptionThrown); assertEquals(0, dq.getSize()); assertTrue(dq.isEmpty()); } @@ -387,4 +392,91 @@ public class AttachmentServiceTests extends TestCase { lastTime = requestTime; } } + + /** + * This function will test the function AttachmentWatchdog.watchdogAlarm() that is executed + * whenever the onReceive() call is made by the AlarmManager + */ + public void testAttachmentWatchdogAlarm() { + final MockAttachmentService mockAttachmentService = new MockAttachmentService(); + + // Add a couple of items to the in-progress queue + final AttachmentService.AttachmentWatchdog testWatchdog = + new AttachmentService.AttachmentWatchdog(); + + // Add one download request object to the in process map that should + // should not need to be cancelled. + final AttachmentService.DownloadRequest dr = + new AttachmentService.DownloadRequest(AttachmentService.PRIORITY_FOREGROUND, 1); + dr.mLastCallbackTime = System.currentTimeMillis(); + mockAttachmentService.mDownloadsInProgress.put(dr.mAttachmentId, dr); + + // Set the alarm to delay 1 second and too look for attachments that have been + // not updated for 60 seconds. + testWatchdog.watchdogAlarm(mockAttachmentService, 60000); + + // Now check the results. The code should have called not cancelled anything but should + // have called processQueue() + assertTrue(mockAttachmentService.mCalledProcessQueue); + assertFalse(mockAttachmentService.mCalledCancel); + } + + /** + * This function will test the function AttachmentWatchdog.watchdogAlarm() that is executed + * whenever the onReceive() call is made by the AlarmManager + */ + public void testAttachmentWatchdogAlarmNeedsCancel() { + final MockAttachmentService mockAttachmentService = new MockAttachmentService(); + + // Add a couple of items to the in-progress queue + final AttachmentService.AttachmentWatchdog testWatchdog = + new AttachmentService.AttachmentWatchdog(); + + // Add one download request object to the in process map that should + // be cancelled by the time the callback is executed. + final AttachmentService.DownloadRequest dr = + new AttachmentService.DownloadRequest(AttachmentService.PRIORITY_FOREGROUND, 1); + dr.mLastCallbackTime = System.currentTimeMillis() - 60000; + mockAttachmentService.mDownloadsInProgress.put(dr.mAttachmentId, dr); + + // Set the alarm to delay 1 second and too look for attachments that have been + // not updated for 10 seconds. + // Set the alarm to delay 1 second and too look for attachments that have been + // not updated for 60 seconds. + testWatchdog.watchdogAlarm(mockAttachmentService, 1000); + + // Now check the results. The code should have called both cancelDownload and + // processQueue() + assertTrue(mockAttachmentService.mCalledProcessQueue); + assertTrue(mockAttachmentService.mCalledCancel); + } + + // Mock test class to stub out a couple of functions but record that they were called. + class MockAttachmentService extends AttachmentService { + // For AttachmentWatchdog tests to see if certain functions were called. + public boolean mCalledCancel = false; + public boolean mCalledProcessQueue = false; + + public MockAttachmentService() { + sRunningService = this; + } + + @Override + boolean isConnected() { return true; } + + @Override + void cancelDownload(final DownloadRequest req) { + mCalledCancel = true; + } + + @Override + void processQueue() { + mCalledProcessQueue = true; + } + + // Forcing this to be false so we don't reset the alarm during testing. + @Override + boolean areDownloadsInProgress() { return false; } + + } }