Improved support for 302/307 redirects.

Change the download manager's handling of 302/307 temporary redirects
so that after an interruption of any kind, the delayed retry/resume
will return to the original URI.  This complies better with the HTTP
spec.  This change also makes the download manager handle other
redirects immediately rather than using the delay that's otherwise
applied to download errors.

I made one more method extraction in DownloadThread to simplify this
change, pulling the top-level logic for a single request into
executeDownload().  It was then just a matter of introducing a new
RetryDownload exeception, similar to StopRequest, and making the
redirection code use it.

Change-Id: Ic719c5725a9fd2e5eebe4dc03453ee71d9f27cd4

2 files changed, 94 insertions, 69 deletions

diff --git a/src/com/android/providers/downloads/DownloadThread.java b/src/com/android/providers/downloads/DownloadThread.java
index 56894feb..736bbce5 100644
--- a/src/com/android/providers/downloads/DownloadThread.java
+++ b/src/com/android/providers/downloads/DownloadThread.java
@@ -88,16 +88,18 @@ public class DownloadThread extends Thread {
         public String mNewUri;
         public Uri mContentUri;
         public boolean mGotData = false;
+        public String mRequestUri;
 
         public State(DownloadInfo info) {
             mMimeType = sanitizeMimeType(info.mMimeType);
             mRedirectCount = info.mRedirectCount;
             mContentUri = Uri.parse(Downloads.Impl.CONTENT_URI + "/" + info.mId);
+            mRequestUri = info.mUri;
         }
     }
 
     /**
-     * State within the outer try block of the run() method.
+     * State within executeDownload()
      */
     private static class InnerState {
         public int mBytesSoFar = 0;
@@ -117,6 +119,12 @@ public class DownloadThread extends Thread {
     private class StopRequest extends Exception {}
 
     /**
+     * Raised from methods called by executeDownload() to indicate that the download should be
+     * retried immediately.
+     */
+    private class RetryDownload extends Exception {}
+
+    /**
      * Executes the download in a separate thread
      */
     public void run() {
@@ -125,12 +133,8 @@ public class DownloadThread extends Thread {
         State state = new State(mInfo);
         AndroidHttpClient client = null;
         PowerManager.WakeLock wakeLock = null;
-        HttpGet request = null;
 
         try {
-            InnerState innerState = new InnerState();
-            byte data[] = new byte[Constants.BUFFER_SIZE];
-
             PowerManager pm = (PowerManager) mContext.getSystemService(Context.POWER_SERVICE);
             wakeLock = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, Constants.TAG);
             wakeLock.acquire();
@@ -139,36 +143,29 @@ public class DownloadThread extends Thread {
             if (Constants.LOGV) {
                 Log.v(Constants.TAG, "initiating download for " + mInfo.mUri);
             }
-            setupDestinationFile(state, innerState);
-            client = AndroidHttpClient.newInstance(userAgent(), mContext);
-            request = new HttpGet(mInfo.mUri);
-            addRequestHeaders(innerState, request);
-
-            // check connectivity just before sending
-            if (!mInfo.canUseNetwork()) {
-                state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
-                return;
-            }
 
-            HttpResponse response = sendRequest(state, client, request);
-            handleExceptionalStatus(state, innerState, response);
+            client = AndroidHttpClient.newInstance(userAgent(), mContext);
 
-            if (Constants.LOGV) {
-                Log.v(Constants.TAG, "received response for " + mInfo.mUri);
+            boolean finished = false;
+            while(!finished) {
+                HttpGet request = new HttpGet(state.mRequestUri);
+                try {
+                    executeDownload(state, client, request);
+                    finished = true;
+                } catch (RetryDownload exc) {
+                    // fall through
+                } finally {
+                    request.abort();
+                    request = null;
+                }
             }
 
-            processResponseHeaders(state, innerState, response);
-            InputStream entityStream = openResponseEntity(state, response);
-            transferData(state, innerState, data, entityStream);
-
             if (Constants.LOGV) {
                 Log.v(Constants.TAG, "download completed for " + mInfo.mUri);
             }
             state.mFinalStatus = Downloads.Impl.STATUS_SUCCESS;
         } catch (StopRequest error) {
-            if (request != null) {
-                request.abort();
-            }
+            // fall through to finally block
         } catch (FileNotFoundException ex) {
             Log.d(Constants.TAG, "FileNotFoundException for " + state.mFilename + " : " +  ex);
             state.mFinalStatus = Downloads.Impl.STATUS_FILE_ERROR;
@@ -200,6 +197,43 @@ public class DownloadThread extends Thread {
     }
 
     /**
+     * Fully execute a single download request - setup and send the request, handle the response,
+     * and transfer the data to the destination file.
+     */
+    private void executeDownload(State state, AndroidHttpClient client, HttpGet request)
+            throws StopRequest, RetryDownload, FileNotFoundException {
+        InnerState innerState = new InnerState();
+        byte data[] = new byte[Constants.BUFFER_SIZE];
+
+        setupDestinationFile(state, innerState);
+        addRequestHeaders(innerState, request);
+
+        // check just before sending the request to avoid using an invalid connection at all
+        checkConnectivity(state);
+
+        HttpResponse response = sendRequest(state, client, request);
+        handleExceptionalStatus(state, innerState, response);
+
+        if (Constants.LOGV) {
+            Log.v(Constants.TAG, "received response for " + mInfo.mUri);
+        }
+
+        processResponseHeaders(state, innerState, response);
+        InputStream entityStream = openResponseEntity(state, response);
+        transferData(state, innerState, data, entityStream);
+    }
+
+    /**
+     * Check if current connectivity is valid for this request.
+     */
+    private void checkConnectivity(State state) throws StopRequest {
+        if (!mInfo.canUseNetwork()) {
+            state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
+            throw new StopRequest();
+        }
+    }
+
+    /**
      * Transfer as much data as possible from the HTTP response to the destination file.
      * @param data buffer to use to read data
      * @param entityStream stream for reading the HTTP response entity
@@ -548,12 +582,8 @@ public class DownloadThread extends Thread {
         }
 
         updateDatabaseFromHeaders(state, innerState);
-
         // check connectivity again now that we know the total size
-        if (!mInfo.canUseNetwork()) {
-            state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
-            throw new StopRequest();
-        }
+        checkConnectivity(state);
     }
 
     /**
@@ -638,7 +668,7 @@ public class DownloadThread extends Thread {
      * Check the HTTP response status and handle anything unusual (e.g. not 200/206).
      */
     private void handleExceptionalStatus(State state, InnerState innerState, HttpResponse response)
-            throws StopRequest {
+            throws StopRequest, RetryDownload {
         int statusCode = response.getStatusLine().getStatusCode();
         if (statusCode == 503 && mInfo.mNumFailed < Constants.MAX_RETRIES) {
             handleServiceUnavailable(state, response);
@@ -680,7 +710,7 @@ public class DownloadThread extends Thread {
      * Handle a 3xx redirect status.
      */
     private void handleRedirect(State state, HttpResponse response, int statusCode)
-            throws StopRequest {
+            throws StopRequest, RetryDownload {
         if (Constants.LOGVV) {
             Log.v(Constants.TAG, "got HTTP redirect " + statusCode);
         }
@@ -695,33 +725,35 @@ public class DownloadThread extends Thread {
             throw new StopRequest();
         }
         Header header = response.getFirstHeader("Location");
-        if (header != null) {
-            if (Constants.LOGVV) {
-                Log.v(Constants.TAG, "Location :" + header.getValue());
-            }
-            try {
-                state.mNewUri = new URI(mInfo.mUri).
-                        resolve(new URI(header.getValue())).
-                        toString();
-            } catch(URISyntaxException ex) {
-                if (Constants.LOGV) {
-                    Log.d(Constants.TAG,
-                            "Couldn't resolve redirect URI " +
-                            header.getValue() +
-                            " for " +
-                            mInfo.mUri);
-                } else if (Config.LOGD) {
-                    Log.d(Constants.TAG,
-                            "Couldn't resolve redirect URI for download " +
-                            mInfo.mId);
-                }
-                state.mFinalStatus = Downloads.Impl.STATUS_BAD_REQUEST;
-                throw new StopRequest();
+        if (header == null) {
+            return;
+        }
+        if (Constants.LOGVV) {
+            Log.v(Constants.TAG, "Location :" + header.getValue());
+        }
+
+        String newUri;
+        try {
+            newUri = new URI(mInfo.mUri).resolve(new URI(header.getValue())).toString();
+        } catch(URISyntaxException ex) {
+            if (Constants.LOGV) {
+                Log.d(Constants.TAG, "Couldn't resolve redirect URI " + header.getValue()
+                        + " for " + mInfo.mUri);
+            } else if (Config.LOGD) {
+                Log.d(Constants.TAG,
+                        "Couldn't resolve redirect URI for download " +
+                        mInfo.mId);
             }
-            ++state.mRedirectCount;
-            state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
+            state.mFinalStatus = Downloads.Impl.STATUS_BAD_REQUEST;
             throw new StopRequest();
         }
+        ++state.mRedirectCount;
+        state.mRequestUri = newUri;
+        if (statusCode == 301 || statusCode == 303) {
+            // use the new URI for all future requests (should a retry/resume be necessary)
+            state.mNewUri = newUri;
+        }
+        throw new RetryDownload();
     }
 
     /**
@@ -824,10 +856,7 @@ public class DownloadThread extends Thread {
                     state.mFilename = null;
                 } else if (mInfo.mETag == null && !mInfo.mNoIntegrity) {
                     // Tough luck, that's not a resumable download
-                    if (Config.LOGD) {
-                        Log.d(Constants.TAG,
-                                "can't resume interrupted non-resumable download");
-                    }
+                    Log.d(Constants.TAG, "can't resume interrupted non-resumable download");
                     f.delete();
                     state.mFinalStatus = Downloads.Impl.STATUS_PRECONDITION_FAILED;
                     throw new StopRequest();
diff --git a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
index b02844fa..c1a015af 100644
--- a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
+++ b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
@@ -293,14 +293,13 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
 
     public void testRedirect301() throws Exception {
         RecordedRequest lastRequest = runRedirectionTest(301);
-        // for 301, upon retry, we reuse the redirected URI
+        // for 301, upon retry/resume, we reuse the redirected URI
         assertEquals(REDIRECTED_PATH, lastRequest.getPath());
     }
 
-    // TODO: currently fails
-    public void disabledTestRedirect302() throws Exception {
+    public void testRedirect302() throws Exception {
         RecordedRequest lastRequest = runRedirectionTest(302);
-        // for 302, upon retry, we use the original URI
+        // for 302, upon retry/resume, we use the original URI
         assertEquals(REQUEST_PATH, lastRequest.getPath());
     }
 
@@ -485,11 +484,8 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         enqueueInterruptedDownloadResponses(5);
 
         Download download = enqueueRequest(getRequest());
-        download.runUntilStatus(DownloadManager.STATUS_PAUSED);
+        runService();
         assertEquals(REQUEST_PATH, takeRequest().getPath());
-
-        mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS);
-        download.runUntilStatus(DownloadManager.STATUS_PAUSED);
         assertEquals(REDIRECTED_PATH, takeRequest().getPath());
 
         mSystemFacade.incrementTimeMillis(RETRY_DELAY_MILLIS);