Merge "Retry more errors in Volley's BasicNetwork."android-wear-n-preview-3android-wear-n-preview-2android-wear-n-preview-1android-wear-7.1.1_r1android-n-preview-5android-n-preview-4android-n-preview-3android-n-preview-2android-n-iot-preview-2nougat-mr1-wear-releasen-iot-preview-2

5 files changed, 284 insertions, 14 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());
                 }
             }
         }
diff --git a/src/test/java/com/android/volley/mock/MockHttpStack.java b/src/test/java/com/android/volley/mock/MockHttpStack.java
index 9594fde..91872d3 100644
--- a/src/test/java/com/android/volley/mock/MockHttpStack.java
+++ b/src/test/java/com/android/volley/mock/MockHttpStack.java
@@ -22,6 +22,7 @@ import com.android.volley.toolbox.HttpStack;
 
 import org.apache.http.HttpResponse;
 
+import java.io.IOException;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -29,6 +30,8 @@ public class MockHttpStack implements HttpStack {
 
     private HttpResponse mResponseToReturn;
 
+    private IOException mExceptionToThrow;
+
     private String mLastUrl;
 
     private Map<String, String> mLastHeaders;
@@ -51,9 +54,16 @@ public class MockHttpStack implements HttpStack {
         mResponseToReturn = response;
     }
 
+    public void setExceptionToThrow(IOException exception) {
+        mExceptionToThrow = exception;
+    }
+
     @Override
     public HttpResponse performRequest(Request<?> request, Map<String, String> additionalHeaders)
-            throws AuthFailureError {
+            throws IOException, AuthFailureError {
+        if (mExceptionToThrow != null) {
+            throw mExceptionToThrow;
+        }
         mLastUrl = request.getUrl();
         mLastHeaders = new HashMap<String, String>();
         if (request.getHeaders() != null) {
diff --git a/src/test/java/com/android/volley/toolbox/BasicNetworkTest.java b/src/test/java/com/android/volley/toolbox/BasicNetworkTest.java
index 89718b1..c01d9b0 100644
--- a/src/test/java/com/android/volley/toolbox/BasicNetworkTest.java
+++ b/src/test/java/com/android/volley/toolbox/BasicNetworkTest.java
@@ -16,29 +16,46 @@
 
 package com.android.volley.toolbox;
 
+import com.android.volley.AuthFailureError;
 import com.android.volley.NetworkResponse;
 import com.android.volley.Request;
 import com.android.volley.Response;
+import com.android.volley.RetryPolicy;
+import com.android.volley.ServerError;
+import com.android.volley.TimeoutError;
+import com.android.volley.VolleyError;
 import com.android.volley.mock.MockHttpStack;
 
 import org.apache.http.ProtocolVersion;
+import org.apache.http.conn.ConnectTimeoutException;
 import org.apache.http.entity.StringEntity;
 import org.apache.http.message.BasicHttpResponse;
-
-import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.Mock;
 import org.robolectric.RobolectricTestRunner;
 
-import static org.junit.Assert.*;
-
+import java.io.IOException;
+import java.net.SocketTimeoutException;
 import java.util.HashMap;
 import java.util.Map;
 
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+import static org.mockito.MockitoAnnotations.initMocks;
+
 @RunWith(RobolectricTestRunner.class)
 public class BasicNetworkTest {
 
+    @Mock private Request<String> mMockRequest;
+    @Mock private RetryPolicy mMockRetryPolicy;
+    private BasicNetwork mNetwork;
+
+    @Before public void setUp() throws Exception {
+        initMocks(this);
+    }
+
     @Test public void headersAndPostParams() throws Exception {
         MockHttpStack mockHttpStack = new MockHttpStack();
         BasicHttpResponse fakeResponse = new BasicHttpResponse(new ProtocolVersion("HTTP", 1, 1),
@@ -46,7 +63,188 @@ public class BasicNetworkTest {
         fakeResponse.setEntity(new StringEntity("foobar"));
         mockHttpStack.setResponseToReturn(fakeResponse);
         BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
-        Request<String> request = new Request<String>(Request.Method.GET, "http://foo", null) {
+        Request<String> request = buildRequest();
+        httpNetwork.performRequest(request);
+        assertEquals("foo", mockHttpStack.getLastHeaders().get("requestheader"));
+        assertEquals("requestpost=foo&", new String(mockHttpStack.getLastPostBody()));
+    }
+
+    @Test public void socketTimeout() throws Exception {
+        MockHttpStack mockHttpStack = new MockHttpStack();
+        mockHttpStack.setExceptionToThrow(new SocketTimeoutException());
+        BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
+        Request<String> request = buildRequest();
+        request.setRetryPolicy(mMockRetryPolicy);
+        doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+        try {
+            httpNetwork.performRequest(request);
+        } catch (VolleyError e) {
+            // expected
+        }
+        // should retry socket timeouts
+        verify(mMockRetryPolicy).retry(any(TimeoutError.class));
+    }
+
+    @Test public void connectTimeout() throws Exception {
+        MockHttpStack mockHttpStack = new MockHttpStack();
+        mockHttpStack.setExceptionToThrow(new ConnectTimeoutException());
+        BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
+        Request<String> request = buildRequest();
+        request.setRetryPolicy(mMockRetryPolicy);
+        doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+        try {
+            httpNetwork.performRequest(request);
+        } catch (VolleyError e) {
+            // expected
+        }
+        // should retry connection timeouts
+        verify(mMockRetryPolicy).retry(any(TimeoutError.class));
+    }
+
+    @Test public void noConnection() throws Exception {
+        MockHttpStack mockHttpStack = new MockHttpStack();
+        mockHttpStack.setExceptionToThrow(new IOException());
+        BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
+        Request<String> request = buildRequest();
+        request.setRetryPolicy(mMockRetryPolicy);
+        doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+        try {
+            httpNetwork.performRequest(request);
+        } catch (VolleyError e) {
+            // expected
+        }
+        // should not retry when there is no connection
+        verify(mMockRetryPolicy, never()).retry(any(VolleyError.class));
+    }
+
+    @Test public void unauthorized() throws Exception {
+        MockHttpStack mockHttpStack = new MockHttpStack();
+        BasicHttpResponse fakeResponse = new BasicHttpResponse(new ProtocolVersion("HTTP", 1, 1),
+                401, "Unauthorized");
+        mockHttpStack.setResponseToReturn(fakeResponse);
+        BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
+        Request<String> request = buildRequest();
+        request.setRetryPolicy(mMockRetryPolicy);
+        doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+        try {
+            httpNetwork.performRequest(request);
+        } catch (VolleyError e) {
+            // expected
+        }
+        // should retry in case it's an auth failure.
+        verify(mMockRetryPolicy).retry(any(AuthFailureError.class));
+    }
+
+    @Test public void forbidden() throws Exception {
+        MockHttpStack mockHttpStack = new MockHttpStack();
+        BasicHttpResponse fakeResponse = new BasicHttpResponse(new ProtocolVersion("HTTP", 1, 1),
+                403, "Forbidden");
+        mockHttpStack.setResponseToReturn(fakeResponse);
+        BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
+        Request<String> request = buildRequest();
+        request.setRetryPolicy(mMockRetryPolicy);
+        doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+        try {
+            httpNetwork.performRequest(request);
+        } catch (VolleyError e) {
+            // expected
+        }
+        // should retry in case it's an auth failure.
+        verify(mMockRetryPolicy).retry(any(AuthFailureError.class));
+    }
+
+    @Test public void redirect() throws Exception {
+        for (int i = 300; i <= 399; i++) {
+            MockHttpStack mockHttpStack = new MockHttpStack();
+            BasicHttpResponse fakeResponse =
+                    new BasicHttpResponse(new ProtocolVersion("HTTP", 1, 1), i, "");
+            mockHttpStack.setResponseToReturn(fakeResponse);
+            BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
+            Request<String> request = buildRequest();
+            request.setRetryPolicy(mMockRetryPolicy);
+            doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+            try {
+                httpNetwork.performRequest(request);
+            } catch (VolleyError e) {
+                // expected
+            }
+            // should not retry 300 responses.
+            verify(mMockRetryPolicy, never()).retry(any(VolleyError.class));
+            reset(mMockRetryPolicy);
+        }
+    }
+
+    @Test public void otherClientError() throws Exception {
+        for (int i = 400; i <= 499; i++) {
+            if (i == 401 || i == 403) {
+                // covered above.
+                continue;
+            }
+            MockHttpStack mockHttpStack = new MockHttpStack();
+            BasicHttpResponse fakeResponse =
+                    new BasicHttpResponse(new ProtocolVersion("HTTP", 1, 1), i, "");
+            mockHttpStack.setResponseToReturn(fakeResponse);
+            BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
+            Request<String> request = buildRequest();
+            request.setRetryPolicy(mMockRetryPolicy);
+            doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+            try {
+                httpNetwork.performRequest(request);
+            } catch (VolleyError e) {
+                // expected
+            }
+            // should not retry other 400 errors.
+            verify(mMockRetryPolicy, never()).retry(any(VolleyError.class));
+            reset(mMockRetryPolicy);
+        }
+    }
+
+    @Test public void serverError_enableRetries() throws Exception {
+        for (int i = 500; i <= 599; i++) {
+            MockHttpStack mockHttpStack = new MockHttpStack();
+            BasicHttpResponse fakeResponse =
+                    new BasicHttpResponse(new ProtocolVersion("HTTP", 1, 1), i, "");
+            mockHttpStack.setResponseToReturn(fakeResponse);
+            BasicNetwork httpNetwork =
+                    new BasicNetwork(mockHttpStack, new ByteArrayPool(4096));
+            Request<String> request = buildRequest();
+            request.setRetryPolicy(mMockRetryPolicy);
+            request.setShouldRetryServerErrors(true);
+            doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+            try {
+                httpNetwork.performRequest(request);
+            } catch (VolleyError e) {
+                // expected
+            }
+            // should retry all 500 errors
+            verify(mMockRetryPolicy).retry(any(ServerError.class));
+            reset(mMockRetryPolicy);
+        }
+    }
+
+    @Test public void serverError_disableRetries() throws Exception {
+        for (int i = 500; i <= 599; i++) {
+            MockHttpStack mockHttpStack = new MockHttpStack();
+            BasicHttpResponse fakeResponse =
+                    new BasicHttpResponse(new ProtocolVersion("HTTP", 1, 1), i, "");
+            mockHttpStack.setResponseToReturn(fakeResponse);
+            BasicNetwork httpNetwork = new BasicNetwork(mockHttpStack);
+            Request<String> request = buildRequest();
+            request.setRetryPolicy(mMockRetryPolicy);
+            doThrow(new VolleyError()).when(mMockRetryPolicy).retry(any(VolleyError.class));
+            try {
+                httpNetwork.performRequest(request);
+            } catch (VolleyError e) {
+                // expected
+            }
+            // should not retry any 500 error w/ HTTP 500 retries turned off (the default).
+            verify(mMockRetryPolicy, never()).retry(any(VolleyError.class));
+            reset(mMockRetryPolicy);
+        }
+    }
+
+    private static Request<String> buildRequest() {
+        return new Request<String>(Request.Method.GET, "http://foo", null) {
 
             @Override
             protected Response<String> parseNetworkResponse(NetworkResponse response) {
@@ -71,8 +269,5 @@ public class BasicNetworkTest {
                 return result;
             }
         };
-        httpNetwork.performRequest(request);
-        assertEquals("foo", mockHttpStack.getLastHeaders().get("requestheader"));
-        assertEquals("requestpost=foo&", new String(mockHttpStack.getLastPostBody()));
     }
 }