Better handling of retryable errors.
Jeff Sharkey [Fri, 18 Jan 2013 01:26:51 +0000 (17:26 -0800)]
Now the final errors are always thrown, and the outer code decides
how to handle them as retries.  Also clean up method signatures.

Bug: 8022478
Change-Id: I4e7e43be793294ab837370df521e7c381e0bb6c3

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

index 8d80618..08ef466 100644 (file)
@@ -48,9 +48,6 @@ public class Constants {
     /** The column that is used to remember whether the media scanner was invoked */
     public static final String MEDIA_SCANNED = "scanned";
 
-    /** The column that is used to count retries */
-    public static final String FAILED_CONNECTIONS = "numfailed";
-
     /** The intent that gets sent when the service must wake up for a retry */
     public static final String ACTION_RETRY = "android.intent.action.DOWNLOAD_WAKEUP";
 
index 10e27a7..b984bde 100644 (file)
@@ -75,7 +75,7 @@ public class DownloadInfo {
             info.mDestination = getInt(Downloads.Impl.COLUMN_DESTINATION);
             info.mVisibility = getInt(Downloads.Impl.COLUMN_VISIBILITY);
             info.mStatus = getInt(Downloads.Impl.COLUMN_STATUS);
-            info.mNumFailed = getInt(Constants.FAILED_CONNECTIONS);
+            info.mNumFailed = getInt(Downloads.Impl.COLUMN_FAILED_CONNECTIONS);
             int retryRedirect = getInt(Constants.RETRY_AFTER_X_REDIRECT_COUNT);
             info.mRetryAfter = retryRedirect & 0xfffffff;
             info.mLastMod = getLong(Downloads.Impl.COLUMN_LAST_MODIFICATION);
index 7af8173..19a4763 100644 (file)
@@ -384,7 +384,7 @@ public final class DownloadProvider extends ContentProvider {
                         Downloads.Impl.COLUMN_VISIBILITY + " INTEGER, " +
                         Downloads.Impl.COLUMN_CONTROL + " INTEGER, " +
                         Downloads.Impl.COLUMN_STATUS + " INTEGER, " +
-                        Constants.FAILED_CONNECTIONS + " INTEGER, " +
+                        Downloads.Impl.COLUMN_FAILED_CONNECTIONS + " INTEGER, " +
                         Downloads.Impl.COLUMN_LAST_MODIFICATION + " BIGINT, " +
                         Downloads.Impl.COLUMN_NOTIFICATION_PACKAGE + " TEXT, " +
                         Downloads.Impl.COLUMN_NOTIFICATION_CLASS + " TEXT, " +
index 1518311..5bb1e9b 100644 (file)
@@ -110,7 +110,6 @@ public class DownloadThread extends Thread {
     static class State {
         public String mFilename;
         public String mMimeType;
-        public boolean mCountRetry = false;
         public int mRetryAfter = 0;
         public boolean mGotData = false;
         public String mRequestUri;
@@ -177,6 +176,7 @@ public class DownloadThread extends Thread {
         State state = new State(mInfo);
         PowerManager.WakeLock wakeLock = null;
         int finalStatus = Downloads.Impl.STATUS_UNKNOWN_ERROR;
+        int numFailed = mInfo.mNumFailed;
         String errorMsg = null;
 
         final NetworkPolicyManager netPolicy = NetworkPolicyManager.from(mContext);
@@ -219,6 +219,24 @@ public class DownloadThread extends Thread {
                 Log.w(Constants.TAG, msg, error);
             }
             finalStatus = error.getFinalStatus();
+
+            if (finalStatus == STATUS_WAITING_TO_RETRY) {
+                throw new IllegalStateException("Execution should always throw final error codes");
+            }
+
+            // Some errors should be retryable later, unless we fail too many times.
+            if (isStatusRetryable(finalStatus)) {
+                if (state.mGotData) {
+                    numFailed = 1;
+                } else {
+                    numFailed += 1;
+                }
+
+                if (numFailed < Constants.MAX_RETRIES) {
+                    finalStatus = STATUS_WAITING_TO_RETRY;
+                }
+            }
+
             // fall through to finally block
         } catch (Throwable ex) {
             errorMsg = ex.getMessage();
@@ -231,8 +249,7 @@ public class DownloadThread extends Thread {
             TrafficStats.clearThreadStatsUid();
 
             cleanupDestination(state, finalStatus);
-            notifyDownloadCompleted(finalStatus, state.mCountRetry, state.mRetryAfter,
-                    state.mGotData, state.mFilename, state.mMimeType, errorMsg);
+            notifyDownloadCompleted(state, finalStatus, errorMsg, numFailed);
 
             netPolicy.unregisterListener(mPolicyListener);
 
@@ -313,14 +330,12 @@ public class DownloadThread extends Thread {
 
                     case HTTP_UNAVAILABLE:
                         parseRetryAfterHeaders(state, conn);
-                        if (mInfo.mNumFailed < Constants.MAX_RETRIES) {
-                            throw new StopRequestException(STATUS_WAITING_TO_RETRY, "Unavailable");
-                        } else {
-                            throw new StopRequestException(STATUS_CANNOT_RESUME, "Unavailable");
-                        }
+                        throw new StopRequestException(
+                                HTTP_UNAVAILABLE, conn.getResponseMessage());
 
                     case HTTP_INTERNAL_ERROR:
-                        throw new StopRequestException(STATUS_WAITING_TO_RETRY, "Internal error");
+                        throw new StopRequestException(
+                                HTTP_INTERNAL_ERROR, conn.getResponseMessage());
 
                     default:
                         StopRequestException.throwUnhandledHttpError(
@@ -328,7 +343,7 @@ public class DownloadThread extends Thread {
                 }
             } catch (IOException e) {
                 // Trouble with low-level sockets
-                throw new StopRequestException(STATUS_WAITING_TO_RETRY, e);
+                throw new StopRequestException(STATUS_HTTP_DATA_ERROR, e);
 
             } finally {
                 if (conn != null) conn.disconnect();
@@ -569,10 +584,10 @@ public class DownloadThread extends Thread {
                 && (state.mCurrentBytes != state.mContentLength);
         if (lengthMismatched) {
             if (cannotResume(state)) {
-                throw new StopRequestException(Downloads.Impl.STATUS_CANNOT_RESUME,
+                throw new StopRequestException(STATUS_CANNOT_RESUME,
                         "mismatched content length; unable to resume");
             } else {
-                throw new StopRequestException(getFinalStatusForHttpError(state),
+                throw new StopRequestException(STATUS_HTTP_DATA_ERROR,
                         "closed socket before end of file");
             }
         }
@@ -603,10 +618,10 @@ public class DownloadThread extends Thread {
             values.put(Downloads.Impl.COLUMN_CURRENT_BYTES, state.mCurrentBytes);
             mContext.getContentResolver().update(mInfo.getAllDownloadsUri(), values, null, null);
             if (cannotResume(state)) {
-                throw new StopRequestException(Downloads.Impl.STATUS_CANNOT_RESUME,
+                throw new StopRequestException(STATUS_CANNOT_RESUME,
                         "Failed reading response: " + ex + "; unable to resume", ex);
             } else {
-                throw new StopRequestException(getFinalStatusForHttpError(state),
+                throw new StopRequestException(STATUS_HTTP_DATA_ERROR,
                         "Failed reading response: " + ex, ex);
             }
         }
@@ -683,13 +698,12 @@ public class DownloadThread extends Thread {
         final boolean noSizeInfo = state.mContentLength == -1
                 && (transferEncoding == null || !transferEncoding.equalsIgnoreCase("chunked"));
         if (!mInfo.mNoIntegrity && noSizeInfo) {
-            throw new StopRequestException(Downloads.Impl.STATUS_HTTP_DATA_ERROR,
+            throw new StopRequestException(STATUS_CANNOT_RESUME,
                     "can't know size of download, giving up");
         }
     }
 
     private void parseRetryAfterHeaders(State state, HttpURLConnection conn) {
-        state.mCountRetry = true;
         state.mRetryAfter = conn.getHeaderFieldInt("Retry-After", -1);
         if (state.mRetryAfter < 0) {
             state.mRetryAfter = 0;
@@ -704,25 +718,6 @@ public class DownloadThread extends Thread {
         }
     }
 
-    private int getFinalStatusForHttpError(State state) {
-        final NetworkState networkUsable = mInfo.checkCanUseNetwork();
-        if (networkUsable != NetworkState.OK) {
-            switch (networkUsable) {
-                case UNUSABLE_DUE_TO_SIZE:
-                case RECOMMENDED_UNUSABLE_DUE_TO_SIZE:
-                    return Downloads.Impl.STATUS_QUEUED_FOR_WIFI;
-                default:
-                    return Downloads.Impl.STATUS_WAITING_FOR_NETWORK;
-            }
-        } else if (mInfo.mNumFailed < Constants.MAX_RETRIES) {
-            state.mCountRetry = true;
-            return Downloads.Impl.STATUS_WAITING_TO_RETRY;
-        } else {
-            Log.w(Constants.TAG, "reached max retries for " + mInfo.mId);
-            return Downloads.Impl.STATUS_HTTP_DATA_ERROR;
-        }
-    }
-
     /**
      * Prepare the destination file to receive data.  If the file already exists, we'll set up
      * appropriately for resumption.
@@ -814,30 +809,24 @@ public class DownloadThread extends Thread {
     /**
      * Stores information about the completed download, and notifies the initiating application.
      */
-    private void notifyDownloadCompleted(int status, boolean countRetry, int retryAfter,
-            boolean gotData, String filename, String mimeType, String errorMsg) {
-        notifyThroughDatabase(
-                status, countRetry, retryAfter, gotData, filename, mimeType, errorMsg);
-        if (Downloads.Impl.isStatusCompleted(status)) {
+    private void notifyDownloadCompleted(
+            State state, int finalStatus, String errorMsg, int numFailed) {
+        notifyThroughDatabase(state, finalStatus, errorMsg, numFailed);
+        if (Downloads.Impl.isStatusCompleted(finalStatus)) {
             mInfo.sendIntentIfRequested();
         }
     }
 
-    private void notifyThroughDatabase(int status, boolean countRetry, int retryAfter,
-            boolean gotData, String filename, String mimeType, String errorMsg) {
+    private void notifyThroughDatabase(
+            State state, int finalStatus, String errorMsg, int numFailed) {
         ContentValues values = new ContentValues();
-        values.put(Downloads.Impl.COLUMN_STATUS, status);
-        values.put(Downloads.Impl._DATA, filename);
-        values.put(Downloads.Impl.COLUMN_MIME_TYPE, mimeType);
+        values.put(Downloads.Impl.COLUMN_STATUS, finalStatus);
+        values.put(Downloads.Impl._DATA, state.mFilename);
+        values.put(Downloads.Impl.COLUMN_MIME_TYPE, state.mMimeType);
         values.put(Downloads.Impl.COLUMN_LAST_MODIFICATION, mSystemFacade.currentTimeMillis());
-        values.put(Constants.RETRY_AFTER_X_REDIRECT_COUNT, retryAfter);
-        if (!countRetry) {
-            values.put(Constants.FAILED_CONNECTIONS, 0);
-        } else if (gotData) {
-            values.put(Constants.FAILED_CONNECTIONS, 1);
-        } else {
-            values.put(Constants.FAILED_CONNECTIONS, mInfo.mNumFailed + 1);
-        }
+        values.put(Downloads.Impl.COLUMN_FAILED_CONNECTIONS, numFailed);
+        values.put(Constants.RETRY_AFTER_X_REDIRECT_COUNT, state.mRetryAfter);
+
         // save the error message. could be useful to developers.
         if (!TextUtils.isEmpty(errorMsg)) {
             values.put(Downloads.Impl.COLUMN_ERROR_MSG, errorMsg);
@@ -874,4 +863,19 @@ public class DownloadThread extends Thread {
             return defaultValue;
         }
     }
+
+    /**
+     * Return if given status is eligible to be treated as
+     * {@link android.provider.Downloads.Impl#STATUS_WAITING_TO_RETRY}.
+     */
+    public static boolean isStatusRetryable(int status) {
+        switch (status) {
+            case STATUS_HTTP_DATA_ERROR:
+            case HTTP_UNAVAILABLE:
+            case HTTP_INTERNAL_ERROR:
+                return true;
+            default:
+                return false;
+        }
+    }
 }
index 157cce8..09739c0 100644 (file)
@@ -398,7 +398,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest {
 
     public void testNoContentLength() throws Exception {
         enqueueResponse(buildEmptyResponse(HTTP_OK).removeHeader("Content-length"));
-        runSimpleFailureTest(DownloadManager.ERROR_HTTP_DATA_ERROR);
+        runSimpleFailureTest(DownloadManager.ERROR_CANNOT_RESUME);
     }
 
     public void testInsufficientSpace() throws Exception {