Fix an infinitely retrying download problem

b/11436795
If an attachment download fails due to a timeout, or
an exception being thrown from startDownload(), we'd call
cancelDownload() on it. But this didn't actually cancel,
it would remove it from the inProgres list, but leave it
in the list of all downloads, so we'd immediately retry it.
This is bad for two reasons:
1. It can starve out other attachment downloads that could
have been successful.
2. It will keep attempting to do network work, even if it's
hopeless, forever, draining battery.
Now, if an attachment download fails in this way, for the first
few times, we'll move it to the tail end of the list of
downloads we'd like to perform. If it fails more than 10 times,
we'll give up completely. Giving up is not permanent, if we
have a reason to attempt a download again (such as the user
tapping on it), then it will get added back to the download
service and retried.

Change-Id: I5364a7d8b4b25ce299b8dcf061db6e9ce12daf75
This commit is contained in:
Martin Hibdon 2013-11-08 13:57:51 -08:00
parent 33efdd7dad
commit 272b317f3d

View File

@ -196,9 +196,25 @@ public class AttachmentDownloadService extends Service implements Runnable {
accountId = messageId = -1; accountId = messageId = -1;
} }
priority = getPriority(attachment); priority = getPriority(attachment);
time = System.currentTimeMillis(); time = SystemClock.elapsedRealtime();
} }
private DownloadRequest(DownloadRequest orig, long newTime) {
priority = orig.priority;
attachmentId = orig.attachmentId;
messageId = orig.messageId;
accountId = orig.accountId;
time = newTime;
inProgress = orig.inProgress;
lastStatusCode = orig.lastStatusCode;
lastProgress = orig.lastProgress;
lastCallbackTime = orig.lastCallbackTime;
startTime = orig.startTime;
retryCount = orig.retryCount;
retryStartTime = orig.retryStartTime;
}
@Override @Override
public int hashCode() { public int hashCode() {
return (int)attachmentId; return (int)attachmentId;
@ -527,8 +543,24 @@ public class AttachmentDownloadService extends Service implements Runnable {
} }
private void cancelDownload(DownloadRequest req) { private void cancelDownload(DownloadRequest req) {
mDownloadsInProgress.remove(req.attachmentId); LogUtils.d(TAG, "cancelDownload #%d", req.attachmentId);
req.inProgress = false; req.inProgress = false;
mDownloadsInProgress.remove(req.attachmentId);
// Remove the download from our queue, and then decide whether or not to add it back.
remove(req);
req.retryCount++;
if (req.retryCount > CONNECTION_ERROR_MAX_RETRIES) {
LogUtils.d(TAG, "too many failures, giving up");
} else {
LogUtils.d(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);
}
} }
/** /**