From e33511699a9b091a6e216d29e4f3e7e756ee6eca Mon Sep 17 00:00:00 2001 From: Anthony Lee Date: Thu, 22 May 2014 09:07:31 -0700 Subject: [PATCH] Cleanup of AttachmentService code and enabling of DownloadQueue. Removed unused functions, 'final'ed variables and parameters, revisited some thread synchronization. Effectively no new code introduced so no unit tests for this CL. Change-Id: I714724a98d31a231ab10b0ad468b8efaa460bd1d --- .../email/service/AttachmentService.java | 1179 ++++++++--------- 1 file changed, 548 insertions(+), 631 deletions(-) diff --git a/src/com/android/email/service/AttachmentService.java b/src/com/android/email/service/AttachmentService.java index 745a86e57..663bbe1ef 100644 --- a/src/com/android/email/service/AttachmentService.java +++ b/src/com/android/email/service/AttachmentService.java @@ -56,10 +56,8 @@ import java.io.PrintWriter; import java.util.Collection; import java.util.Comparator; import java.util.HashMap; -import java.util.Iterator; import java.util.PriorityQueue; import java.util.Queue; -import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedQueue; @@ -81,15 +79,15 @@ public class AttachmentService extends Service implements Runnable { // Try to download an attachment in the background this many times before giving up private static final int MAX_DOWNLOAD_RETRIES = 5; - /* package */ static final int PRIORITY_NONE = -1; + static final int PRIORITY_NONE = -1; // High priority is for user requests - /* package */ static final int PRIORITY_FOREGROUND = 0; - /* package */ static final int PRIORITY_HIGHEST = PRIORITY_FOREGROUND; + static final int PRIORITY_FOREGROUND = 0; + static final int PRIORITY_HIGHEST = PRIORITY_FOREGROUND; // Normal priority is for forwarded downloads in outgoing mail - /* package */ static final int PRIORITY_SEND_MAIL = 1; + static final int PRIORITY_SEND_MAIL = 1; // Low priority will be used for opportunistic downloads - /* package */ static final int PRIORITY_BACKGROUND = 2; - /* package */ static final int PRIORITY_LOWEST = PRIORITY_BACKGROUND; + static final int PRIORITY_BACKGROUND = 2; + static final int PRIORITY_LOWEST = PRIORITY_BACKGROUND; // Minimum free storage in order to perform prefetch (25% of total memory) private static final float PREFETCH_MINIMUM_STORAGE_AVAILABLE = 0.25F; @@ -106,20 +104,20 @@ public class AttachmentService extends Service implements Runnable { 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 + // This callback is invoked by the various service implementations 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; + 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(); + Context mContext; + EmailConnectivityManager mConnectivityManager; + final AttachmentWatchdog mWatchdog = new AttachmentWatchdog(); private final Object mLock = new Object(); @@ -127,22 +125,18 @@ public class AttachmentService extends Service implements Runnable { // 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 loaded). If and when we reach the per-account // limit, we recalculate the actual usage - /*package*/ final ConcurrentHashMap mAttachmentStorageMap = - new ConcurrentHashMap(); + 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 ConcurrentHashMap mAttachmentFailureMap = - new ConcurrentHashMap(); + final ConcurrentHashMap mAttachmentFailureMap = new ConcurrentHashMap(); // Keeps tracks of downloads in progress based on an attachment ID to DownloadRequest mapping. - /*package*/ final ConcurrentHashMap mDownloadsInProgress = + final ConcurrentHashMap mDownloadsInProgress = new ConcurrentHashMap(); - // 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(); + final DownloadQueue mDownloadQueue = new DownloadQueue(); /** * This class is used to contain the details and state of a particular request to download @@ -150,8 +144,7 @@ public class AttachmentService extends Service implements Runnable { * or in the in-progress map used to keep track of downloads that are currently happening * in the system */ -// public static class DownloadRequest { - /*package*/ static class DownloadRequest { + static class DownloadRequest { // Details of the request. final int mPriority; final long mTime; @@ -227,29 +220,6 @@ 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. - */ - private static class DownloadComparator implements Comparator { - @Override - public int compare(DownloadRequest req1, DownloadRequest req2) { - int res; - if (req1.mPriority != req2.mPriority) { - res = (req1.mPriority < req2.mPriority) ? -1 : 1; - } else { - if (req1.mTime == req2.mTime) { - res = 0; - } else { - res = (req1.mTime < req2.mTime) ? -1 : 1; - } - } - return res; - } - } - /** * This class is used to organize the various download requests that are pending. * We need a class that allows us to prioritize a collection of {@link DownloadRequest} objects @@ -259,20 +229,40 @@ public class AttachmentService extends Service implements Runnable { * as we can avoid pesky ConcurrentModificationException when one thread has the iterator * and another thread modifies the collection. */ - /*package*/ static class DownloadQueue { + static class DownloadQueue { private final int DEFAULT_SIZE = 10; // For synchronization private final Object mLock = new Object(); + /** + * 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) + */ + private static class DownloadComparator implements Comparator { + @Override + public int compare(DownloadRequest req1, DownloadRequest req2) { + int res; + if (req1.mPriority != req2.mPriority) { + res = (req1.mPriority < req2.mPriority) ? -1 : 1; + } else { + if (req1.mTime == req2.mTime) { + res = 0; + } else { + res = (req1.mTime < req2.mTime) ? -1 : 1; + } + } + return res; + } + } + // For prioritization of DownloadRequests. - /*package*/ final PriorityQueue mRequestQueue = + final PriorityQueue mRequestQueue = 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. - /*package*/ final ConcurrentHashMap mRequestMap = - new ConcurrentHashMap(); + final ConcurrentHashMap mRequestMap = new ConcurrentHashMap(); /** * This function will add the request to our collections if it does not already @@ -280,7 +270,7 @@ 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 boolean addRequest(final DownloadRequest request) throws NullPointerException { // It is key to keep the map and queue in lock step if (request == null) { @@ -312,7 +302,7 @@ public class AttachmentService extends Service implements Runnable { * @return true if it was removed or the request was invalid (meaning that the request * is not in our queue), false otherwise. */ - public synchronized boolean removeRequest(final DownloadRequest request) { + public boolean removeRequest(final DownloadRequest request) { if (request == null) { // If it is invalid, its not in the queue. return true; @@ -332,7 +322,7 @@ public class AttachmentService extends Service implements Runnable { * Return the next request from our queue. * @return The next {@link DownloadRequest} object or null if the queue is empty */ - public synchronized DownloadRequest getNextRequest() { + public DownloadRequest getNextRequest() { // It is key to keep the map and queue in lock step final DownloadRequest returnRequest; synchronized (mLock) { @@ -354,15 +344,21 @@ public class AttachmentService extends Service implements Runnable { if (requestId < 0) { return null; } - return mRequestMap.get(requestId); + synchronized (mLock) { + return mRequestMap.get(requestId); + } } public int getSize() { - return mRequestMap.size(); + synchronized (mLock) { + return mRequestMap.size(); + } } public boolean isEmpty() { - return mRequestMap.isEmpty(); + synchronized (mLock) { + return mRequestMap.isEmpty(); + } } } @@ -420,7 +416,7 @@ public class AttachmentService extends Service implements Runnable { * 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) { + 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. @@ -433,6 +429,7 @@ public class AttachmentService extends Service implements Runnable { LogUtils.d(LOG_TAG, "== Download of " + req.mAttachmentId + " timed out"); } service.cancelDownload(req); + // TODO: Should we also mark the attachment as failed at this point in time? } } // Check whether we can start new downloads... @@ -448,486 +445,444 @@ public class AttachmentService extends Service implements Runnable { } } - /** - * 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() { + boolean isConnected() { if (mConnectivityManager != null) { return mConnectivityManager.hasConnectivity(); } return false; } - /*package*/ Collection getInProgressDownloads() { + Collection getInProgressDownloads() { return mDownloadsInProgress.values(); } - /*package*/ boolean areDownloadsInProgress() { + 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 - * priority key represents this ordering. Note: All methods that change the attachment map are - * synchronized on the map itself + * Set the bits in the provider to mark this download as failed. + * @param att The attachment that failed to download. */ - /*package*/ class DownloadSet extends TreeSet { - private static final long serialVersionUID = 1L; + void markAttachmentAsFailed(final Attachment att) { + final ContentValues cv = new ContentValues(); + final int flags = Attachment.FLAG_DOWNLOAD_FORWARD | Attachment.FLAG_DOWNLOAD_USER_REQUEST; + cv.put(AttachmentColumns.FLAGS, att.mFlags &= ~flags); + cv.put(AttachmentColumns.UI_STATE, AttachmentState.FAILED); + att.update(mContext, cv); + } - /*package*/ DownloadSet(Comparator comparator) { - super(comparator); - } - - private void markAttachmentAsFailed(final Attachment att) { - final ContentValues cv = new ContentValues(); - final int flags = Attachment.FLAG_DOWNLOAD_FORWARD | Attachment.FLAG_DOWNLOAD_USER_REQUEST; - cv.put(AttachmentColumns.FLAGS, att.mFlags &= ~flags); - cv.put(AttachmentColumns.UI_STATE, AttachmentState.FAILED); - att.update(mContext, cv); - } - - /** - * onChange is called by the AttachmentReceiver upon receipt of a valid notification from - * EmailProvider that an attachment has been inserted or modified. It's not strictly - * necessary that we detect a deleted attachment, as the code always checks for the - * existence of an attachment before acting on it. - */ - public synchronized void onChange(Context context, Attachment att) { - DownloadRequest req = findDownloadRequest(att.mId); - long priority = getPriority(att); - if (priority == PRIORITY_NONE) { - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "== Attachment changed: " + att.mId); - } - // In this case, there is no download priority for this attachment - if (req != null) { - // If it exists in the map, remove it - // NOTE: We don't yet support deleting downloads in progress - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "== Attachment " + att.mId + " was in queue, removing"); - } - remove(req); - } - } else { - // Ignore changes that occur during download - if (mDownloadsInProgress.containsKey(att.mId)) return; - // If this is new, add the request to the queue - if (req == null) { - req = new DownloadRequest(context, att); - final AttachmentInfo attachInfo = new AttachmentInfo(context, att); - if (!attachInfo.isEligibleForDownload()) { - // We can't download this file due to policy, depending on what type - // of request we received, we handle the response differently. - if (((att.mFlags & Attachment.FLAG_DOWNLOAD_USER_REQUEST) != 0) || - ((att.mFlags & Attachment.FLAG_POLICY_DISALLOWS_DOWNLOAD) != 0)) { - // There are a couple of situations where we will not even allow this - // request to go in the queue because we can already process it as a - // failure. - // 1. The user explictly wants to download this attachment from the - // email view but they should not be able to...either because there is - // no app to view it or because its been marked as a policy violation. - // 2. The user is forwarding an email and the attachment has been - // marked as a policy violation. If the attachment is non viewable - // that is OK for forwarding a message so we'll let it pass through - markAttachmentAsFailed(att); - return; - } - // If we get this far it a forward of an attachment that is only - // ineligible because we can't view it or process it. Not because we - // can't download it for policy reasons. Let's let this go through because - // the final recipient of this forward email might be able to process it. - } - add(req); - } - // If the request already existed, we'll update the priority (so that the time is - // up-to-date); otherwise, we create a new request - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "== Download queued for attachment " + att.mId + ", class " + - req.mPriority + ", priority time " + req.mTime); - } - } - // Process the queue if we're in a wait - kick(); - } - - /** - * Find a queued DownloadRequest, given the attachment's id - * @param id the id of the attachment - * @return the DownloadRequest for that attachment (or null, if none) - */ - /*package*/ synchronized DownloadRequest findDownloadRequest(long id) { - Iterator iterator = iterator(); - while(iterator.hasNext()) { - DownloadRequest req = iterator.next(); - if (req.mAttachmentId == id) { - return req; - } - } - return null; - } - - @Override - public synchronized boolean isEmpty() { - return super.isEmpty() && mDownloadsInProgress.isEmpty(); - } - - /** - * Run through the AttachmentMap and find DownloadRequests that can be executed, enforcing - * the limit on maximum downloads - */ - /*package*/ synchronized void processQueue() { + /** + * onChange is called by the AttachmentReceiver upon receipt of a valid notification from + * EmailProvider that an attachment has been inserted or modified. It's not strictly + * necessary that we detect a deleted attachment, as the code always checks for the + * existence of an attachment before acting on it. + */ + public synchronized void onChange(final Context context, final Attachment att) { + DownloadRequest req = mDownloadQueue.findRequestById(att.mId); + final long priority = getPriority(att); + if (priority == PRIORITY_NONE) { if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "== Checking attachment queue, " + mDownloadSet.size() - + " entries"); + LogUtils.d(LOG_TAG, "== Attachment changed: " + att.mId); } - Iterator iterator = mDownloadSet.descendingIterator(); - // First, start up any required downloads, in priority order - while (iterator.hasNext() && - (mDownloadsInProgress.size() < MAX_SIMULTANEOUS_DOWNLOADS)) { - DownloadRequest req = iterator.next(); - // Enforce per-account limit here - if (downloadsForAccount(req.mAccountId) >= MAX_SIMULTANEOUS_DOWNLOADS_PER_ACCOUNT) { - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "== Skip #" + req.mAttachmentId + "; maxed for acct #" + - req.mAccountId); - } - continue; - } else if (Attachment.restoreAttachmentWithId(mContext, req.mAttachmentId) == null) { - continue; - } - if (!req.mInProgress) { - final long currentTime = SystemClock.elapsedRealtime(); - if (req.mRetryCount > 0 && req.mRetryStartTime > currentTime) { - LogUtils.d(LOG_TAG, "== waiting to retry attachment %d", req.mAttachmentId); - mWatchdog.setWatchdogAlarm(mContext, CONNECTION_ERROR_RETRY_MILLIS, - CALLBACK_TIMEOUT); - continue; - } - // TODO: We try to gate ineligible downloads from entering the queue but its - // always possible that they made it in here regardless in the future. In a - // perfect world, we would make it bullet proof with a check for eligibility - // here instead/also. - mDownloadSet.tryStartDownload(req); - } - } - - // Don't prefetch if background downloading is disallowed - EmailConnectivityManager ecm = mConnectivityManager; - if (ecm == null) return; - if (!ecm.isAutoSyncAllowed()) return; - // Don't prefetch unless we're on a WiFi network - if (ecm.getActiveNetworkType() != ConnectivityManager.TYPE_WIFI) { - return; - } - // Then, try opportunistic download of appropriate attachments - int backgroundDownloads = MAX_SIMULTANEOUS_DOWNLOADS - mDownloadsInProgress.size(); - // Always leave one slot for user requested download - if (backgroundDownloads > (MAX_SIMULTANEOUS_DOWNLOADS - 1)) { - // We'll load up the newest 25 attachments that aren't loaded or queued - Uri lookupUri = EmailContent.uriWithLimit(Attachment.CONTENT_URI, - MAX_ATTACHMENTS_TO_CHECK); - Cursor c = mContext.getContentResolver().query(lookupUri, - Attachment.CONTENT_PROJECTION, - EmailContent.Attachment.PRECACHE_INBOX_SELECTION, - null, AttachmentColumns._ID + " DESC"); - File cacheDir = mContext.getCacheDir(); - try { - while (c.moveToNext()) { - Attachment att = new Attachment(); - att.restore(c); - Account account = Account.restoreAccountWithId(mContext, att.mAccountKey); - if (account == null) { - // Clean up this orphaned attachment; there's no point in keeping it - // around; then try to find another one - EmailContent.delete(mContext, Attachment.CONTENT_URI, att.mId); - } else { - // Check that the attachment meets system requirements for download - AttachmentInfo info = new AttachmentInfo(mContext, att); - if (info.isEligibleForDownload()) { - // Either the account must be able to prefetch or this must be - // an inline attachment - if (att.mContentId != null || - (canPrefetchForAccount(account, cacheDir))) { - 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); - break; - } - } else { - // If this attachment was ineligible for download - // because of policy related issues, its flags would be set to - // FLAG_POLICY_DISALLOWS_DOWNLOAD and would not show up in the - // query results. We are most likely here for other reasons such - // as the inability to view the attachment. In that case, let's just - // skip it for now. - LogUtils.e(LOG_TAG, "== skip attachment %d, it is ineligible", att.mId); - } - } - } - } finally { - c.close(); - } - } - } - - /** - * Count the number of running downloads in progress for this account - * @param accountId the id of the account - * @return the count of running downloads - */ - /*package*/ synchronized int downloadsForAccount(long accountId) { - int count = 0; - for (DownloadRequest req: mDownloadsInProgress.values()) { - if (req.mAccountId == accountId) { - count++; - } - } - return count; - } - - /** - * Attempt to execute the DownloadRequest, enforcing the maximum downloads per account - * parameter - * @param req the DownloadRequest - * @return whether or not the download was started - */ - /*package*/ synchronized boolean tryStartDownload(DownloadRequest req) { - EmailServiceProxy service = EmailServiceUtils.getServiceForAccount( - AttachmentService.this, req.mAccountId); - - // Do not download the same attachment multiple times - boolean alreadyInProgress = mDownloadsInProgress.get(req.mAttachmentId) != null; - if (alreadyInProgress) return false; - - try { - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, ">> Starting download for attachment #" + req.mAttachmentId); - } - startDownload(service, req); - } catch (RemoteException e) { - // TODO: Consider whether we need to do more in this case... - // For now, fix up our data to reflect the failure - cancelDownload(req); - } - return true; - } - - private synchronized DownloadRequest getDownloadInProgress(long attachmentId) { - return mDownloadsInProgress.get(attachmentId); - } - - - /** - * Do the work of starting an attachment download using the EmailService interface, and - * set our watchdog alarm - * - * @param service the service handling the download - * @param req the DownloadRequest - * @throws RemoteException - */ - private void startDownload(EmailServiceProxy service, DownloadRequest req) - throws RemoteException { - req.mStartTime = System.currentTimeMillis(); - req.mInProgress = true; - mDownloadsInProgress.put(req.mAttachmentId, req); - service.loadAttachment(mServiceCallback, req.mAccountId, req.mAttachmentId, - req.mPriority != PRIORITY_FOREGROUND); - mWatchdog.setWatchdogAlarm(mContext); - } - - /*package*/ synchronized void cancelDownload(DownloadRequest req) { - LogUtils.d(LOG_TAG, "cancelDownload #%d", req.mAttachmentId); - req.mInProgress = false; - mDownloadsInProgress.remove(req.mAttachmentId); - // Remove the download from our queue, and then decide whether or not to add it back. - remove(req); - req.mRetryCount++; - if (req.mRetryCount > CONNECTION_ERROR_MAX_RETRIES) { - LogUtils.d(LOG_TAG, "too many failures, giving up"); - } else { - LogUtils.d(LOG_TAG, "moving to end of queue, will retry"); - // The time field of DownloadRequest is final, because it's unsafe to change it - // as long as the DownloadRequest is in the DownloadSet. It's needed for the - // comparator, so changing time would make the request unfindable. - // Instead, we'll create a new DownloadRequest with an updated time. - // This will sort at the end of the set. - req = new DownloadRequest(req, SystemClock.elapsedRealtime()); - add(req); - } - } - - /** - * Called when a download is finished; we get notified of this via our EmailServiceCallback - * @param attachmentId the id of the attachment whose download is finished - * @param statusCode the EmailServiceStatus code returned by the Service - */ - /*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 - if (req != null) { - req.mRetryCount++; - if (req.mRetryCount > CONNECTION_ERROR_MAX_RETRIES) { - LogUtils.d(LOG_TAG, "Connection Error #%d, giving up", attachmentId); - remove(req); - } else if (req.mRetryCount > CONNECTION_ERROR_DELAY_THRESHOLD) { - // TODO: I'm not sure this is a great retry/backoff policy, but we're - // afraid of changing behavior too much in case something relies upon it. - // So now, for the first five errors, we'll retry immediately. For the next - // five tries, we'll add a ten second delay between each. After that, we'll - // give up. - LogUtils.d(LOG_TAG, "ConnectionError #%d, retried %d times, adding delay", - attachmentId, req.mRetryCount); - req.mInProgress = false; - req.mRetryStartTime = SystemClock.elapsedRealtime() + - 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); - req.mInProgress = false; - req.mRetryStartTime = 0; - kick(); - } - } - return; - } - - // If the request is still in the queue, remove it + // In this case, there is no download priority for this attachment if (req != null) { - remove(req); + // If it exists in the map, remove it + // NOTE: We don't yet support deleting downloads in progress + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + LogUtils.d(LOG_TAG, "== Attachment " + att.mId + " was in queue, removing"); + } + mDownloadQueue.removeRequest(req); } - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - long secs = 0; - if (req != null) { - secs = (System.currentTimeMillis() - req.mTime) / 1000; - } - String status = (statusCode == EmailServiceStatus.SUCCESS) ? "Success" : - "Error " + statusCode; - LogUtils.d(LOG_TAG, "<< Download finished for attachment #" + attachmentId + "; " + secs - + " seconds from request, status: " + status); - } - - Attachment attachment = Attachment.restoreAttachmentWithId(mContext, attachmentId); - if (attachment != null) { - long accountId = attachment.mAccountKey; - // Update our attachment storage for this account - Long currentStorage = mAttachmentStorageMap.get(accountId); - if (currentStorage == null) { - currentStorage = 0L; - } - mAttachmentStorageMap.put(accountId, currentStorage + attachment.mSize); - boolean deleted = false; - if ((attachment.mFlags & Attachment.FLAG_DOWNLOAD_FORWARD) != 0) { - if (statusCode == EmailServiceStatus.ATTACHMENT_NOT_FOUND) { - // If this is a forwarding download, and the attachment doesn't exist (or - // can't be downloaded) delete it from the outgoing message, lest that - // message never get sent - EmailContent.delete(mContext, Attachment.CONTENT_URI, attachment.mId); - // TODO: Talk to UX about whether this is even worth doing - NotificationController nc = NotificationController.getInstance(mContext); - nc.showDownloadForwardFailedNotification(attachment); - deleted = true; - } - // If we're an attachment on forwarded mail, and if we're not still blocked, - // try to send pending mail now (as mediated by MailService) - if ((req != null) && - !Utility.hasUnloadedAttachments(mContext, attachment.mMessageKey)) { - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "== Downloads finished for outgoing msg #" - + req.mMessageId); - } - EmailServiceProxy service = EmailServiceUtils.getServiceForAccount( - mContext, accountId); - try { - service.sendMail(accountId); - } catch (RemoteException e) { - // We tried - } - } - } - if (statusCode == EmailServiceStatus.MESSAGE_NOT_FOUND) { - Message msg = Message.restoreMessageWithId(mContext, attachment.mMessageKey); - if (msg == null) { - // If there's no associated message, delete the attachment - EmailContent.delete(mContext, Attachment.CONTENT_URI, attachment.mId); - } else { - // If there really is a message, retry - // TODO: How will this get retried? It's still marked as inProgress? - kick(); + } else { + // Ignore changes that occur during download + if (mDownloadsInProgress.containsKey(att.mId)) return; + // If this is new, add the request to the queue + if (req == null) { + req = new DownloadRequest(context, att); + final AttachmentInfo attachInfo = new AttachmentInfo(context, att); + if (!attachInfo.isEligibleForDownload()) { + // We can't download this file due to policy, depending on what type + // of request we received, we handle the response differently. + if (((att.mFlags & Attachment.FLAG_DOWNLOAD_USER_REQUEST) != 0) || + ((att.mFlags & Attachment.FLAG_POLICY_DISALLOWS_DOWNLOAD) != 0)) { + // There are a couple of situations where we will not even allow this + // request to go in the queue because we can already process it as a + // failure. + // 1. The user explictly wants to download this attachment from the + // email view but they should not be able to...either because there is + // no app to view it or because its been marked as a policy violation. + // 2. The user is forwarding an email and the attachment has been + // marked as a policy violation. If the attachment is non viewable + // that is OK for forwarding a message so we'll let it pass through + markAttachmentAsFailed(att); return; } - } else if (!deleted) { - // Clear the download flags, since we're done for now. Note that this happens - // only for non-recoverable errors. When these occur for forwarded mail, we can - // ignore it and continue; otherwise, it was either 1) a user request, in which - // case the user can retry manually or 2) an opportunistic download, in which - // case the download wasn't critical - ContentValues cv = new ContentValues(); - int flags = - Attachment.FLAG_DOWNLOAD_FORWARD | Attachment.FLAG_DOWNLOAD_USER_REQUEST; - cv.put(AttachmentColumns.FLAGS, attachment.mFlags &= ~flags); - cv.put(AttachmentColumns.UI_STATE, AttachmentState.SAVED); - attachment.update(mContext, cv); + // If we get this far it a forward of an attachment that is only + // ineligible because we can't view it or process it. Not because we + // can't download it for policy reasons. Let's let this go through because + // the final recipient of this forward email might be able to process it. + } + mDownloadQueue.addRequest(req); + } + // If the request already existed, we'll update the priority (so that the time is + // up-to-date); otherwise, we create a new request + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + LogUtils.d(LOG_TAG, "== Download queued for attachment " + att.mId + ", class " + + req.mPriority + ", priority time " + req.mTime); + } + } + // Process the queue if we're in a wait + kick(); + } + + /** + * Run through the AttachmentMap and find DownloadRequests that can be executed, enforcing + * the limit on maximum downloads + */ + synchronized void processQueue() { + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + LogUtils.d(LOG_TAG, "== Checking attachment queue, " + mDownloadQueue.getSize() + + " entries"); + } + + while (mDownloadsInProgress.size() < MAX_SIMULTANEOUS_DOWNLOADS) { + final DownloadRequest req = mDownloadQueue.getNextRequest(); + if (req == null) { + // No more queued requests? We are done for now. + break; + } + // Enforce per-account limit here + if (downloadsForAccount(req.mAccountId) >= MAX_SIMULTANEOUS_DOWNLOADS_PER_ACCOUNT) { + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + LogUtils.d(LOG_TAG, "== Skip #" + req.mAttachmentId + "; maxed for acct #" + + req.mAccountId); + } + continue; + } else if (Attachment.restoreAttachmentWithId(mContext, req.mAttachmentId) == null) { + continue; + } + if (!req.mInProgress) { + final long currentTime = SystemClock.elapsedRealtime(); + if (req.mRetryCount > 0 && req.mRetryStartTime > currentTime) { + LogUtils.d(LOG_TAG, "== waiting to retry attachment %d", req.mAttachmentId); + mWatchdog.setWatchdogAlarm(mContext, CONNECTION_ERROR_RETRY_MILLIS, + CALLBACK_TIMEOUT); + continue; + } + // TODO: We try to gate ineligible downloads from entering the queue but its + // always possible that they made it in here regardless in the future. In a + // perfect world, we would make it bullet proof with a check for eligibility + // here instead/also. + tryStartDownload(req); + } + } + + // Check our ability to be opportunistic regarding background downloads. + final EmailConnectivityManager ecm = mConnectivityManager; + if ((ecm == null) || !ecm.isAutoSyncAllowed() || + (ecm.getActiveNetworkType() != ConnectivityManager.TYPE_WIFI)) { + // Only prefetch if it if connectivity is available, prefetch is enabled + // and we are on WIFI + return; + } + + // Then, try opportunistic download of appropriate attachments + final int backgroundDownloads = MAX_SIMULTANEOUS_DOWNLOADS - mDownloadsInProgress.size(); + if ((backgroundDownloads + 1) >= MAX_SIMULTANEOUS_DOWNLOADS) { + // We want to leave one spot open for a user requested download that we haven't + // started processing yet. + return; + } + + // We'll load up the newest 25 attachments that aren't loaded or queued + // TODO: We are always looking for MAX_ATTACHMENTS_TO_CHECK, shouldn't this be + // backgroundDownloads instead? We should fix and test this. + final Uri lookupUri = EmailContent.uriWithLimit(Attachment.CONTENT_URI, + MAX_ATTACHMENTS_TO_CHECK); + final Cursor c = mContext.getContentResolver().query(lookupUri, + Attachment.CONTENT_PROJECTION, + EmailContent.Attachment.PRECACHE_INBOX_SELECTION, + null, AttachmentColumns._ID + " DESC"); + File cacheDir = mContext.getCacheDir(); + try { + while (c.moveToNext()) { + final Attachment att = new Attachment(); + att.restore(c); + final Account account = Account.restoreAccountWithId(mContext, att.mAccountKey); + if (account == null) { + // Clean up this orphaned attachment; there's no point in keeping it + // around; then try to find another one + EmailContent.delete(mContext, Attachment.CONTENT_URI, att.mId); + } else { + // Check that the attachment meets system requirements for download + // Note that there couple be policy that does not allow this attachment + // to be downloaded. + final AttachmentInfo info = new AttachmentInfo(mContext, att); + if (info.isEligibleForDownload()) { + // Either the account must be able to prefetch or this must be + // an inline attachment. + if (att.mContentId != null || + (canPrefetchForAccount(account, cacheDir))) { + final Integer 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 + final DownloadRequest req = new DownloadRequest(mContext, att); + tryStartDownload(req); + break; + } + } else { + // If this attachment was ineligible for download + // because of policy related issues, its flags would be set to + // FLAG_POLICY_DISALLOWS_DOWNLOAD and would not show up in the + // query results. We are most likely here for other reasons such + // as the inability to view the attachment. In that case, let's just + // skip it for now. + LogUtils.e(LOG_TAG, "== skip attachment %d, it is ineligible", att.mId); + } } } - // Process the queue - kick(); + } finally { + c.close(); } } + /** + * Count the number of running downloads in progress for this account + * @param accountId the id of the account + * @return the count of running downloads + */ + synchronized int downloadsForAccount(final long accountId) { + int count = 0; + for (final DownloadRequest req: mDownloadsInProgress.values()) { + if (req.mAccountId == accountId) { + count++; + } + } + return count; + } + + /** + * Attempt to execute the DownloadRequest, enforcing the maximum downloads per account + * parameter + * @param req the DownloadRequest + * @return whether or not the download was started + */ + synchronized boolean tryStartDownload(final DownloadRequest req) { + final EmailServiceProxy service = EmailServiceUtils.getServiceForAccount( + AttachmentService.this, req.mAccountId); + + // Do not download the same attachment multiple times + boolean alreadyInProgress = mDownloadsInProgress.get(req.mAttachmentId) != null; + if (alreadyInProgress) return false; + + try { + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + LogUtils.d(LOG_TAG, ">> Starting download for attachment #" + req.mAttachmentId); + } + startDownload(service, req); + } catch (RemoteException e) { + // TODO: Consider whether we need to do more in this case... + // For now, fix up our data to reflect the failure + cancelDownload(req); + } + return true; + } + + /** + * Do the work of starting an attachment download using the EmailService interface, and + * set our watchdog alarm + * + * @param service the service handling the download + * @param req the DownloadRequest + * @throws RemoteException + */ + private void startDownload(final EmailServiceProxy service, final DownloadRequest req) + throws RemoteException { + req.mStartTime = System.currentTimeMillis(); + req.mInProgress = true; + mDownloadsInProgress.put(req.mAttachmentId, req); + service.loadAttachment(mServiceCallback, req.mAccountId, req.mAttachmentId, + req.mPriority != PRIORITY_FOREGROUND); + mWatchdog.setWatchdogAlarm(mContext); + } + + synchronized void cancelDownload(final DownloadRequest req) { + LogUtils.d(LOG_TAG, "cancelDownload #%d", req.mAttachmentId); + req.mInProgress = false; + mDownloadsInProgress.remove(req.mAttachmentId); + // Remove the download from our queue, and then decide whether or not to add it back. + mDownloadQueue.removeRequest(req); + req.mRetryCount++; + if (req.mRetryCount > CONNECTION_ERROR_MAX_RETRIES) { + LogUtils.d(LOG_TAG, "too many failures, giving up"); + } else { + LogUtils.d(LOG_TAG, "moving to end of queue, will retry"); + // The time field of DownloadRequest is final, because it's unsafe to change it + // as long as the DownloadRequest is in the DownloadSet. It's needed for the + // comparator, so changing time would make the request unfindable. + // Instead, we'll create a new DownloadRequest with an updated time. + // This will sort at the end of the set. + final DownloadRequest newReq = new DownloadRequest(req, SystemClock.elapsedRealtime()); + mDownloadQueue.addRequest(newReq); + } + } + + /** + * Called when a download is finished; we get notified of this via our EmailServiceCallback + * @param attachmentId the id of the attachment whose download is finished + * @param statusCode the EmailServiceStatus code returned by the Service + */ + synchronized void endDownload(final long attachmentId, final 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); + } + + final DownloadRequest req = mDownloadQueue.findRequestById(attachmentId); + if (statusCode == EmailServiceStatus.CONNECTION_ERROR) { + // If this needs to be retried, just process the queue again + if (req != null) { + req.mRetryCount++; + if (req.mRetryCount > CONNECTION_ERROR_MAX_RETRIES) { + // We are done, we maxed out our total number of tries. + LogUtils.d(LOG_TAG, "Connection Error #%d, giving up", attachmentId); + mDownloadQueue.removeRequest(req); + // Note that we are not doing anything with the attachment right now + // We will annotate it later in this function if needed. + } else if (req.mRetryCount > CONNECTION_ERROR_DELAY_THRESHOLD) { + // TODO: I'm not sure this is a great retry/backoff policy, but we're + // afraid of changing behavior too much in case something relies upon it. + // So now, for the first five errors, we'll retry immediately. For the next + // five tries, we'll add a ten second delay between each. After that, we'll + // give up. + LogUtils.d(LOG_TAG, "ConnectionError #%d, retried %d times, adding delay", + attachmentId, req.mRetryCount); + req.mInProgress = false; + req.mRetryStartTime = SystemClock.elapsedRealtime() + + 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); + req.mInProgress = false; + req.mRetryStartTime = 0; + kick(); + } + } + return; + } + + // If the request is still in the queue, remove it + if (req != null) { + mDownloadQueue.removeRequest(req); + } + + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + long secs = 0; + if (req != null) { + secs = (System.currentTimeMillis() - req.mTime) / 1000; + } + final String status = (statusCode == EmailServiceStatus.SUCCESS) ? "Success" : + "Error " + statusCode; + LogUtils.d(LOG_TAG, "<< Download finished for attachment #" + attachmentId + "; " + secs + + " seconds from request, status: " + status); + } + + final Attachment attachment = Attachment.restoreAttachmentWithId(mContext, attachmentId); + if (attachment != null) { + final long accountId = attachment.mAccountKey; + // Update our attachment storage for this account + Long currentStorage = mAttachmentStorageMap.get(accountId); + if (currentStorage == null) { + currentStorage = 0L; + } + mAttachmentStorageMap.put(accountId, currentStorage + attachment.mSize); + boolean deleted = false; + if ((attachment.mFlags & Attachment.FLAG_DOWNLOAD_FORWARD) != 0) { + if (statusCode == EmailServiceStatus.ATTACHMENT_NOT_FOUND) { + // If this is a forwarding download, and the attachment doesn't exist (or + // can't be downloaded) delete it from the outgoing message, lest that + // message never get sent + EmailContent.delete(mContext, Attachment.CONTENT_URI, attachment.mId); + // TODO: Talk to UX about whether this is even worth doing + NotificationController nc = NotificationController.getInstance(mContext); + nc.showDownloadForwardFailedNotification(attachment); + deleted = true; + } + // If we're an attachment on forwarded mail, and if we're not still blocked, + // try to send pending mail now (as mediated by MailService) + if ((req != null) && + !Utility.hasUnloadedAttachments(mContext, attachment.mMessageKey)) { + if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { + LogUtils.d(LOG_TAG, "== Downloads finished for outgoing msg #" + + req.mMessageId); + } + EmailServiceProxy service = EmailServiceUtils.getServiceForAccount( + mContext, accountId); + try { + service.sendMail(accountId); + } catch (RemoteException e) { + // We tried + } + } + } + if (statusCode == EmailServiceStatus.MESSAGE_NOT_FOUND) { + Message msg = Message.restoreMessageWithId(mContext, attachment.mMessageKey); + if (msg == null) { + // If there's no associated message, delete the attachment + EmailContent.delete(mContext, Attachment.CONTENT_URI, attachment.mId); + } else { + // If there really is a message, retry + // TODO: How will this get retried? It's still marked as inProgress? + kick(); + return; + } + } else if (!deleted) { + // Clear the download flags, since we're done for now. Note that this happens + // only for non-recoverable errors. When these occur for forwarded mail, we can + // ignore it and continue; otherwise, it was either 1) a user request, in which + // case the user can retry manually or 2) an opportunistic download, in which + // case the download wasn't critical + ContentValues cv = new ContentValues(); + int flags = + Attachment.FLAG_DOWNLOAD_FORWARD | Attachment.FLAG_DOWNLOAD_USER_REQUEST; + cv.put(AttachmentColumns.FLAGS, attachment.mFlags &= ~flags); + cv.put(AttachmentColumns.UI_STATE, AttachmentState.SAVED); + attachment.update(mContext, cv); + } + } + // Process the queue + kick(); + } + /** * Calculate the download priority of an Attachment. A priority of zero means that the * attachment is not marked for download. * @param att the Attachment * @return the priority key of the Attachment */ - private static int getPriority(Attachment att) { + private static int getPriority(final Attachment att) { int priorityClass = PRIORITY_NONE; - int flags = att.mFlags; + final int flags = att.mFlags; if ((flags & Attachment.FLAG_DOWNLOAD_FORWARD) != 0) { priorityClass = PRIORITY_SEND_MAIL; } else if ((flags & Attachment.FLAG_DOWNLOAD_USER_REQUEST) != 0) { @@ -947,15 +902,15 @@ public class AttachmentService extends Service implements Runnable { * come from either Controller (IMAP) or ExchangeService (EAS). Note that we only implement the * single callback that's defined by the EmailServiceCallback interface. */ - private class ServiceCallback extends IEmailServiceCallback.Stub { + class ServiceCallback extends IEmailServiceCallback.Stub { @Override - public void loadAttachmentStatus(long messageId, long attachmentId, int statusCode, - int progress) { + public void loadAttachmentStatus(final long messageId, final long attachmentId, + final int statusCode, final int progress) { // Record status and progress - DownloadRequest req = mDownloadSet.getDownloadInProgress(attachmentId); + final DownloadRequest req = mDownloadsInProgress.get(attachmentId); if (req != null) { if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - String code; + final String code; switch(statusCode) { case EmailServiceStatus.SUCCESS: code = "Success"; break; case EmailServiceStatus.IN_PROGRESS: code = "In progress"; break; @@ -970,88 +925,32 @@ public class AttachmentService extends Service implements Runnable { req.mLastStatusCode = statusCode; req.mLastProgress = progress; req.mLastCallbackTime = System.currentTimeMillis(); - Attachment attachment = Attachment.restoreAttachmentWithId(mContext, attachmentId); + final Attachment attachment = + Attachment.restoreAttachmentWithId(mContext, attachmentId); if (attachment != null && statusCode == EmailServiceStatus.IN_PROGRESS) { - ContentValues values = new ContentValues(); + final ContentValues values = new ContentValues(); values.put(AttachmentColumns.UI_DOWNLOADED_SIZE, attachment.mSize * progress / 100); // Update UIProvider with updated download size // Individual services will set contentUri and state when finished attachment.update(mContext, values); } - } - switch (statusCode) { - case EmailServiceStatus.IN_PROGRESS: - break; - default: - mDownloadSet.endDownload(attachmentId, statusCode); - break; + switch (statusCode) { + case EmailServiceStatus.IN_PROGRESS: + break; + default: + endDownload(attachmentId, statusCode); + break; + } + } else { + // The only way that we can get a callback from the service implementation for + // an attachment that doesn't exist is if it was cancelled due to the + // AttachmentWatchdog. This is a valid scenario and the Watchdog should have already + // marked this attachment as failed/cancelled. } } } - /*package*/ void onChange(Attachment att) { - mDownloadSet.onChange(this, att); - } - - /*package*/ boolean isQueued(long attachmentId) { - return mDownloadSet.findDownloadRequest(attachmentId) != null; - } - - /*package*/ int getSize() { - return mDownloadSet.size(); - } - - /*package*/ boolean dequeue(long attachmentId) { - DownloadRequest req = mDownloadSet.findDownloadRequest(attachmentId); - if (req != null) { - if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, "Dequeued attachmentId: " + attachmentId); - } - mDownloadSet.remove(req); - return true; - } - return false; - } - - /** - * Ask the service for the number of items in the download queue - * @return the number of items queued for download - */ - public static int getQueueSize() { - AttachmentService service = sRunningService; - if (service != null) { - return service.getSize(); - } - return 0; - } - - /** - * Ask the service whether a particular attachment is queued for download - * @param attachmentId the id of the Attachment (as stored by EmailProvider) - * @return whether or not the attachment is queued for download - */ - public static boolean isAttachmentQueued(long attachmentId) { - AttachmentService service = sRunningService; - if (service != null) { - return service.isQueued(attachmentId); - } - return false; - } - - /** - * Ask the service to remove an attachment from the download queue - * @param attachmentId the id of the Attachment (as stored by EmailProvider) - * @return whether or not the attachment was removed from the queue - */ - public static boolean cancelQueuedAttachment(long attachmentId) { - AttachmentService service = sRunningService; - if (service != null) { - return service.dequeue(attachmentId); - } - return false; - } - // The queue entries here are entries of the form {id, flags}, with the values passed in to // attachmentChanged() private static final Queue sAttachmentChangedQueue = @@ -1059,7 +958,9 @@ public class AttachmentService extends Service implements Runnable { private static AsyncTask sAttachmentChangedTask; /** - * Called directly by EmailProvider whenever an attachment is inserted or changed + * Called directly by EmailProvider whenever an attachment is inserted or changed. Since this + * call is being invoked on the UI thread, we need to make sure that the downloads are + * happening in the background. * @param context the caller's context * @param id the attachment's id * @param flags the new flags for the attachment @@ -1067,7 +968,6 @@ public class AttachmentService extends Service implements Runnable { public static void attachmentChanged(final Context context, final long id, final int flags) { synchronized (sAttachmentChangedQueue) { sAttachmentChangedQueue.add(new long[]{id, flags}); - if (sAttachmentChangedTask == null) { sAttachmentChangedTask = new AsyncTask() { @Override @@ -1092,6 +992,8 @@ public class AttachmentService extends Service implements Runnable { final Intent intent = new Intent(context, AttachmentService.class); intent.putExtra(EXTRA_ATTACHMENT, attachment); + // This is result in a call to AttachmentService.onStartCommand() + // which will queue the attachment in its internal prioritized queue. context.startService(intent); } } @@ -1101,31 +1003,34 @@ public class AttachmentService extends Service implements Runnable { } /** - * Determine whether an attachment can be prefetched for the given account + * Determine whether an attachment can be prefetched for the given account based on + * total download size restrictions tied to the account. * @return true if download is allowed, false otherwise */ - public boolean canPrefetchForAccount(Account account, File dir) { + public boolean canPrefetchForAccount(final Account account, final File dir) { // Check account, just in case if (account == null) return false; + // First, check preference and quickly return if prefetch isn't allowed if ((account.mFlags & Account.FLAGS_BACKGROUND_ATTACHMENTS) == 0) return false; - long totalStorage = dir.getTotalSpace(); - long usableStorage = dir.getUsableSpace(); - long minAvailable = (long)(totalStorage * PREFETCH_MINIMUM_STORAGE_AVAILABLE); + final long totalStorage = dir.getTotalSpace(); + final long usableStorage = dir.getUsableSpace(); + final long minAvailable = (long)(totalStorage * PREFETCH_MINIMUM_STORAGE_AVAILABLE); // If there's not enough overall storage available, stop now - if (usableStorage < minAvailable) { - return false; - } + if (usableStorage < minAvailable) return false; - int numberOfAccounts = mAccountManagerStub.getNumberOfAccounts(); - long perAccountMaxStorage = + final int numberOfAccounts = mAccountManagerStub.getNumberOfAccounts(); + // Calculate an even per-account storage although it would make a lot of sense to not + // do this as you may assign more storage to your corporate account rather than a personal + // account. + final long perAccountMaxStorage = (long)(totalStorage * PREFETCH_MAXIMUM_ATTACHMENT_STORAGE / numberOfAccounts); // Retrieve our idea of currently used attachment storage; since we don't track deletions, // this number is the "worst case". If the number is greater than what's allowed per - // account, we walk the directory to determine the actual number + // account, we walk the directory to determine the actual number. Long accountStorage = mAttachmentStorageMap.get(account.mId); if (accountStorage == null || (accountStorage > perAccountMaxStorage)) { // Calculate the exact figure for attachment storage for this account @@ -1136,22 +1041,24 @@ public class AttachmentService extends Service implements Runnable { accountStorage += file.length(); } } - // Cache the value + // Cache the value. No locking here since this is a concurrent collection object. mAttachmentStorageMap.put(account.mId, accountStorage); } // Return true if we're using less than the maximum per account - if (accountStorage < perAccountMaxStorage) { - return true; - } else { + if (accountStorage >= perAccountMaxStorage) { if (LogUtils.isLoggable(LOG_TAG, LogUtils.DEBUG)) { - LogUtils.d(LOG_TAG, ">> Prefetch not allowed for account " + account.mId + "; used " + - accountStorage + ", limit " + perAccountMaxStorage); + LogUtils.d(LOG_TAG, ">> Prefetch not allowed for account " + account.mId + + "; used " + accountStorage + ", limit " + perAccountMaxStorage); } return false; } + return true; } + /** + * The main routine for our AttachmentService service thread. + */ @Override public void run() { // These fields are only used within the service thread @@ -1160,24 +1067,24 @@ public class AttachmentService extends Service implements Runnable { mAccountManagerStub = new AccountManagerStub(this); // Run through all attachments in the database that require download and add them to - // the queue - int mask = Attachment.FLAG_DOWNLOAD_FORWARD | Attachment.FLAG_DOWNLOAD_USER_REQUEST; - Cursor c = getContentResolver().query(Attachment.CONTENT_URI, + // the queue. This is the case where a previous AttachmentService may have been notified + // to stop before processing everything in its queue. + final int mask = Attachment.FLAG_DOWNLOAD_FORWARD | Attachment.FLAG_DOWNLOAD_USER_REQUEST; + final Cursor c = getContentResolver().query(Attachment.CONTENT_URI, EmailContent.ID_PROJECTION, "(" + AttachmentColumns.FLAGS + " & ?) != 0", new String[] {Integer.toString(mask)}, null); try { LogUtils.d(LOG_TAG, "Count: " + c.getCount()); while (c.moveToNext()) { - Attachment attachment = Attachment.restoreAttachmentWithId( + final Attachment attachment = Attachment.restoreAttachmentWithId( this, c.getLong(EmailContent.ID_PROJECTION_COLUMN)); if (attachment != null) { - mDownloadSet.onChange(this, attachment); + onChange(this, attachment); } } } catch (Exception e) { e.printStackTrace(); - } - finally { + } finally { c.close(); } @@ -1191,10 +1098,11 @@ public class AttachmentService extends Service implements Runnable { } if (mStop) { // We might be bailing out here due to the service shutting down + LogUtils.d(LOG_TAG, "*** AttachmentService has been instructed to stop"); break; } - mDownloadSet.processQueue(); - if (mDownloadSet.isEmpty()) { + processQueue(); + if (mDownloadQueue.isEmpty()) { LogUtils.d(LOG_TAG, "*** All done; shutting down service"); stopSelf(); break; @@ -1217,32 +1125,37 @@ public class AttachmentService extends Service implements Runnable { } @Override - public int onStartCommand(Intent intent, int flags, int startId) { + public int onStartCommand(final Intent intent, final int flags, final int startId) { if (sRunningService == null) { sRunningService = this; } if (intent != null && intent.hasExtra(EXTRA_ATTACHMENT)) { Attachment att = intent.getParcelableExtra(EXTRA_ATTACHMENT); - onChange(att); + onChange(mContext, att); + } else { + LogUtils.wtf(LOG_TAG, "Received an invalid intent w/o EXTRA_ATTACHMENT"); } return Service.START_STICKY; } @Override public void onCreate() { - // Start up our service thread + // Start up our service thread. new Thread(this, "AttachmentService").start(); } + @Override - public IBinder onBind(Intent intent) { + public IBinder onBind(final Intent intent) { return null; } @Override public void onDestroy() { - // Mark this instance of the service as stopped + // Mark this instance of the service as stopped. Our main loop for the AttachmentService + // checks for this flag along with the AttachmentWatchdog. mStop = true; if (sRunningService != null) { + // Kick it awake to get it to realize that we are stopping. kick(); sRunningService = null; } @@ -1255,27 +1168,31 @@ public class AttachmentService extends Service implements Runnable { // For Debugging. @Override - public void dump(FileDescriptor fd, PrintWriter pw, String[] args) { + public void dump(final FileDescriptor fd, final PrintWriter pw, final String[] args) { pw.println("AttachmentService"); - long time = System.currentTimeMillis(); - synchronized(mDownloadSet) { - pw.println(" Queue, " + mDownloadSet.size() + " entries"); - Iterator iterator = mDownloadSet.descendingIterator(); - // First, start up any required downloads, in priority order - while (iterator.hasNext()) { - DownloadRequest req = iterator.next(); + final long time = System.currentTimeMillis(); + synchronized(mDownloadQueue) { + pw.println(" Queue, " + mDownloadQueue.getSize() + " entries"); + // If you iterate over the queue either via iterator or collection, they are not + // returned in any particular order. With all things being equal its better to go with + // a collection to avoid any potential ConcurrentModificationExceptions. + // If we really want this sorted, we can sort it manually since performance isn't a big + // concern with this debug method. + for (final DownloadRequest req : mDownloadQueue.mRequestMap.values()) { pw.println(" Account: " + req.mAccountId + ", Attachment: " + req.mAttachmentId); pw.println(" Priority: " + req.mPriority + ", Time: " + req.mTime + (req.mInProgress ? " [In progress]" : "")); - Attachment att = Attachment.restoreAttachmentWithId(this, req.mAttachmentId); + final Attachment att = Attachment.restoreAttachmentWithId(this, req.mAttachmentId); if (att == null) { pw.println(" Attachment not in database?"); } else if (att.mFileName != null) { - String fileName = att.mFileName; - String suffix = "[none]"; - int lastDot = fileName.lastIndexOf('.'); + final String fileName = att.mFileName; + final String suffix; + final int lastDot = fileName.lastIndexOf('.'); if (lastDot >= 0) { suffix = fileName.substring(lastDot); + } else { + suffix = "[none]"; } pw.print(" Suffix: " + suffix); if (att.getContentUri() != null) { @@ -1305,10 +1222,10 @@ public class AttachmentService extends Service implements Runnable { } // For Testing - /*package*/ AccountManagerStub mAccountManagerStub; + AccountManagerStub mAccountManagerStub; private final HashMap mAccountServiceMap = new HashMap(); - /*package*/ void addServiceIntentForTest(long accountId, Intent intent) { + void addServiceIntentForTest(final long accountId, final Intent intent) { mAccountServiceMap.put(accountId, intent); } @@ -1316,11 +1233,11 @@ public class AttachmentService extends Service implements Runnable { * 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 { + static class AccountManagerStub { private int mNumberOfAccounts; private final AccountManager mAccountManager; - AccountManagerStub(Context context) { + AccountManagerStub(final Context context) { if (context != null) { mAccountManager = AccountManager.get(context); } else { @@ -1328,7 +1245,7 @@ public class AttachmentService extends Service implements Runnable { } } - /*package*/ int getNumberOfAccounts() { + int getNumberOfAccounts() { if (mAccountManager != null) { return mAccountManager.getAccounts().length; } else { @@ -1336,7 +1253,7 @@ public class AttachmentService extends Service implements Runnable { } } - /*package*/ void setNumberOfAccounts(int numberOfAccounts) { + void setNumberOfAccounts(final int numberOfAccounts) { mNumberOfAccounts = numberOfAccounts; } }