Retry more errors in Volley's BasicNetwork.

Always retry I/O errors while reading the HTTP response entity.
Furthermore, if a client opts in, retry HTTP 500 errors indicating
something went wrong on the server.

This resolves a longstanding TODO to only throw a ServerError on 5xx
errors by adding a ClientError for 4xx errors. For backwards
compatibility, this extends ServerError.

Note that Volley already may retry a request that reached the server
if the connection times out, which means that lack of idempotency
shouldn't be a concern here if it wasn't already. But if we wanted to
be even safer, we could require clients to opt into the additional
retry cases, at the cost of a somewhat more polluted API.

Add unit tests for most failure scenarios.

Bug: 23152983
Change-Id: I92cf35c66ccf98a1682adf41654afeb8634911db

3 files changed, 70 insertions, 5 deletions

diff --git a/src/main/java/com/android/volley/ClientError.java b/src/main/java/com/android/volley/ClientError.java
new file mode 100644
index 0000000..a8c8141
--- /dev/null
+++ b/src/main/java/com/android/volley/ClientError.java
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.volley;
+
+/**
+ * Indicates that the server responded with an error response indicating that the client has erred.
+ *
+ * For backwards compatibility, extends ServerError which used to be thrown for all server errors,
+ * including 4xx error codes indicating a client error.
+ */
+@SuppressWarnings("serial")
+public class ClientError extends ServerError {
+    public ClientError(NetworkResponse networkResponse) {
+        super(networkResponse);
+    }
+
+    public ClientError() {
+        super();
+    }
+}
+
diff --git a/src/main/java/com/android/volley/Request.java b/src/main/java/com/android/volley/Request.java
index 5b42d1f..8200f6e 100644
--- a/src/main/java/com/android/volley/Request.java
+++ b/src/main/java/com/android/volley/Request.java
@@ -20,7 +20,6 @@ import android.net.TrafficStats;
 import android.net.Uri;
 import android.os.Handler;
 import android.os.Looper;
-import android.os.SystemClock;
 import android.text.TextUtils;
 
 import com.android.volley.VolleyLog.MarkerLog;
@@ -90,6 +89,9 @@ public abstract class Request<T> implements Comparable<Request<T>> {
     /** Whether or not a response has been delivered for this request yet. */
     private boolean mResponseDelivered = false;
 
+    /** Whether the request should be retried in the event of an HTTP 5xx (server) error. */
+    private boolean mShouldRetryServerErrors = false;
+
     /** The retry policy for this request. */
     private RetryPolicy mRetryPolicy;
 
@@ -474,6 +476,23 @@ public abstract class Request<T> implements Comparable<Request<T>> {
     }
 
     /**
+     * Sets whether or not the request should be retried in the event of an HTTP 5xx (server) error.
+     *
+     * @return This Request object to allow for chaining.
+     */
+    public final Request<?> setShouldRetryServerErrors(boolean shouldRetryServerErrors) {
+        mShouldRetryServerErrors = shouldRetryServerErrors;
+        return this;
+    }
+
+    /**
+     * Returns true if this request should be retried in the event of an HTTP 5xx (server) error.
+     */
+    public final boolean shouldRetryServerErrors() {
+        return mShouldRetryServerErrors;
+    }
+
+    /**
      * Priority values.  Requests will be processed from higher priorities to
      * lower priorities, in FIFO order.
      */
diff --git a/src/main/java/com/android/volley/toolbox/BasicNetwork.java b/src/main/java/com/android/volley/toolbox/BasicNetwork.java
index 4b1603b..37c35ec 100644
--- a/src/main/java/com/android/volley/toolbox/BasicNetwork.java
+++ b/src/main/java/com/android/volley/toolbox/BasicNetwork.java
@@ -21,6 +21,7 @@ import android.os.SystemClock;
 import com.android.volley.AuthFailureError;
 import com.android.volley.Cache;
 import com.android.volley.Cache.Entry;
+import com.android.volley.ClientError;
 import com.android.volley.Network;
 import com.android.volley.NetworkError;
 import com.android.volley.NetworkResponse;
@@ -143,14 +144,14 @@ public class BasicNetwork implements Network {
             } catch (MalformedURLException e) {
                 throw new RuntimeException("Bad URL " + request.getUrl(), e);
             } catch (IOException e) {
-                int statusCode = 0;
-                NetworkResponse networkResponse = null;
+                int statusCode;
                 if (httpResponse != null) {
                     statusCode = httpResponse.getStatusLine().getStatusCode();
                 } else {
                     throw new NoConnectionError(e);
                 }
                 VolleyLog.e("Unexpected response code %d for %s", statusCode, request.getUrl());
+                NetworkResponse networkResponse;
                 if (responseContents != null) {
                     networkResponse = new NetworkResponse(statusCode, responseContents,
                             responseHeaders, false, SystemClock.elapsedRealtime() - requestStart);
@@ -158,12 +159,22 @@ public class BasicNetwork implements Network {
                             statusCode == HttpStatus.SC_FORBIDDEN) {
                         attemptRetryOnException("auth",
                                 request, new AuthFailureError(networkResponse));
+                    } else if (statusCode >= 400 && statusCode <= 499) {
+                        // Don't retry other client errors.
+                        throw new ClientError(networkResponse);
+                    } else if (statusCode >= 500 && statusCode <= 599) {
+                        if (request.shouldRetryServerErrors()) {
+                            attemptRetryOnException("server",
+                                    request, new ServerError(networkResponse));
+                        } else {
+                            throw new ServerError(networkResponse);
+                        }
                     } else {
-                        // TODO: Only throw ServerError for 5xx status codes.
+                        // 3xx? No reason to retry.
                         throw new ServerError(networkResponse);
                     }
                 } else {
-                    throw new NetworkError(networkResponse);
+                    attemptRetryOnException("network", request, new NetworkError());
                 }
             }
         }