Bug 1209513 - DownloadContentService: Add support for resuming downloads. r?rnewman draft
authorSebastian Kaspari <s.kaspari@gmail.com>
Thu, 17 Dec 2015 11:36:16 +0100
changeset 315970 c4d304723818c2888dc99aa4cd5806bebbafa73b
parent 315968 97f2becb4b8de0c7d9ff55dc07ca62ee16a0d30e
child 512109 44c5cd28a134dfa130df1085b79a8af3cc88bc31
push id8491
push users.kaspari@gmail.com
push dateThu, 17 Dec 2015 11:46:21 +0000
reviewersrnewman
bugs1209513
milestone46.0a1
Bug 1209513 - DownloadContentService: Add support for resuming downloads. r?rnewman
mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/dlc/TestDownloadAction.java
--- a/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java
@@ -15,16 +15,17 @@ import org.mozilla.gecko.dlc.catalog.Dow
 import org.mozilla.gecko.dlc.catalog.DownloadContentCatalog;
 import org.mozilla.gecko.util.HardwareUtils;
 import org.mozilla.gecko.util.IOUtils;
 
 import java.io.BufferedInputStream;
 import java.io.BufferedOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
+import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.zip.GZIPInputStream;
 
 import ch.boye.httpclientandroidlib.HttpEntity;
 import ch.boye.httpclientandroidlib.HttpResponse;
@@ -76,66 +77,79 @@ public class DownloadAction extends Base
                     catalog.markAsDownloaded(content);
                     continue;
                 }
 
                 temporaryFile = createTemporaryFile(context, content);
 
                 // TODO: Check space on disk before downloading content (bug 1220145)
                 final String url = createDownloadURL(content);
-                download(client, url, temporaryFile);
+
+                if (!temporaryFile.exists() || temporaryFile.length() < content.getSize()) {
+                    download(client, url, temporaryFile);
+                }
 
                 if (!verify(temporaryFile, content.getDownloadChecksum())) {
                     Log.w(LOGTAG, "Wrong checksum after download, content=" + content.getId());
                     temporaryFile.delete();
                     continue;
                 }
 
                 if (!content.isAssetArchive()) {
                     Log.e(LOGTAG, "Downloaded content is not of type 'asset-archive': " + content.getType());
+                    temporaryFile.delete();
                     continue;
                 }
 
                 extract(temporaryFile, destinationFile, content.getChecksum());
 
                 catalog.markAsDownloaded(content);
 
                 Log.d(LOGTAG, "Successfully downloaded: " + content);
 
                 if (callback != null) {
                     callback.onContentDownloaded(content);
                 }
+
+                if (temporaryFile != null && temporaryFile.exists()) {
+                    temporaryFile.delete();
+                }
             } catch (RecoverableDownloadContentException e) {
                 Log.w(LOGTAG, "Downloading content failed (Recoverable): " + content , e);
                 // TODO: Reschedule download (bug 1209498)
             } catch (UnrecoverableDownloadContentException e) {
                 Log.w(LOGTAG, "Downloading content failed (Unrecoverable): " + content, e);
 
                 catalog.markAsPermanentlyFailed(content);
-            } finally {
+
                 if (temporaryFile != null && temporaryFile.exists()) {
                     temporaryFile.delete();
                 }
             }
         }
 
         Log.v(LOGTAG, "Done");
     }
 
-    /* package-private */ void download(HttpClient client, String source, File temporaryFile)
+    protected void download(HttpClient client, String source, File temporaryFile)
             throws RecoverableDownloadContentException, UnrecoverableDownloadContentException {
         InputStream inputStream = null;
         OutputStream outputStream = null;
 
         final HttpGet request = new HttpGet(source);
 
+        final long offset = temporaryFile.exists() ? temporaryFile.length() : 0;
+        if (offset > 0) {
+            request.setHeader("Range", "bytes=" + offset + "-");
+        }
+
         try {
             final HttpResponse response = client.execute(request);
             final int status = response.getStatusLine().getStatusCode();
-            if (status != HttpStatus.SC_OK) {
+            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);
                 } 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);
@@ -149,34 +163,35 @@ public class DownloadAction extends Base
 
             final HttpEntity entity = response.getEntity();
             if (entity == null) {
                 // Recoverable: Should not happen for a valid asset
                 throw new RecoverableDownloadContentException("Null entity");
             }
 
             inputStream = new BufferedInputStream(entity.getContent());
-            outputStream = new BufferedOutputStream(new FileOutputStream(temporaryFile));
+            outputStream = openFile(temporaryFile, status == HttpStatus.SC_PARTIAL_CONTENT);
 
             IOUtils.copy(inputStream, outputStream);
 
             inputStream.close();
             outputStream.close();
         } catch (IOException e) {
-            // TODO: Support resuming downloads using 'Range' header (Bug 1209513)
-            temporaryFile.delete();
-
             // Recoverable: Just I/O discontinuation
             throw new RecoverableDownloadContentException(e);
         } finally {
             IOUtils.safeStreamClose(inputStream);
             IOUtils.safeStreamClose(outputStream);
         }
     }
 
+    protected OutputStream openFile(File file, boolean append) throws FileNotFoundException {
+        return new BufferedOutputStream(new FileOutputStream(file, append));
+    }
+
     protected void extract(File sourceFile, File destinationFile, String checksum)
             throws UnrecoverableDownloadContentException, RecoverableDownloadContentException {
         InputStream inputStream = null;
         OutputStream outputStream = null;
         File temporaryFile = null;
 
         try {
             File destinationDirectory = destinationFile.getParentFile();
@@ -196,19 +211,18 @@ 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 do not support resume yet (Bug 1209513). Therefore we have to treat this as unrecoverable: The
-            // temporarily file will be deleted and we want to avoid downloading and failing repeatedly.
-            throw new UnrecoverableDownloadContentException(e);
+            // We could not extract to the destination: Keep temporary file and try again next time we run.
+            throw new RecoverableDownloadContentException(e);
         } finally {
             IOUtils.safeStreamClose(inputStream);
             IOUtils.safeStreamClose(outputStream);
 
             if (temporaryFile != null && temporaryFile.exists()) {
                 temporaryFile.delete();
             }
         }
@@ -268,19 +282,17 @@ public class DownloadAction extends Base
             inputStream = new BufferedInputStream(new FileInputStream(temporaryFile));
             outputStream = new BufferedOutputStream(new FileOutputStream(destinationFile));
 
             IOUtils.copy(inputStream, outputStream);
 
             inputStream.close();
             outputStream.close();
         } catch (IOException e) {
-            // Meh. This is an awkward situation: We downloaded the content but we can't move it to its destination. We
-            // are treating this as "unrecoverable" error because we want to avoid downloading this again and again and
-            // then always failing to copy it to the destination. This will be fixed after we implement resuming
-            // downloads (Bug 1209513): We will keep the downloaded temporary file and just retry copying.
-            throw new UnrecoverableDownloadContentException(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);
         } finally {
             IOUtils.safeStreamClose(inputStream);
             IOUtils.safeStreamClose(outputStream);
         }
     }
 }
--- 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
@@ -1,35 +1,46 @@
 /* -*- 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 org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
 import org.mozilla.gecko.background.testhelpers.TestRunner;
 import org.mozilla.gecko.dlc.catalog.DownloadContent;
 import org.mozilla.gecko.dlc.catalog.DownloadContentCatalog;
 import org.robolectric.RuntimeEnvironment;
 
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.File;
+import java.io.IOException;
 import java.util.Collections;
 
+import ch.boye.httpclientandroidlib.HttpEntity;
 import ch.boye.httpclientandroidlib.HttpResponse;
+import ch.boye.httpclientandroidlib.HttpStatus;
 import ch.boye.httpclientandroidlib.StatusLine;
 import ch.boye.httpclientandroidlib.client.HttpClient;
+import ch.boye.httpclientandroidlib.client.methods.HttpGet;
 import ch.boye.httpclientandroidlib.client.methods.HttpUriRequest;
 
 import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyBoolean;
+import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 
 /**
  * DownloadAction: Download content that has been scheduled during "study" or "verify".
  */
@@ -86,50 +97,42 @@ public class TestDownloadAction {
     /**
      * Scenario: Server returns a server error (HTTP 500).
      *
      * Verify that:
      *  * Situation is treated as recoverable (RecoverableDownloadContentException)
      */
     @Test(expected=BaseAction.RecoverableDownloadContentException.class)
     public void testServerErrorsAreRecoverable() throws Exception {
-        StatusLine status = mock(StatusLine.class);
-        doReturn(500).when(status).getStatusCode();
+        HttpClient client = mockHttpClient(500, "");
 
-        HttpResponse response = mock(HttpResponse.class);
-        doReturn(status).when(response).getStatusLine();
-
-        HttpClient client = mock(HttpClient.class);
-        doReturn(response).when(client).execute(any(HttpUriRequest.class));
+        File temporaryFile = mock(File.class);
+        doReturn(false).when(temporaryFile).exists();
 
         DownloadAction action = spy(new DownloadAction(null));
-        action.download(client, TEST_URL, null);
+        action.download(client, TEST_URL, temporaryFile);
 
         verify(client).execute(any(HttpUriRequest.class));
     }
 
     /**
      * Scenario: Server returns a client error (HTTP 404).
      *
      * Verify that:
      *  * Situation is treated as unrecoverable (UnrecoverableDownloadContentException)
      */
     @Test(expected=BaseAction.UnrecoverableDownloadContentException.class)
     public void testClientErrorsAreUnrecoverable() throws Exception {
-        StatusLine status = mock(StatusLine.class);
-        doReturn(404).when(status).getStatusCode();
+        HttpClient client = mockHttpClient(404, "");
 
-        HttpResponse response = mock(HttpResponse.class);
-        doReturn(status).when(response).getStatusLine();
-
-        HttpClient client = mock(HttpClient.class);
-        doReturn(response).when(client).execute(any(HttpUriRequest.class));
+        File temporaryFile = mock(File.class);
+        doReturn(false).when(temporaryFile).exists();
 
         DownloadAction action = spy(new DownloadAction(null));
-        action.download(client, TEST_URL, null);
+        action.download(client, TEST_URL, temporaryFile);
 
         verify(client).execute(any(HttpUriRequest.class));
     }
 
     /**
      * Scenario: A successful download has been performed.
      *
      * Verify that:
@@ -161,9 +164,201 @@ public class TestDownloadAction {
 
         action.perform(RuntimeEnvironment.application, catalog);
 
         verify(action).buildHttpClient();
         verify(action).download(any(HttpClient.class), anyString(), eq(file));
         verify(action).extract(eq(file), eq(file), anyString());
         verify(catalog).markAsDownloaded(content);
     }
+
+    /**
+     * Scenario: Pretend a partially downloaded file already exists.
+     *
+     * Verify that:
+     *  * Range header is set in request
+     *  * Content will be appended to existing file
+     *  * Content will be marked as downloaded in catalog
+     */
+    @Test
+    public void testResumingDownloadFromExistingFile() throws Exception {
+        DownloadContent content = new DownloadContent.Builder()
+                .setKind(DownloadContent.KIND_FONT)
+                .setType(DownloadContent.TYPE_ASSET_ARCHIVE)
+                .setSize(4223)
+                .build();
+
+        DownloadContentCatalog catalog = mock(DownloadContentCatalog.class);
+        doReturn(Collections.singletonList(content)).when(catalog).getScheduledDownloads();
+
+        DownloadAction action = spy(new DownloadAction(null));
+        doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+
+        File temporaryFile = mockTemporaryFile(true, 1337L);
+        doReturn(temporaryFile).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
+
+        ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
+        doReturn(outputStream).when(action).openFile(eq(temporaryFile), anyBoolean());
+
+        HttpClient client = mockHttpClient(HttpStatus.SC_PARTIAL_CONTENT, "HelloWorld");
+        doReturn(client).when(action).buildHttpClient();
+
+        File destinationFile = mock(File.class);
+        doReturn(destinationFile).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+
+        doReturn(true).when(action).verify(eq(temporaryFile), anyString());
+        doNothing().when(action).extract(eq(temporaryFile), eq(destinationFile), anyString());
+
+        action.perform(RuntimeEnvironment.application, catalog);
+
+        ArgumentCaptor<HttpGet> argument = ArgumentCaptor.forClass(HttpGet.class);
+        verify(client).execute(argument.capture());
+
+        HttpGet request = argument.getValue();
+        Assert.assertTrue(request.containsHeader("Range"));
+        Assert.assertEquals("bytes=1337-", request.getFirstHeader("Range").getValue());
+        Assert.assertEquals("HelloWorld", new String(outputStream.toByteArray(), "UTF-8"));
+
+        verify(action).openFile(eq(temporaryFile), eq(true));
+        verify(catalog).markAsDownloaded(content);
+        verify(temporaryFile).delete();
+    }
+
+    /**
+     * Scenario: Download fails with IOException.
+     *
+     * Verify that:
+     *  * Partially downloaded file will not be deleted
+     *  * Content will not be marked as downloaded in catalog
+     */
+    @Test
+    public void testTemporaryFileIsNotDeletedAfterDownloadAborted() throws Exception {
+        DownloadContent content = new DownloadContent.Builder()
+                .setKind(DownloadContent.KIND_FONT)
+                .setType(DownloadContent.TYPE_ASSET_ARCHIVE)
+                .setSize(4223)
+                .build();
+
+        DownloadContentCatalog catalog = mock(DownloadContentCatalog.class);
+        doReturn(Collections.singletonList(content)).when(catalog).getScheduledDownloads();
+
+        DownloadAction action = spy(new DownloadAction(null));
+        doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+
+        File temporaryFile = mockTemporaryFile(true, 1337L);
+        doReturn(temporaryFile).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
+
+        ByteArrayOutputStream outputStream = spy(new ByteArrayOutputStream());
+        doReturn(outputStream).when(action).openFile(eq(temporaryFile), anyBoolean());
+        doThrow(IOException.class).when(outputStream).write(any(byte[].class), anyInt(), anyInt());
+
+        HttpClient client = mockHttpClient(HttpStatus.SC_PARTIAL_CONTENT, "HelloWorld");
+        doReturn(client).when(action).buildHttpClient();
+
+        File destinationFile = mock(File.class);
+        doReturn(destinationFile).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+
+        action.perform(RuntimeEnvironment.application, catalog);
+
+        verify(catalog, never()).markAsDownloaded(content);
+        verify(temporaryFile, never()).delete();
+    }
+
+    /**
+     * Scenario: Partially downloaded file is already complete.
+     *
+     * Verify that:
+     *  * No download request is made
+     *  * File is treated as completed and will be verified and extracted
+     *  * Content is marked as downloaded in catalog
+     */
+    @Test
+    public void testNoRequestIsSentIfFileIsAlreadyComplete() throws Exception {
+        DownloadContent content = new DownloadContent.Builder()
+                .setKind(DownloadContent.KIND_FONT)
+                .setType(DownloadContent.TYPE_ASSET_ARCHIVE)
+                .setSize(1337L)
+                .build();
+
+        DownloadContentCatalog catalog = mock(DownloadContentCatalog.class);
+        doReturn(Collections.singletonList(content)).when(catalog).getScheduledDownloads();
+
+        DownloadAction action = spy(new DownloadAction(null));
+        doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+
+        File temporaryFile = mockTemporaryFile(true, 1337L);
+        doReturn(temporaryFile).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
+
+        File destinationFile = mock(File.class);
+        doReturn(destinationFile).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+
+        doReturn(true).when(action).verify(eq(temporaryFile), anyString());
+        doNothing().when(action).extract(eq(temporaryFile), eq(destinationFile), anyString());
+
+        action.perform(RuntimeEnvironment.application, catalog);
+
+        verify(action, never()).download(any(HttpClient.class), anyString(), eq(temporaryFile));
+        verify(action).verify(eq(temporaryFile), anyString());
+        verify(action).extract(eq(temporaryFile), eq(destinationFile), anyString());
+        verify(catalog).markAsDownloaded(content);
+    }
+
+    /**
+     * Scenario: Download is completed but verification (checksum) failed.
+     *
+     * Verify that:
+     *  * Downloaded file is deleted
+     *  * File will not be extracted
+     *  * Content is not marked as downloaded in the catalog
+     */
+    @Test
+    public void testTemporaryFileWillBeDeletedIfVerificationFails() throws Exception {
+        DownloadContent content = new DownloadContent.Builder()
+                .setKind(DownloadContent.KIND_FONT)
+                .setType(DownloadContent.TYPE_ASSET_ARCHIVE)
+                .setSize(1337L)
+                .build();
+
+        DownloadContentCatalog catalog = mock(DownloadContentCatalog.class);
+        doReturn(Collections.singletonList(content)).when(catalog).getScheduledDownloads();
+
+        DownloadAction action = spy(new DownloadAction(null));
+        doReturn(false).when(action).isActiveNetworkMetered(RuntimeEnvironment.application);
+        doNothing().when(action).download(any(HttpClient.class), anyString(), any(File.class));
+        doReturn(false).when(action).verify(any(File.class), anyString());
+
+        File temporaryFile = mockTemporaryFile(true, 1337L);
+        doReturn(temporaryFile).when(action).createTemporaryFile(RuntimeEnvironment.application, content);
+
+        File destinationFile = mock(File.class);
+        doReturn(destinationFile).when(action).getDestinationFile(RuntimeEnvironment.application, content);
+
+        action.perform(RuntimeEnvironment.application, catalog);
+
+        verify(temporaryFile).delete();
+        verify(action, never()).extract(any(File.class), any(File.class), anyString());
+        verify(catalog, never()).markAsDownloaded(content);
+    }
+
+    private static File mockTemporaryFile(boolean exists, long length) {
+        File temporaryFile = mock(File.class);
+        doReturn(exists).when(temporaryFile).exists();
+        doReturn(length).when(temporaryFile).length();
+        return temporaryFile;
+    }
+
+    private static HttpClient mockHttpClient(int statusCode, String content) throws Exception {
+        StatusLine status = mock(StatusLine.class);
+        doReturn(statusCode).when(status).getStatusCode();
+
+        HttpEntity entity = mock(HttpEntity.class);
+        doReturn(new ByteArrayInputStream(content.getBytes("UTF-8"))).when(entity).getContent();
+
+        HttpResponse response = mock(HttpResponse.class);
+        doReturn(status).when(response).getStatusLine();
+        doReturn(entity).when(response).getEntity();
+
+        HttpClient client = mock(HttpClient.class);
+        doReturn(response).when(client).execute(any(HttpUriRequest.class));
+
+        return client;
+    }
 }