Clean up error codes returned by download manager.
Steve Howard [Sat, 31 Jul 2010 01:55:38 +0000 (18:55 -0700)]
This set of changes cleans up the error codes returned by the download
manager in various failure cases, aiming for improved consistency.
Error codes are part of the public API so it's important to get this
right now.

The main changes here are:
* Refactoring the flow of error status information throughout
  DownloadThread to make it more explicit, by having StopRequest
  accept a status code in its constructor and eliminating
  State.mFinaStatus.
* Eliminating the use of valid HTTP 4xx statuses when those statuses
  weren't actually returned by the server.  Now, if the returned error
  code is a valid HTTP status code, that means it was returned by the
  server.  These cases have been replaced with more sensible
  artificial error codes, including a new ERROR_CANNOT_RESUME when an
  interrupted download can't be resumed.
* Improvements to some of the error handling code paths -- ensuring we
  don't clear the cache for external downloads, ensuring we don't fail
  with CANNOT_RESUME when the download hasn't actually started yet,
  removing the restriction on acceptable mime types for public API
  downloads.

Change-Id: I0d825845fe0fe7ed5df74bad26e8d34ac0d1cc4e

src/com/android/providers/downloads/DownloadInfo.java
src/com/android/providers/downloads/DownloadThread.java
src/com/android/providers/downloads/Helpers.java
tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java

index 4c08b15..92d8d50 100644 (file)
@@ -371,4 +371,10 @@ public class DownloadInfo {
         mHasActiveThread = true;
         mSystemFacade.startThread(downloader);
     }
+
+    public boolean isOnCache() {
+        return (mDestination == Downloads.Impl.DESTINATION_CACHE_PARTITION
+                || mDestination == Downloads.Impl.DESTINATION_CACHE_PARTITION_NOROAMING
+                || mDestination == Downloads.Impl.DESTINATION_CACHE_PARTITION_PURGEABLE);
+    }
 }
index be02e3e..f0aed3f 100644 (file)
@@ -79,7 +79,6 @@ public class DownloadThread extends Thread {
      */
     private static class State {
         public String mFilename;
-        public int mFinalStatus = Downloads.Impl.STATUS_UNKNOWN_ERROR;
         public FileOutputStream mStream;
         public String mMimeType;
         public boolean mCountRetry = false;
@@ -95,6 +94,7 @@ public class DownloadThread extends Thread {
             mRedirectCount = info.mRedirectCount;
             mContentUri = Uri.parse(Downloads.Impl.CONTENT_URI + "/" + info.mId);
             mRequestUri = info.mUri;
+            mFilename = info.mFileName;
         }
     }
 
@@ -116,11 +116,16 @@ public class DownloadThread extends Thread {
      * Raised from methods called by run() to indicate that the current request should be stopped
      * immediately.
      */
-    private class StopRequest extends Exception {
-        public StopRequest() {}
+    private class StopRequest extends Throwable {
+        public int mFinalStatus;
 
-        public StopRequest(Throwable throwable) {
+        public StopRequest(int finalStatus) {
+            mFinalStatus = finalStatus;
+        }
+
+        public StopRequest(int finalStatus, Throwable throwable) {
             super(throwable);
+            mFinalStatus = finalStatus;
         }
     }
 
@@ -128,7 +133,7 @@ public class DownloadThread extends Thread {
      * Raised from methods called by executeDownload() to indicate that the download should be
      * retried immediately.
      */
-    private class RetryDownload extends Exception {}
+    private class RetryDownload extends Throwable {}
 
     /**
      * Executes the download in a separate thread
@@ -139,6 +144,7 @@ public class DownloadThread extends Thread {
         State state = new State(mInfo);
         AndroidHttpClient client = null;
         PowerManager.WakeLock wakeLock = null;
+        int finalStatus = Downloads.Impl.STATUS_UNKNOWN_ERROR;
 
         try {
             PowerManager pm = (PowerManager) mContext.getSystemService(Context.POWER_SERVICE);
@@ -169,15 +175,17 @@ public class DownloadThread extends Thread {
             if (Constants.LOGV) {
                 Log.v(Constants.TAG, "download completed for " + mInfo.mUri);
             }
-            state.mFinalStatus = Downloads.Impl.STATUS_SUCCESS;
+            finalizeDestinationFile(state);
+            finalStatus = Downloads.Impl.STATUS_SUCCESS;
         } catch (StopRequest error) {
             if (Constants.LOGV) {
                 Log.v(Constants.TAG, "Aborting request for " + mInfo.mUri, error);
             }
+            finalStatus = error.mFinalStatus;
             // fall through to finally block
         } catch (FileNotFoundException ex) {
             Log.d(Constants.TAG, "FileNotFoundException for " + state.mFilename + " : " +  ex);
-            state.mFinalStatus = Downloads.Impl.STATUS_FILE_ERROR;
+            finalStatus = Downloads.Impl.STATUS_FILE_ERROR;
             // falls through to the code that reports an error
         } catch (RuntimeException ex) { //sometimes the socket code throws unchecked exceptions
             if (Constants.LOGV) {
@@ -185,7 +193,7 @@ public class DownloadThread extends Thread {
             } else if (Config.LOGD) {
                 Log.d(Constants.TAG, "Exception for id " + mInfo.mId, ex);
             }
-            state.mFinalStatus = Downloads.Impl.STATUS_UNKNOWN_ERROR;
+            finalStatus = Downloads.Impl.STATUS_UNKNOWN_ERROR;
             // falls through to the code that reports an error
         } finally {
             mInfo.mHasActiveThread = false;
@@ -197,9 +205,8 @@ public class DownloadThread extends Thread {
                 client.close();
                 client = null;
             }
-            closeDestination(state);
-            finalizeDestinationFile(state);
-            notifyDownloadCompleted(state.mFinalStatus, state.mCountRetry, state.mRetryAfter,
+            cleanupDestination(state, finalStatus);
+            notifyDownloadCompleted(finalStatus, state.mCountRetry, state.mRetryAfter,
                                     state.mRedirectCount, state.mGotData, state.mFilename,
                                     state.mNewUri, state.mMimeType);
         }
@@ -237,8 +244,7 @@ public class DownloadThread extends Thread {
      */
     private void checkConnectivity(State state) throws StopRequest {
         if (!mInfo.canUseNetwork()) {
-            state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
-            throw new StopRequest();
+            throw new StopRequest(Downloads.Impl.STATUS_RUNNING_PAUSED);
         }
     }
 
@@ -271,33 +277,28 @@ public class DownloadThread extends Thread {
     }
 
     /**
-     * Called after a download transfer has just completed to take any necessary action on the
-     * downloaded file.
+     * Called after a successful completion to take any necessary action on the downloaded file.
      */
-    private void finalizeDestinationFile(State state) {
-        if (state.mFilename == null) {
-            return;
+    private void finalizeDestinationFile(State state) throws StopRequest {
+        if (isDrmFile(state)) {
+            transferToDrm(state);
+        } else {
+            // make sure the file is readable
+            FileUtils.setPermissions(state.mFilename, 0644, -1, -1);
+            syncDestination(state);
         }
+    }
 
-        if (Downloads.Impl.isStatusError(state.mFinalStatus)) {
+    /**
+     * Called just before the thread finishes, regardless of status, to take any necessary action on
+     * the downloaded file.
+     */
+    private void cleanupDestination(State state, int finalStatus) {
+        closeDestination(state);
+        if (state.mFilename != null && Downloads.Impl.isStatusError(finalStatus)) {
             new File(state.mFilename).delete();
             state.mFilename = null;
-            return;
-        }
-
-        if (!Downloads.Impl.isStatusSuccess(state.mFinalStatus)) {
-            // not yet complete
-            return;
-        }
-
-        if (isDrmFile(state)) {
-            transferToDrm(state);
-            return;
         }
-
-        // make sure the file is readable
-        FileUtils.setPermissions(state.mFilename, 0644, -1, -1);
-        syncDestination(state);
     }
 
     /**
@@ -339,18 +340,18 @@ public class DownloadThread extends Thread {
     /**
      * Transfer the downloaded destination file to the DRM store.
      */
-    private void transferToDrm(State state) {
+    private void transferToDrm(State state) throws StopRequest {
         File file = new File(state.mFilename);
         Intent item = DrmStore.addDrmFile(mContext.getContentResolver(), file, null);
+        file.delete();
+
         if (item == null) {
             Log.w(Constants.TAG, "unable to add file " + state.mFilename + " to DrmProvider");
-            state.mFinalStatus = Downloads.Impl.STATUS_UNKNOWN_ERROR;
+            throw new StopRequest(Downloads.Impl.STATUS_UNKNOWN_ERROR);
         } else {
             state.mFilename = item.getDataString();
             state.mMimeType = item.getType();
         }
-
-        file.delete();
     }
 
     /**
@@ -381,16 +382,14 @@ public class DownloadThread extends Thread {
                 if (Constants.LOGV) {
                     Log.v(Constants.TAG, "paused " + mInfo.mUri);
                 }
-                state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
-                throw new StopRequest();
+                throw new StopRequest(Downloads.Impl.STATUS_RUNNING_PAUSED);
             }
         }
         if (mInfo.mStatus == Downloads.Impl.STATUS_CANCELED) {
             if (Constants.LOGV) {
                 Log.d(Constants.TAG, "canceled " + mInfo.mUri);
             }
-            state.mFinalStatus = Downloads.Impl.STATUS_CANCELED;
-            throw new StopRequest();
+            throw new StopRequest(Downloads.Impl.STATUS_CANCELED);
         }
     }
 
@@ -431,10 +430,11 @@ public class DownloadThread extends Thread {
                 }
                 return;
             } catch (IOException ex) {
-                if (!Helpers.discardPurgeableFiles(mContext, Constants.BUFFER_SIZE)) {
-                    state.mFinalStatus = Downloads.Impl.STATUS_FILE_ERROR;
-                    throw new StopRequest(ex);
+                if (mInfo.isOnCache()
+                        && Helpers.discardPurgeableFiles(mContext, Constants.BUFFER_SIZE)) {
+                    continue;
                 }
+                throw new StopRequest(Downloads.Impl.STATUS_FILE_ERROR, ex);
             }
         }
     }
@@ -454,7 +454,7 @@ public class DownloadThread extends Thread {
         boolean lengthMismatched = (innerState.mHeaderContentLength != null)
                 && (innerState.mBytesSoFar != Integer.parseInt(innerState.mHeaderContentLength));
         if (lengthMismatched) {
-            if (!mInfo.mNoIntegrity && innerState.mHeaderETag == null) {
+            if (cannotResume(innerState)) {
                 if (Constants.LOGV) {
                     Log.d(Constants.TAG, "mismatched content length " +
                             mInfo.mUri);
@@ -462,14 +462,17 @@ public class DownloadThread extends Thread {
                     Log.d(Constants.TAG, "mismatched content length for " +
                             mInfo.mId);
                 }
-                state.mFinalStatus = Downloads.Impl.STATUS_LENGTH_REQUIRED;
+                throw new StopRequest(Downloads.Impl.STATUS_CANNOT_RESUME);
             } else {
-                handleHttpError(state, "closed socket");
+                throw new StopRequest(handleHttpError(state, "closed socket"));
             }
-            throw new StopRequest();
         }
     }
 
+    private boolean cannotResume(InnerState innerState) {
+        return innerState.mBytesSoFar > 0 && !mInfo.mNoIntegrity && innerState.mHeaderETag == null;
+    }
+
     /**
      * Read some data from the HTTP response stream, handling I/O errors.
      * @param data buffer to use to read data
@@ -485,14 +488,13 @@ public class DownloadThread extends Thread {
             ContentValues values = new ContentValues();
             values.put(Downloads.Impl.COLUMN_CURRENT_BYTES, innerState.mBytesSoFar);
             mContext.getContentResolver().update(state.mContentUri, values, null, null);
-            if (!mInfo.mNoIntegrity && innerState.mHeaderETag == null) {
+            if (cannotResume(innerState)) {
                 Log.d(Constants.TAG, "download IOException for download " + mInfo.mId, ex);
                 Log.d(Constants.TAG, "can't resume interrupted download with no ETag");
-                state.mFinalStatus = Downloads.Impl.STATUS_PRECONDITION_FAILED;
+                throw new StopRequest(Downloads.Impl.STATUS_CANNOT_RESUME, ex);
             } else {
-                handleHttpError(state, "download IOException");
+                throw new StopRequest(handleHttpError(state, "download IOException"), ex);
             }
-            throw new StopRequest(ex);
         }
     }
 
@@ -506,8 +508,7 @@ public class DownloadThread extends Thread {
             return response.getEntity().getContent();
         } catch (IOException ex) {
             logNetworkState();
-            handleHttpError(state, "IOException getting entity");
-            throw new StopRequest(ex);
+            throw new StopRequest(handleHttpError(state, "IOException getting entity"), ex);
         }
     }
 
@@ -540,10 +541,10 @@ public class DownloadThread extends Thread {
                 state.mMimeType,
                 mInfo.mDestination,
                 (innerState.mHeaderContentLength != null) ?
-                        Long.parseLong(innerState.mHeaderContentLength) : 0);
+                        Long.parseLong(innerState.mHeaderContentLength) : 0,
+                mInfo.mIsPublicApi);
         if (fileInfo.mFileName == null) {
-            state.mFinalStatus = fileInfo.mStatus;
-            throw new StopRequest();
+            throw new StopRequest(fileInfo.mStatus);
         }
         state.mFilename = fileInfo.mFileName;
         state.mStream = fileInfo.mStream;
@@ -629,8 +630,7 @@ public class DownloadThread extends Thread {
                     || !headerTransferEncoding.equalsIgnoreCase("chunked"));
         if (!mInfo.mNoIntegrity && noSizeInfo) {
             Log.d(Constants.TAG, "can't know size of download, giving up");
-            state.mFinalStatus = Downloads.Impl.STATUS_LENGTH_REQUIRED;
-            throw new StopRequest();
+            throw new StopRequest(Downloads.Impl.STATUS_HTTP_DATA_ERROR);
         }
     }
 
@@ -664,16 +664,17 @@ public class DownloadThread extends Thread {
             Log.d(Constants.TAG, "http error " + statusCode + " for download " +
                     mInfo.mId);
         }
+        int finalStatus;
         if (Downloads.Impl.isStatusError(statusCode)) {
-            state.mFinalStatus = statusCode;
+            finalStatus = statusCode;
         } else if (statusCode >= 300 && statusCode < 400) {
-            state.mFinalStatus = Downloads.Impl.STATUS_UNHANDLED_REDIRECT;
+            finalStatus = Downloads.Impl.STATUS_UNHANDLED_REDIRECT;
         } else if (innerState.mContinuingDownload && statusCode == Downloads.Impl.STATUS_SUCCESS) {
-            state.mFinalStatus = Downloads.Impl.STATUS_PRECONDITION_FAILED;
+            finalStatus = Downloads.Impl.STATUS_CANNOT_RESUME;
         } else {
-            state.mFinalStatus = Downloads.Impl.STATUS_UNHANDLED_HTTP_CODE;
+            finalStatus = Downloads.Impl.STATUS_UNHANDLED_HTTP_CODE;
         }
-        throw new StopRequest();
+        throw new StopRequest(finalStatus);
     }
 
     /**
@@ -691,8 +692,7 @@ public class DownloadThread extends Thread {
             } else if (Config.LOGD) {
                 Log.d(Constants.TAG, "too many redirects for download " + mInfo.mId);
             }
-            state.mFinalStatus = Downloads.Impl.STATUS_TOO_MANY_REDIRECTS;
-            throw new StopRequest();
+            throw new StopRequest(Downloads.Impl.STATUS_TOO_MANY_REDIRECTS);
         }
         Header header = response.getFirstHeader("Location");
         if (header == null) {
@@ -714,8 +714,7 @@ public class DownloadThread extends Thread {
                         "Couldn't resolve redirect URI for download " +
                         mInfo.mId);
             }
-            state.mFinalStatus = Downloads.Impl.STATUS_BAD_REQUEST;
-            throw new StopRequest();
+            throw new StopRequest(Downloads.Impl.STATUS_HTTP_DATA_ERROR);
         }
         ++state.mRedirectCount;
         state.mRequestUri = newUri;
@@ -733,7 +732,6 @@ public class DownloadThread extends Thread {
         if (Constants.LOGVV) {
             Log.v(Constants.TAG, "got HTTP response code 503");
         }
-        state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
         state.mCountRetry = true;
         Header header = response.getFirstHeader("Retry-After");
         if (header != null) {
@@ -757,7 +755,7 @@ public class DownloadThread extends Thread {
                // ignored - retryAfter stays 0 in this case.
            }
         }
-        throw new StopRequest();
+        throw new StopRequest(Downloads.Impl.STATUS_RUNNING_PAUSED);
     }
 
     /**
@@ -775,28 +773,30 @@ public class DownloadThread extends Thread {
                 Log.d(Constants.TAG, "Arg exception trying to execute request for " +
                         mInfo.mId + " : " +  ex);
             }
-            state.mFinalStatus = Downloads.Impl.STATUS_BAD_REQUEST;
-            throw new StopRequest(ex);
+            throw new StopRequest(Downloads.Impl.STATUS_HTTP_DATA_ERROR, ex);
         } catch (IOException ex) {
             logNetworkState();
-            handleHttpError(state, "IOException trying to execute request");
-            throw new StopRequest(ex);
+            throw new StopRequest(handleHttpError(state, "IOException trying to execute request"),
+                    ex);
         }
     }
 
-    private void handleHttpError(State state, String message) {
+    /**
+     * @return the final status for this attempt
+     */
+    private int handleHttpError(State state, String message) {
         if (Constants.LOGV) {
             Log.d(Constants.TAG, message + " for " + mInfo.mUri);
         }
 
         if (!Helpers.isNetworkAvailable(mSystemFacade)) {
-            state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
+            return Downloads.Impl.STATUS_RUNNING_PAUSED;
         } else if (mInfo.mNumFailed < Constants.MAX_RETRIES) {
-            state.mFinalStatus = Downloads.Impl.STATUS_RUNNING_PAUSED;
             state.mCountRetry = true;
+            return Downloads.Impl.STATUS_RUNNING_PAUSED;
         } else {
             Log.d(Constants.TAG, "reached max retries: " + message + " for " + mInfo.mId);
-            state.mFinalStatus = Downloads.Impl.STATUS_HTTP_DATA_ERROR;
+            return Downloads.Impl.STATUS_HTTP_DATA_ERROR;
         }
     }
 
@@ -806,11 +806,9 @@ public class DownloadThread extends Thread {
      */
     private void setupDestinationFile(State state, InnerState innerState)
             throws StopRequest, FileNotFoundException {
-        state.mFilename = mInfo.mFileName;
-        if (state.mFilename != null) {
+        if (state.mFilename != null) { // only true if we've already run a thread for this download
             if (!Helpers.isFilenameValid(state.mFilename)) {
-                state.mFinalStatus = Downloads.Impl.STATUS_FILE_ERROR;
-                throw new StopRequest();
+                throw new StopRequest(Downloads.Impl.STATUS_FILE_ERROR);
             }
             // We're resuming a download that got interrupted
             File f = new File(state.mFilename);
@@ -821,11 +819,10 @@ public class DownloadThread extends Thread {
                     f.delete();
                     state.mFilename = null;
                 } else if (mInfo.mETag == null && !mInfo.mNoIntegrity) {
-                    // Tough luck, that's not a resumable download
-                    Log.d(Constants.TAG, "can't resume interrupted non-resumable download");
+                    // This should've been caught upon failure
+                    Log.wtf(Constants.TAG, "Trying to resume a download that can't be resumed");
                     f.delete();
-                    state.mFinalStatus = Downloads.Impl.STATUS_PRECONDITION_FAILED;
-                    throw new StopRequest();
+                    throw new StopRequest(Downloads.Impl.STATUS_CANNOT_RESUME);
                 } else {
                     // All right, we'll be able to resume this download
                     state.mStream = new FileOutputStream(state.mFilename, true);
index f298895..5d546ff 100644 (file)
@@ -94,9 +94,10 @@ public class Helpers {
             String contentLocation,
             String mimeType,
             int destination,
-            long contentLength) throws FileNotFoundException {
+            long contentLength,
+            boolean isPublicApi) throws FileNotFoundException {
 
-        if (!canHandleDownload(context, mimeType, destination)) {
+        if (!canHandleDownload(context, mimeType, destination, isPublicApi)) {
             return new DownloadFileInfo(null, null, Downloads.Impl.STATUS_NOT_ACCEPTABLE);
         }
 
@@ -156,7 +157,12 @@ public class Helpers {
         return chooseUniqueFilename(destination, filename, extension, recoveryDir);
     }
 
-    private static boolean canHandleDownload(Context context, String mimeType, int destination) {
+    private static boolean canHandleDownload(Context context, String mimeType, int destination,
+            boolean isPublicApi) {
+        if (isPublicApi) {
+            return true;
+        }
+
         if (destination == Downloads.Impl.DESTINATION_EXTERNAL
                 || destination == Downloads.Impl.DESTINATION_CACHE_PARTITION_PURGEABLE) {
             if (mimeType == null) {
index 840b20a..6d60477 100644 (file)
@@ -35,8 +35,6 @@ import java.util.List;
 
 @LargeTest
 public class PublicApiFunctionalTest extends AbstractPublicApiTest {
-    private static final int HTTP_NOT_ACCEPTABLE = 406;
-    private static final int HTTP_LENGTH_REQUIRED = 411;
     private static final String REDIRECTED_PATH = "/other_path";
     private static final String ETAG = "my_etag";
 
@@ -305,7 +303,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
 
     public void testNoEtag() throws Exception {
         enqueuePartialResponse(0, 5).removeHeader("Etag");
-        runSimpleFailureTest(HTTP_LENGTH_REQUIRED);
+        runSimpleFailureTest(DownloadManager.ERROR_CANNOT_RESUME);
     }
 
     public void testSanitizeMediaType() throws Exception {
@@ -317,12 +315,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
 
     public void testNoContentLength() throws Exception {
         enqueueEmptyResponse(HTTP_OK).removeHeader("Content-Length");
-        runSimpleFailureTest(HTTP_LENGTH_REQUIRED);
-    }
-
-    public void testNoContentType() throws Exception {
-        enqueueResponse(HTTP_OK, "").removeHeader("Content-Type");
-        runSimpleFailureTest(HTTP_NOT_ACCEPTABLE);
+        runSimpleFailureTest(DownloadManager.ERROR_HTTP_DATA_ERROR);
     }
 
     public void testInsufficientSpace() throws Exception {
@@ -478,6 +471,17 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
         checkCompleteDownload(download);
     }
 
+    public void testExistingFile() throws Exception {
+        Uri destination = getExternalUri();
+        new File(destination.getSchemeSpecificPart()).createNewFile();
+
+        enqueueEmptyResponse(HTTP_OK);
+        Download download = enqueueRequest(getRequest().setDestinationUri(destination));
+        download.runUntilStatus(DownloadManager.STATUS_FAILED);
+        assertEquals(DownloadManager.ERROR_FILE_ERROR,
+                     download.getLongField(DownloadManager.COLUMN_ERROR_CODE));
+    }
+
     private void checkCompleteDownload(Download download) throws Exception {
         assertEquals(FILE_CONTENT.length(),
                      download.getLongField(DownloadManager.COLUMN_BYTES_DOWNLOADED_SO_FAR));