Bug 1215106 - DownloadContentService: Mark content as permanently failed after multiple errors of the same type. r?rnewman
--- a/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java
@@ -1,16 +1,17 @@
/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package org.mozilla.gecko.dlc;
import android.content.Context;
+import android.support.annotation.IntDef;
import android.util.Log;
import org.mozilla.gecko.background.nativecode.NativeCrypto;
import org.mozilla.gecko.dlc.catalog.DownloadContent;
import org.mozilla.gecko.dlc.catalog.DownloadContentCatalog;
import org.mozilla.gecko.sync.Utils;
import org.mozilla.gecko.util.IOUtils;
@@ -24,22 +25,50 @@ public abstract class BaseAction {
private static final String LOGTAG = "GeckoDLCBaseAction";
/**
* Exception indicating a recoverable error has happened. Download of the content will be retried later.
*/
/* package-private */ static class RecoverableDownloadContentException extends Exception {
private static final long serialVersionUID = -2246772819507370734L;
- public RecoverableDownloadContentException(String message) {
+ @IntDef({MEMORY, DISK_IO, SERVER, NETWORK})
+ public @interface ErrorType {}
+ public static final int MEMORY = 1;
+ public static final int DISK_IO = 2;
+ public static final int SERVER = 3;
+ public static final int NETWORK = 4;
+
+ private int errorType;
+
+ public RecoverableDownloadContentException(@ErrorType int errorType, String message) {
super(message);
+ this.errorType = errorType;
}
- public RecoverableDownloadContentException(Throwable cause) {
+ public RecoverableDownloadContentException(@ErrorType int errorType, Throwable cause) {
super(cause);
+ this.errorType = errorType;
+ }
+
+ @ErrorType
+ public int getErrorType() {
+ return errorType;
+ }
+
+ /**
+ * Should this error be counted as failure? If this type of error will happen multiple times in a row then this
+ * error will be treated as permanently and the operation will not be tried again until the content changes.
+ */
+ public boolean shouldBeCountedAsFailure() {
+ if (NETWORK == errorType) {
+ return false; // Always retry after network errors
+ }
+
+ return true;
}
}
/**
* If this exception is thrown the content will be marked as unrecoverable, permanently failed and we will not try
* downloading it again - until a newer version of the content is available.
*/
/* package-private */ static class UnrecoverableDownloadContentException extends Exception {
@@ -69,17 +98,18 @@ public abstract class BaseAction {
throws RecoverableDownloadContentException, UnrecoverableDownloadContentException {
InputStream inputStream = null;
try {
inputStream = new BufferedInputStream(new FileInputStream(file));
byte[] ctx = NativeCrypto.sha256init();
if (ctx == null) {
- throw new RecoverableDownloadContentException("Could not create SHA-256 context");
+ throw new RecoverableDownloadContentException(RecoverableDownloadContentException.MEMORY,
+ "Could not create SHA-256 context");
}
byte[] buffer = new byte[4096];
int read;
while ((read = inputStream.read(buffer)) != -1) {
NativeCrypto.sha256update(ctx, buffer, read);
}
@@ -89,14 +119,14 @@ public abstract class BaseAction {
if (!actualChecksum.equalsIgnoreCase(expectedChecksum)) {
Log.w(LOGTAG, "Checksums do not match. Expected=" + expectedChecksum + ", Actual=" + actualChecksum);
return false;
}
return true;
} catch (IOException e) {
// Recoverable: Just I/O discontinuation
- throw new RecoverableDownloadContentException(e);
+ throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO, e);
} finally {
IOUtils.safeStreamClose(inputStream);
}
}
}
--- a/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
@@ -121,16 +121,21 @@ public class DownloadAction extends Base
callback.onContentDownloaded(content);
}
if (temporaryFile != null && temporaryFile.exists()) {
temporaryFile.delete();
}
} catch (RecoverableDownloadContentException e) {
Log.w(LOGTAG, "Downloading content failed (Recoverable): " + content , e);
+
+ if (e.shouldBeCountedAsFailure()) {
+ catalog.rememberFailure(content, e.getErrorType());
+ }
+
// TODO: Reschedule download (bug 1209498)
} catch (UnrecoverableDownloadContentException e) {
Log.w(LOGTAG, "Downloading content failed (Unrecoverable): " + content, e);
catalog.markAsPermanentlyFailed(content);
if (temporaryFile != null && temporaryFile.exists()) {
temporaryFile.delete();
@@ -156,44 +161,45 @@ public class DownloadAction extends Base
try {
final HttpResponse response = client.execute(request);
final int status = response.getStatusLine().getStatusCode();
if (status != HttpStatus.SC_OK && status != HttpStatus.SC_PARTIAL_CONTENT) {
// We are trying to be smart and only retry if this is an error that might resolve in the future.
// TODO: This is guesstimating at best. We want to implement failure counters (Bug 1215106).
if (status >= 500) {
// Recoverable: Server errors 5xx
- throw new RecoverableDownloadContentException("(Recoverable) Download failed. Status code: " + status);
+ throw new RecoverableDownloadContentException(RecoverableDownloadContentException.SERVER,
+ "(Recoverable) Download failed. Status code: " + status);
} else if (status >= 400) {
// Unrecoverable: Client errors 4xx - Unlikely that this version of the client will ever succeed.
throw new UnrecoverableDownloadContentException("(Unrecoverable) Download failed. Status code: " + status);
} else {
// Informational 1xx: They have no meaning to us.
// Successful 2xx: We don't know how to handle anything but 200.
// Redirection 3xx: HttpClient should have followed redirects if possible. We should not see those errors here.
throw new UnrecoverableDownloadContentException("(Unrecoverable) Download failed. Status code: " + status);
}
}
final HttpEntity entity = response.getEntity();
if (entity == null) {
// Recoverable: Should not happen for a valid asset
- throw new RecoverableDownloadContentException("Null entity");
+ throw new RecoverableDownloadContentException(RecoverableDownloadContentException.SERVER, "Null entity");
}
inputStream = new BufferedInputStream(entity.getContent());
outputStream = openFile(temporaryFile, status == HttpStatus.SC_PARTIAL_CONTENT);
IOUtils.copy(inputStream, outputStream);
inputStream.close();
outputStream.close();
} catch (IOException e) {
// Recoverable: Just I/O discontinuation
- throw new RecoverableDownloadContentException(e);
+ throw new RecoverableDownloadContentException(RecoverableDownloadContentException.NETWORK, e);
} finally {
IOUtils.safeStreamClose(inputStream);
IOUtils.safeStreamClose(outputStream);
}
}
protected OutputStream openFile(File file, boolean append) throws FileNotFoundException {
return new BufferedOutputStream(new FileOutputStream(file, append));
@@ -224,17 +230,17 @@ public class DownloadAction extends Base
if (!verify(temporaryFile, checksum)) {
Log.w(LOGTAG, "Checksum of extracted file does not match.");
return;
}
move(temporaryFile, destinationFile);
} catch (IOException e) {
// We could not extract to the destination: Keep temporary file and try again next time we run.
- throw new RecoverableDownloadContentException(e);
+ throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO, e);
} finally {
IOUtils.safeStreamClose(inputStream);
IOUtils.safeStreamClose(outputStream);
if (temporaryFile != null && temporaryFile.exists()) {
temporaryFile.delete();
}
}
@@ -267,17 +273,18 @@ public class DownloadAction extends Base
}
protected File createTemporaryFile(Context context, DownloadContent content)
throws RecoverableDownloadContentException {
File cacheDirectory = new File(context.getCacheDir(), CACHE_DIRECTORY);
if (!cacheDirectory.exists() && !cacheDirectory.mkdirs()) {
// Recoverable: File system might not be mounted NOW and we didn't download anything yet anyways.
- throw new RecoverableDownloadContentException("Could not create cache directory: " + cacheDirectory);
+ throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO,
+ "Could not create cache directory: " + cacheDirectory);
}
return new File(cacheDirectory, content.getDownloadChecksum() + "-" + content.getId());
}
protected void move(File temporaryFile, File destinationFile)
throws RecoverableDownloadContentException, UnrecoverableDownloadContentException {
if (!temporaryFile.renameTo(destinationFile)) {
@@ -303,17 +310,17 @@ public class DownloadAction extends Base
IOUtils.copy(inputStream, outputStream);
inputStream.close();
outputStream.close();
} catch (IOException e) {
// We could not copy the temporary file to its destination: Keep the temporary file and
// try again the next time we run.
- throw new RecoverableDownloadContentException(e);
+ throw new RecoverableDownloadContentException(RecoverableDownloadContentException.DISK_IO, e);
} finally {
IOUtils.safeStreamClose(inputStream);
IOUtils.safeStreamClose(outputStream);
}
}
protected boolean hasEnoughDiskSpace(DownloadContent content, File destinationFile, File temporaryFile) {
final File temporaryDirectory = temporaryFile.getParentFile();
--- a/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java
+++ b/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java
@@ -18,16 +18,18 @@ public class DownloadContent {
private static final String KEY_FILENAME = "filename";
private static final String KEY_CHECKSUM = "checksum";
private static final String KEY_DOWNLOAD_CHECKSUM = "download_checksum";
private static final String KEY_LAST_MODIFIED = "last_modified";
private static final String KEY_TYPE = "type";
private static final String KEY_KIND = "kind";
private static final String KEY_SIZE = "size";
private static final String KEY_STATE = "state";
+ private static final String KEY_FAILURES = "failures";
+ private static final String KEY_LAST_FAILURE_TYPE = "last_failure_type";
@IntDef({STATE_NONE, STATE_SCHEDULED, STATE_DOWNLOADED, STATE_FAILED, STATE_IGNORED})
public @interface State {}
public static final int STATE_NONE = 0;
public static final int STATE_SCHEDULED = 1;
public static final int STATE_DOWNLOADED = 2;
public static final int STATE_FAILED = 3; // Permanently failed for this version of the content
public static final int STATE_IGNORED = 4;
@@ -45,16 +47,18 @@ public class DownloadContent {
private final String filename;
private final String checksum;
private final String downloadChecksum;
private final long lastModified;
private final String type;
private final String kind;
private final long size;
private int state;
+ private int failures;
+ private int lastFailureType;
private DownloadContent(@NonNull String id, @NonNull String location, @NonNull String filename,
@NonNull String checksum, @NonNull String downloadChecksum, @NonNull long lastModified,
@NonNull String type, @NonNull String kind, long size) {
this.id = id;
this.location = location;
this.filename = filename;
this.checksum = checksum;
@@ -116,43 +120,68 @@ public class DownloadContent {
public boolean isFont() {
return KIND_FONT.equals(kind);
}
public boolean isAssetArchive() {
return TYPE_ASSET_ARCHIVE.equals(type);
}
+ /* package-private */ int getFailures() {
+ return failures;
+ }
+
+ /* package-private */ void rememberFailure(int failureType) {
+ if (lastFailureType != failureType) {
+ lastFailureType = failureType;
+ failures = 1;
+ } else {
+ failures++;
+ }
+ }
+
+ /* package-private */ void resetFailures() {
+ failures = 0;
+ lastFailureType = 0;
+ }
+
public static DownloadContent fromJSON(JSONObject object) throws JSONException {
return new Builder()
.setId(object.getString(KEY_ID))
.setLocation(object.getString(KEY_LOCATION))
.setFilename(object.getString(KEY_FILENAME))
.setChecksum(object.getString(KEY_CHECKSUM))
.setDownloadChecksum(object.getString(KEY_DOWNLOAD_CHECKSUM))
.setLastModified(object.getLong(KEY_LAST_MODIFIED))
.setType(object.getString(KEY_TYPE))
.setKind(object.getString(KEY_KIND))
.setSize(object.getLong(KEY_SIZE))
.setState(object.getInt(KEY_STATE))
+ .setFailures(object.optInt(KEY_FAILURES), object.optInt(KEY_LAST_FAILURE_TYPE))
.build();
}
public JSONObject toJSON() throws JSONException {
JSONObject object = new JSONObject();
object.put(KEY_ID, id);
object.put(KEY_LOCATION, location);
object.put(KEY_FILENAME, filename);
object.put(KEY_CHECKSUM, checksum);
object.put(KEY_DOWNLOAD_CHECKSUM, downloadChecksum);
object.put(KEY_LAST_MODIFIED, lastModified);
object.put(KEY_TYPE, type);
object.put(KEY_KIND, kind);
object.put(KEY_SIZE, size);
object.put(KEY_STATE, state);
+
+ if (failures > 0) {
+ object.put(KEY_FAILURES, failures);
+ object.put(KEY_LAST_FAILURE_TYPE, lastFailureType);
+ }
+
return object;
}
public String toString() {
return String.format("[%s,%s] %s (%d bytes) %s", getType(), getKind(), getId(), getSize(), getChecksum());
}
public static class Builder {
@@ -161,21 +190,26 @@ public class DownloadContent {
private String filename;
private String checksum;
private String downloadChecksum;
private long lastModified;
private String type;
private String kind;
private long size;
private int state;
+ private int failures;
+ private int lastFailureType;
public DownloadContent build() {
DownloadContent content = new DownloadContent(id, location, filename, checksum, downloadChecksum,
lastModified, type, kind, size);
content.setState(state);
+ content.failures = failures;
+ content.lastFailureType = lastFailureType;
+
return content;
}
public Builder setId(String id) {
this.id = id;
return this;
}
@@ -218,10 +252,17 @@ public class DownloadContent {
this.size = size;
return this;
}
public Builder setState(int state) {
this.state = state;
return this;
}
+
+ /* package-private */ Builder setFailures(int failures, int lastFailureType) {
+ this.failures = failures;
+ this.lastFailureType = lastFailureType;
+
+ return this;
+ }
}
}
--- a/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java
+++ b/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java
@@ -26,16 +26,18 @@ import java.util.List;
*
* Changing elements returned by the catalog should be guarded by the catalog instance to guarantee visibility when
* persisting changes.
*/
public class DownloadContentCatalog {
private static final String LOGTAG = "GeckoDLCCatalog";
private static final String FILE_NAME = "download_content_catalog";
+ private static final int MAX_FAILURES_UNTIL_PERMANENTLY_FAILED = 10;
+
private final AtomicFile file; // Guarded by 'file'
private List<DownloadContent> content; // Guarded by 'this'
private boolean hasLoadedCatalog; // Guarded by 'this
private boolean hasCatalogChanged; // Guarded by 'this'
public DownloadContentCatalog(Context context) {
this(new AtomicFile(new File(context.getApplicationInfo().dataDir, FILE_NAME)));
@@ -102,29 +104,41 @@ public class DownloadContentCatalog {
public synchronized void scheduleDownload(DownloadContent content) {
content.setState(DownloadContent.STATE_SCHEDULED);
hasCatalogChanged = true;
}
public synchronized void markAsDownloaded(DownloadContent content) {
content.setState(DownloadContent.STATE_DOWNLOADED);
+ content.resetFailures();
hasCatalogChanged = true;
}
public synchronized void markAsPermanentlyFailed(DownloadContent content) {
content.setState(DownloadContent.STATE_FAILED);
hasCatalogChanged = true;
}
public synchronized void markAsIgnored(DownloadContent content) {
content.setState(DownloadContent.STATE_IGNORED);
hasCatalogChanged = true;
}
+ public synchronized void rememberFailure(DownloadContent content, int failureType) {
+ if (content.getFailures() == MAX_FAILURES_UNTIL_PERMANENTLY_FAILED) {
+ Log.d(LOGTAG, "Maximum number of failures reached. Marking content has permanently failed.");
+
+ markAsPermanentlyFailed(content);
+ } else {
+ content.rememberFailure(failureType);
+ hasCatalogChanged = true;
+ }
+ }
+
public void persistChanges() {
new Thread(LOGTAG + "-Persist") {
public void run() {
writeToDisk();
}
}.start();
}
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/TestDownloadAction.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/TestDownloadAction.java
@@ -421,16 +421,93 @@ public class TestDownloadAction {
DownloadContent content = createFontWithSize(2048);
File destinationFile = mockNotExistingFileWithUsableSpace(4096);
File temporaryFile = mockNotExistingFileWithUsableSpace(4096);
DownloadAction action = new DownloadAction(null);
Assert.assertTrue(action.hasEnoughDiskSpace(content, destinationFile, temporaryFile));
}
+ /**
+ * Scenario: Download failed with network I/O error.
+ *
+ * Verify that:
+ * * Error is not counted as failure
+ */
+ @Test
+ public void testNetworkErrorIsNotCountedAsFailure() throws Exception {
+ DownloadContent content = createFont();
+ DownloadContentCatalog catalog = mockCatalogWithScheduledDownloads(content);
+
+ DownloadAction action = spy(new DownloadAction(null));
+ doReturn(true).when(action).isConnectedToNetwork(RuntimeEnvironment.application);
+ doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+ doReturn(mockNotExistingFile()).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
+ doReturn(mockNotExistingFile()).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+ doReturn(true).when(action).hasEnoughDiskSpace(eq(content), any(File.class), any(File.class));
+
+ HttpClient client = mock(HttpClient.class);
+ doThrow(IOException.class).when(client).execute(any(HttpUriRequest.class));
+ doReturn(client).when(action).buildHttpClient();
+
+ action.perform(RuntimeEnvironment.application, catalog);
+
+ verify(catalog, never()).rememberFailure(eq(content), anyInt());
+ verify(catalog, never()).markAsDownloaded(content);
+ }
+
+ /**
+ * Scenario: Disk IO Error when extracting file.
+ *
+ * Verify that:
+ * * Error is counted as failure
+ * * After multiple errors the content is marked as permanently failed
+ */
+ @Test
+ public void testDiskIOErrorIsCountedAsFailure() throws Exception {
+ DownloadContent content = createFont();
+ DownloadContentCatalog catalog = mockCatalogWithScheduledDownloads(content);
+ doCallRealMethod().when(catalog).rememberFailure(eq(content), anyInt());
+ doCallRealMethod().when(catalog).markAsPermanentlyFailed(content);
+
+ Assert.assertEquals(DownloadContent.STATE_NONE, content.getState());
+
+ DownloadAction action = spy(new DownloadAction(null));
+ doReturn(true).when(action).isConnectedToNetwork(RuntimeEnvironment.application);
+ doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+ doReturn(mockNotExistingFile()).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
+ doReturn(mockNotExistingFile()).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+ doReturn(true).when(action).hasEnoughDiskSpace(eq(content), any(File.class), any(File.class));
+ doNothing().when(action).download(any(HttpClient.class), anyString(), any(File.class));
+ doReturn(true).when(action).verify(any(File.class), anyString());
+
+ File destinationFile = mock(File.class);
+ doReturn(false).when(destinationFile).exists();
+ File parentFile = mock(File.class);
+ doReturn(false).when(parentFile).mkdirs();
+ doReturn(false).when(parentFile).exists();
+ doReturn(parentFile).when(destinationFile).getParentFile();
+ doReturn(destinationFile).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+
+ for (int i = 0; i < 10; i++) {
+ action.perform(RuntimeEnvironment.application, catalog);
+
+ Assert.assertEquals(DownloadContent.STATE_NONE, content.getState());
+ }
+
+ action.perform(RuntimeEnvironment.application, catalog);
+
+ Assert.assertEquals(DownloadContent.STATE_FAILED, content.getState());
+ verify(catalog, times(11)).rememberFailure(eq(content), anyInt());
+ }
+
+ private DownloadContent createFont() {
+ return createFontWithSize(102400L);
+ }
+
private DownloadContent createFontWithSize(long size) {
return new DownloadContent.Builder()
.setKind(DownloadContent.KIND_FONT)
.setType(DownloadContent.TYPE_ASSET_ARCHIVE)
.setSize(size)
.build();
}
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/catalog/TestDownloadContentCatalog.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/catalog/TestDownloadContentCatalog.java
@@ -201,9 +201,61 @@ public class TestDownloadContentCatalog
Assert.assertTrue(catalog.getContentWithoutState().contains(content2));
Assert.assertTrue(catalog.getDownloadedContent().contains(content6));
Assert.assertTrue(catalog.getScheduledDownloads().contains(content3));
Assert.assertTrue(catalog.getScheduledDownloads().contains(content4));
Assert.assertTrue(catalog.getScheduledDownloads().contains(content5));
}
+
+ /**
+ * Scenario: Calling rememberFailure() on a catalog with varying values
+ */
+ @Test
+ public void testRememberingFailures() {
+ DownloadContentCatalog catalog = new DownloadContentCatalog(mock(AtomicFile.class));
+ Assert.assertFalse(catalog.hasCatalogChanged());
+
+ DownloadContent content = new DownloadContent.Builder().build();
+ Assert.assertEquals(0, content.getFailures());
+
+ catalog.rememberFailure(content, 42);
+ Assert.assertEquals(1, content.getFailures());
+ Assert.assertTrue(catalog.hasCatalogChanged());
+
+ catalog.rememberFailure(content, 42);
+ Assert.assertEquals(2, content.getFailures());
+
+ // Failure counter is reset if different failure has been reported
+ catalog.rememberFailure(content, 23);
+ Assert.assertEquals(1, content.getFailures());
+
+ // Failure counter is reset after successful download
+ catalog.markAsDownloaded(content);
+ Assert.assertEquals(0, content.getFailures());
+ }
+
+ /**
+ * Scenario: Content has failed multiple times with the same failure type.
+ *
+ * Verify that:
+ * * Content is marked as permanently failed
+ */
+ @Test
+ public void testContentWillBeMarkedAsPermanentlyFailedAfterMultipleFailures() {
+ DownloadContentCatalog catalog = new DownloadContentCatalog(mock(AtomicFile.class));
+
+ DownloadContent content = new DownloadContent.Builder().build();
+ Assert.assertEquals(DownloadContent.STATE_NONE, content.getState());
+
+ for (int i = 0; i < 10; i++) {
+ catalog.rememberFailure(content, 42);
+
+ Assert.assertEquals(i + 1, content.getFailures());
+ Assert.assertEquals(DownloadContent.STATE_NONE, content.getState());
+ }
+
+ catalog.rememberFailure(content, 42);
+ Assert.assertEquals(10, content.getFailures());
+ Assert.assertEquals(DownloadContent.STATE_FAILED, content.getState());
+ }
}