From 7943c8757e8e9a0ad94d0a662105317f078834fc Mon Sep 17 00:00:00 2001 From: Sebastian Kaspari Date: Mon, 21 Dec 2015 09:49:57 +0100 Subject: [PATCH] Bug 1215106 - DownloadContentService: Mark content as permanently failed after multiple errors of the same type. r=rnewman --HG-- extra : commitid : AuFevajQBmB extra : rebase_source : 55dc093df961d8e7962d3c6689a7666a924660e6 --- .../org/mozilla/gecko/dlc/BaseAction.java | 38 ++++++++- .../org/mozilla/gecko/dlc/DownloadAction.java | 19 +++-- .../gecko/dlc/catalog/DownloadContent.java | 41 ++++++++++ .../dlc/catalog/DownloadContentCatalog.java | 14 ++++ .../mozilla/gecko/dlc/TestDownloadAction.java | 77 +++++++++++++++++++ .../catalog/TestDownloadContentCatalog.java | 52 +++++++++++++ 6 files changed, 231 insertions(+), 10 deletions(-) diff --git a/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java b/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java index a734e5403bf9..3a0414caa801 100644 --- a/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java +++ b/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java @@ -6,6 +6,7 @@ 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; @@ -29,12 +30,40 @@ public abstract class BaseAction { /* 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; } } @@ -74,7 +103,8 @@ public abstract class BaseAction { 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]; @@ -94,7 +124,7 @@ public abstract class BaseAction { 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); } diff --git a/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java b/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java index 225b2376e1c5..ac8aedc86f9e 100644 --- a/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java +++ b/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java @@ -126,6 +126,11 @@ public class DownloadAction extends BaseAction { } } 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); @@ -161,7 +166,8 @@ public class DownloadAction extends BaseAction { // 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); @@ -176,7 +182,7 @@ public class DownloadAction extends BaseAction { 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()); @@ -188,7 +194,7 @@ public class DownloadAction extends BaseAction { 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); @@ -229,7 +235,7 @@ public class DownloadAction extends BaseAction { 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); @@ -272,7 +278,8 @@ public class DownloadAction extends BaseAction { 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()); @@ -308,7 +315,7 @@ public class DownloadAction extends BaseAction { } 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); diff --git a/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java b/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java index fbb3eb34dddf..445dd995ddb9 100644 --- a/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java +++ b/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContent.java @@ -23,6 +23,8 @@ public class DownloadContent { 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 {} @@ -50,6 +52,8 @@ public class DownloadContent { 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, @@ -121,6 +125,24 @@ public class DownloadContent { 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)) @@ -133,6 +155,7 @@ public class DownloadContent { .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(); } @@ -148,6 +171,12 @@ public class DownloadContent { 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; } @@ -166,11 +195,16 @@ public class DownloadContent { 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; } @@ -223,5 +257,12 @@ public class DownloadContent { this.state = state; return this; } + + /* package-private */ Builder setFailures(int failures, int lastFailureType) { + this.failures = failures; + this.lastFailureType = lastFailureType; + + return this; + } } } diff --git a/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java b/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java index 2b4a1ee55021..9fbcb1adb1f9 100644 --- a/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java +++ b/mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java @@ -31,6 +31,8 @@ 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 content; // Guarded by 'this' private boolean hasLoadedCatalog; // Guarded by 'this @@ -107,6 +109,7 @@ public class DownloadContentCatalog { public synchronized void markAsDownloaded(DownloadContent content) { content.setState(DownloadContent.STATE_DOWNLOADED); + content.resetFailures(); hasCatalogChanged = true; } @@ -120,6 +123,17 @@ public class DownloadContentCatalog { 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() { diff --git 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 index a33cd560ec52..18cca87f6d59 100644 --- 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 @@ -426,6 +426,83 @@ public class TestDownloadAction { 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) diff --git 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 index 3ad958b7e76c..aa7947b8044a 100644 --- 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 @@ -206,4 +206,56 @@ public class TestDownloadContentCatalog { 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()); + } }