From 819db01eadc3a95b02b2f820b483056b68c631f0 Mon Sep 17 00:00:00 2001 From: Todd Kennedy Date: Fri, 21 Jan 2011 11:58:39 -0800 Subject: [PATCH] Limit attachment background download attempts Do not retry downloading attachments infinitely. After some number of failures, black list the attachment and move on. The black list is not persisted, so, restarting the app will again try to fetch the attachments. In this way, any transient network failures will not permanently affect the ability to download attachments in the background NOTE: This is a partial fix for general background attachment downloading issues bug 3373982 Change-Id: I7f3ad9667ebebb95fbba95278b62bf40c5fce67c --- .../service/AttachmentDownloadService.java | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/com/android/email/service/AttachmentDownloadService.java b/src/com/android/email/service/AttachmentDownloadService.java index e0872674b..0f4f2caa3 100644 --- a/src/com/android/email/service/AttachmentDownloadService.java +++ b/src/com/android/email/service/AttachmentDownloadService.java @@ -64,7 +64,8 @@ public class AttachmentDownloadService extends Service implements Runnable { private static final int WATCHDOG_CHECK_INTERVAL = 15 * ((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 + private static final int MAX_DOWNLOAD_RETRIES = 5; private static final int PRIORITY_NONE = -1; @SuppressWarnings("unused") // Low priority will be used for opportunistic downloads @@ -100,6 +101,10 @@ public class AttachmentDownloadService extends Service implements Runnable { // amount plus the size of any new attachments laoded). If and when we reach the per-account // limit, we recalculate the actual usage /*package*/ final HashMap mAttachmentStorageMap = new HashMap(); + // 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(); private final Object mLock = new Object(); @@ -344,6 +349,12 @@ public class AttachmentDownloadService extends Service implements Runnable { if (info.isEligibleForDownload()) { Attachment att = Attachment.restoreAttachmentWithId(mContext, id); if (att != null) { + Integer tryCount; + tryCount = mAttachmentFailureMap.get(att.mId); + if (tryCount != null && tryCount > MAX_DOWNLOAD_RETRIES) { + // move onto the next attachment + continue; + } // Start this download and we're done DownloadRequest req = new DownloadRequest(mContext, att); mDownloadSet.tryStartDownload(req); @@ -442,6 +453,11 @@ public class AttachmentDownloadService extends Service implements Runnable { /*package*/ synchronized boolean tryStartDownload(DownloadRequest req) { Class serviceClass = getServiceClassForAccount(req.accountId); if (serviceClass == null) return false; + + // Do not download the same attachment multiple times + boolean alreadyInProgress = mDownloadsInProgress.get(req.attachmentId) != null; + if (alreadyInProgress) return false; + try { if (Email.DEBUG) { Log.d(TAG, ">> Starting download for attachment #" + req.attachmentId); @@ -468,6 +484,23 @@ public class AttachmentDownloadService extends Service implements Runnable { /*package*/ synchronized void endDownload(long attachmentId, int statusCode) { // Say we're no longer downloading this mDownloadsInProgress.remove(attachmentId); + + // TODO: This code is conservative and treats connection issues as failures. + // Since we have no mechanism to throttle reconnection attempts, it makes + // sense to be cautious here. Once logic is in place to prevent connecting + // in a tight loop, we can exclude counting connection issues as "failures". + + // Update the attachment failure list if needed + Integer downloadCount; + downloadCount = mAttachmentFailureMap.remove(attachmentId); + if (statusCode != EmailServiceStatus.SUCCESS) { + if (downloadCount == null) { + downloadCount = 0; + } + downloadCount += 1; + mAttachmentFailureMap.put(attachmentId, downloadCount); + } + DownloadRequest req = mDownloadSet.findDownloadRequest(attachmentId); if (statusCode == EmailServiceStatus.CONNECTION_ERROR) { // If this needs to be retried, just process the queue again