Tests for max retries/redirects, ETag switches.
Jeff Sharkey [Tue, 29 Jan 2013 22:48:46 +0000 (14:48 -0800)]
Verify that servers responding with many retries or redirects result
in failed download, instead of spinning out of control.  Test to
verify that changed ETag results in download failing.

Also fix handling of HTTP 301 to update Uri in database.

Change-Id: Iff2948d79961a245b7900117d107edaa356618c9

src/com/android/providers/downloads/DownloadThread.java
src/com/android/providers/downloads/Helpers.java
tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java
tests/src/com/android/providers/downloads/AbstractPublicApiTest.java
tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java
tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
tests/src/com/android/providers/downloads/ThreadingTest.java

index bd347e4..0d427fd 100644 (file)
@@ -65,7 +65,6 @@ import java.net.URL;
 import java.net.URLConnection;
 
 import libcore.io.IoUtils;
-import libcore.net.http.HttpEngine;
 
 /**
  * Task which executes a given {@link DownloadInfo}: making network requests,
@@ -275,9 +274,7 @@ public class DownloadThread implements Runnable {
             return;
         }
 
-        // TODO: compare mInfo.mNumFailed against Constants.MAX_RETRIES
-
-        while (state.mRedirectionCount++ < HttpEngine.MAX_REDIRECTS) {
+        while (state.mRedirectionCount++ < Constants.MAX_REDIRECTS) {
             // Open connection and follow any redirects until we have a useful
             // response with body.
             HttpURLConnection conn = null;
@@ -315,18 +312,16 @@ public class DownloadThread implements Runnable {
                     case HTTP_TEMP_REDIRECT:
                         final String location = conn.getHeaderField("Location");
                         state.mUrl = new URL(state.mUrl, location);
+                        if (responseCode == HTTP_MOVED_PERM) {
+                            // Push updated URL back to database
+                            state.mRequestUri = state.mUrl.toString();
+                        }
                         continue;
 
                     case HTTP_REQUESTED_RANGE_NOT_SATISFIABLE:
                         throw new StopRequestException(
                                 STATUS_CANNOT_RESUME, "Requested range not satisfiable");
 
-                    case HTTP_PRECON_FAILED:
-                        // TODO: probably means our etag precondition was
-                        // changed; flush and retry again
-                        StopRequestException.throwUnhandledHttpError(
-                                responseCode, conn.getResponseMessage());
-
                     case HTTP_UNAVAILABLE:
                         parseRetryAfterHeaders(state, conn);
                         throw new StopRequestException(
@@ -645,7 +640,7 @@ public class DownloadThread implements Runnable {
                 state.mMimeType,
                 mInfo.mDestination,
                 state.mContentLength,
-                mInfo.mIsPublicApi, mStorageManager);
+                mStorageManager);
 
         updateDatabaseFromHeaders(state);
         // check connectivity again now that we know the total size
@@ -826,6 +821,10 @@ public class DownloadThread implements Runnable {
         values.put(Downloads.Impl.COLUMN_FAILED_CONNECTIONS, numFailed);
         values.put(Constants.RETRY_AFTER_X_REDIRECT_COUNT, state.mRetryAfter);
 
+        if (!TextUtils.equals(mInfo.mUri, state.mRequestUri)) {
+            values.put(Downloads.Impl.COLUMN_URI, state.mRequestUri);
+        }
+
         // save the error message. could be useful to developers.
         if (!TextUtils.isEmpty(errorMsg)) {
             values.put(Downloads.Impl.COLUMN_ERROR_MSG, errorMsg);
index 059e970..5c34ebe 100644 (file)
@@ -77,7 +77,7 @@ public class Helpers {
             String mimeType,
             int destination,
             long contentLength,
-            boolean isPublicApi, StorageManager storageManager) throws StopRequestException {
+            StorageManager storageManager) throws StopRequestException {
         if (contentLength < 0) {
             contentLength = 0;
         }
index 48eb0b4..acfd473 100644 (file)
@@ -52,11 +52,9 @@ public abstract class AbstractDownloadProviderFunctionalTest extends
     protected static final String LOG_TAG = "DownloadProviderFunctionalTest";
     private static final String PROVIDER_AUTHORITY = "downloads";
     protected static final long RETRY_DELAY_MILLIS = 61 * 1000;
-    protected static final String FILE_CONTENT = "hello world hello world hello world hello world";
-    protected static final int HTTP_OK = 200;
-    protected static final int HTTP_PARTIAL_CONTENT = 206;
-    protected static final int HTTP_NOT_FOUND = 404;
-    protected static final int HTTP_SERVICE_UNAVAILABLE = 503;
+
+    protected static final String
+            FILE_CONTENT = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
 
     protected MockWebServer mServer;
     protected MockContentResolverWithNotify mResolver;
index e7f08c9..bff9333 100644 (file)
@@ -27,7 +27,6 @@ import android.os.ParcelFileDescriptor;
 import android.os.SystemClock;
 import android.util.Log;
 
-import java.io.FileInputStream;
 import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.net.UnknownHostException;
@@ -49,6 +48,10 @@ public abstract class AbstractPublicApiTest extends AbstractDownloadProviderFunc
             return (int) getLongField(DownloadManager.COLUMN_STATUS);
         }
 
+        public int getReason() {
+            return (int) getLongField(DownloadManager.COLUMN_REASON);
+        }
+
         public int getStatusIfExists() {
             Cursor cursor = mManager.query(new DownloadManager.Query().setFilterById(mId));
             try {
@@ -91,7 +94,8 @@ public abstract class AbstractPublicApiTest extends AbstractDownloadProviderFunc
             ParcelFileDescriptor downloadedFile = mManager.openDownloadedFile(mId);
             assertTrue("Invalid file descriptor: " + downloadedFile,
                        downloadedFile.getFileDescriptor().valid());
-            InputStream stream = new FileInputStream(downloadedFile.getFileDescriptor());
+            final InputStream stream = new ParcelFileDescriptor.AutoCloseInputStream(
+                    downloadedFile);
             try {
                 return readStream(stream);
             } finally {
index d524937..dbab203 100644 (file)
@@ -17,6 +17,7 @@
 package com.android.providers.downloads;
 
 import static android.text.format.DateUtils.SECOND_IN_MILLIS;
+import static java.net.HttpURLConnection.HTTP_OK;
 
 import android.content.ContentValues;
 import android.database.Cursor;
index 09739c0..84390aa 100644 (file)
@@ -16,6 +16,9 @@
 
 package com.android.providers.downloads;
 
+import static android.app.DownloadManager.STATUS_FAILED;
+import static android.app.DownloadManager.STATUS_PAUSED;
+import static android.text.format.DateUtils.SECOND_IN_MILLIS;
 import static com.google.testing.littlemock.LittleMock.anyInt;
 import static com.google.testing.littlemock.LittleMock.anyString;
 import static com.google.testing.littlemock.LittleMock.atLeastOnce;
@@ -23,6 +26,12 @@ import static com.google.testing.littlemock.LittleMock.isA;
 import static com.google.testing.littlemock.LittleMock.never;
 import static com.google.testing.littlemock.LittleMock.times;
 import static com.google.testing.littlemock.LittleMock.verify;
+import static java.net.HttpURLConnection.HTTP_MOVED_TEMP;
+import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
+import static java.net.HttpURLConnection.HTTP_OK;
+import static java.net.HttpURLConnection.HTTP_PARTIAL;
+import static java.net.HttpURLConnection.HTTP_PRECON_FAILED;
+import static java.net.HttpURLConnection.HTTP_UNAVAILABLE;
 
 import android.app.DownloadManager;
 import android.app.Notification;
@@ -33,6 +42,7 @@ import android.database.Cursor;
 import android.net.ConnectivityManager;
 import android.net.Uri;
 import android.os.Environment;
+import android.os.SystemClock;
 import android.provider.Downloads;
 import android.test.suitebuilder.annotation.LargeTest;
 
@@ -44,8 +54,8 @@ import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
-import java.net.MalformedURLException;
 import java.util.List;
+import java.util.concurrent.TimeoutException;
 
 @LargeTest
 public class PublicApiFunctionalTest extends AbstractPublicApiTest {
@@ -189,7 +199,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
     private MockResponse buildPartialResponse(int start, int end) {
         int totalLength = FILE_CONTENT.length();
         boolean isFirstResponse = (start == 0);
-        int status = isFirstResponse ? HTTP_OK : HTTP_PARTIAL_CONTENT;
+        int status = isFirstResponse ? HTTP_OK : HTTP_PARTIAL;
         MockResponse response = buildResponse(status, FILE_CONTENT.substring(start, end))
                 .setHeader("Content-length", totalLength)
                 .setHeader("Etag", ETAG);
@@ -383,11 +393,72 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         assertEquals(REQUEST_PATH, lastRequest.getPath());
     }
 
+    public void testRunawayRedirect() throws Exception {
+        for (int i = 0; i < 16; i++) {
+            enqueueResponse(buildEmptyResponse(HTTP_MOVED_TEMP)
+                    .setHeader("Location", mServer.getUrl("/" + i).toString()));
+        }
+
+        final Download download = enqueueRequest(getRequest());
+
+        // Ensure that we arrive at failed download, instead of spinning forever
+        download.runUntilStatus(DownloadManager.STATUS_FAILED);
+        assertEquals(DownloadManager.ERROR_TOO_MANY_REDIRECTS, download.getReason());
+    }
+
+    public void testRunawayUnavailable() throws Exception {
+        final int RETRY_DELAY = 120;
+        for (int i = 0; i < 16; i++) {
+            enqueueResponse(
+                    buildEmptyResponse(HTTP_UNAVAILABLE).setHeader("Retry-after", RETRY_DELAY));
+        }
+
+        final Download download = enqueueRequest(getRequest());
+        for (int i = 0; i < Constants.MAX_RETRIES - 1; i++) {
+            download.runUntilStatus(DownloadManager.STATUS_PAUSED);
+            mSystemFacade.incrementTimeMillis((RETRY_DELAY + 60) * SECOND_IN_MILLIS);
+        }
+
+        // Ensure that we arrive at failed download, instead of spinning forever
+        download.runUntilStatus(DownloadManager.STATUS_FAILED);
+    }
+
     public void testNoEtag() throws Exception {
         enqueueResponse(buildPartialResponse(0, 5).removeHeader("Etag"));
         runSimpleFailureTest(DownloadManager.ERROR_CANNOT_RESUME);
     }
 
+    public void testEtagChanged() throws Exception {
+        final String A = "kittenz";
+        final String B = "puppiez";
+
+        // 1. Try downloading A, but partial result
+        enqueueResponse(buildResponse(HTTP_OK, A.substring(0, 2))
+                .setHeader("Content-length", A.length())
+                .setHeader("Etag", A));
+
+        // 2. Try resuming A, but fail ETag check
+        enqueueResponse(buildEmptyResponse(HTTP_PRECON_FAILED));
+
+        final Download download = enqueueRequest(getRequest());
+        RecordedRequest req;
+
+        // 1. Try downloading A, but partial result
+        download.runUntilStatus(STATUS_PAUSED);
+        assertEquals(DownloadManager.PAUSED_WAITING_TO_RETRY, download.getReason());
+        req = takeRequest();
+        assertNull(getHeaderValue(req, "Range"));
+        assertNull(getHeaderValue(req, "If-Match"));
+
+        // 2. Try resuming A, but fail ETag check
+        mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS);
+        download.runUntilStatus(STATUS_FAILED);
+        assertEquals(HTTP_PRECON_FAILED, download.getReason());
+        req = takeRequest();
+        assertEquals("bytes=2-", getHeaderValue(req, "Range"));
+        assertEquals(A, getHeaderValue(req, "If-Match"));
+    }
+
     public void testSanitizeMediaType() throws Exception {
         enqueueResponse(buildEmptyResponse(HTTP_OK)
                 .setHeader("Content-Type", "text/html; charset=ISO-8859-4"));
@@ -420,8 +491,14 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         assertTrue(rslt);
         mManager.remove(download.mId);
 
-        // make sure the row is gone from the database
-        download.waitForStatus(-1, -1);
+        // Verify that row is removed from database
+        final long timeout = SystemClock.elapsedRealtime() + (15 * SECOND_IN_MILLIS);
+        while (download.getStatusIfExists() != -1) {
+            if (SystemClock.elapsedRealtime() > timeout) {
+                throw new TimeoutException("Row wasn't removed");
+            }
+            SystemClock.sleep(100);
+        }
     }
 
     public void testDownloadCompleteBroadcast() throws Exception {
@@ -550,7 +627,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
     public void testRetryAfter() throws Exception {
         final int delay = 120;
         enqueueResponse(
-                buildEmptyResponse(HTTP_SERVICE_UNAVAILABLE).setHeader("Retry-after", delay));
+                buildEmptyResponse(HTTP_UNAVAILABLE).setHeader("Retry-after", delay));
         enqueueResponse(buildEmptyResponse(HTTP_OK));
 
         Download download = enqueueRequest(getRequest());
@@ -634,8 +711,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
      * 3) Resume request to complete download
      * @return the last request sent to the server, resuming after the interruption
      */
-    private RecordedRequest runRedirectionTest(int status)
-            throws MalformedURLException, Exception {
+    private RecordedRequest runRedirectionTest(int status) throws Exception {
         enqueueResponse(buildEmptyResponse(status)
                 .setHeader("Location", mServer.getUrl(REDIRECTED_PATH).toString()));
         enqueueInterruptedDownloadResponses(5);
@@ -650,4 +726,17 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
 
         return takeRequest();
     }
+
+    /**
+     * Return value of requested HTTP header, if it exists.
+     */
+    private static String getHeaderValue(RecordedRequest req, String header) {
+        header = header.toLowerCase() + ":";
+        for (String h : req.getHeaders()) {
+            if (h.toLowerCase().startsWith(header)) {
+                return h.substring(header.length()).trim();
+            }
+        }
+        return null;
+    }
 }
index e73bae2..fcb3329 100644 (file)
@@ -16,6 +16,8 @@
 
 package com.android.providers.downloads;
 
+import static java.net.HttpURLConnection.HTTP_OK;
+
 import android.app.DownloadManager;
 import android.test.suitebuilder.annotation.LargeTest;