Merge pull request square#2479 from square/jwilson_0412_nuanced_conne…

…ct_recovery

Further constrain when we recover from failed routes.

okhttp-tests/src/test/java/okhttp3/CallTest.java

@@ -27,6 +27,7 @@
 import java.net.ProtocolException;
 import java.net.Proxy;
 import java.net.ServerSocket;
+import java.net.SocketTimeoutException;
 import java.net.UnknownHostException;
 import java.net.UnknownServiceException;
 import java.security.cert.Certificate;
@@ -761,10 +762,36 @@ private void postBodyRetransmittedAfterAuthorizationFail(String body) throws Exc
   }
 
   /**
-   * Make a request with two routes. The first route will time out because it's connecting via a
-   * null proxy server. The second will succeed.
+   * Make a request with two routes. The first route will time out because it's connecting to a
+   * special address that never connects. The automatic retry will succeed.
    */
   @Test public void connectTimeoutsAttemptsAlternateRoute() throws Exception {
+    InetSocketAddress unreachableAddress = new InetSocketAddress("10.255.255.1", 8080);
+
+    RecordingProxySelector proxySelector = new RecordingProxySelector();
+    proxySelector.proxies.add(new Proxy(Proxy.Type.HTTP, unreachableAddress));
+    proxySelector.proxies.add(server.toProxyAddress());
+
+    server.enqueue(new MockResponse()
+        .setBody("success!"));
+
+    client = client.newBuilder()
+        .proxySelector(proxySelector)
+        .readTimeout(100, TimeUnit.MILLISECONDS)
+        .connectTimeout(100, TimeUnit.MILLISECONDS)
+        .build();
+
+    Request request = new Request.Builder().url("http://android.com/").build();
+    executeSynchronously(request)
+        .assertCode(200)
+        .assertBody("success!");
+  }
+
+  /**
+   * Make a request with two routes. The first route will fail because the null server connects but
+   * never responds. The manual retry will succeed.
+   */
+  @Test public void readTimeoutFails() throws Exception {
     InetSocketAddress nullServerAddress = startNullServer();
 
     RecordingProxySelector proxySelector = new RecordingProxySelector();
@@ -780,6 +807,8 @@ private void postBodyRetransmittedAfterAuthorizationFail(String body) throws Exc
         .build();
 
     Request request = new Request.Builder().url("http://android.com/").build();
+    executeSynchronously(request)
+        .assertFailure(SocketTimeoutException.class);
     executeSynchronously(request)
         .assertCode(200)
         .assertBody("success!");

okhttp-urlconnection/src/main/java/okhttp3/internal/huc/HttpURLConnectionImpl.java

@@ -490,7 +490,7 @@ private boolean execute(boolean readResponse) throws IOException {
       throw toThrow;
     } catch (RouteException e) {
       // The attempt to connect via a route failed. The request will not have been sent.
-      HttpEngine retryEngine = httpEngine.recover(e.getLastConnectException());
+      HttpEngine retryEngine = httpEngine.recover(e.getLastConnectException(), true);
       if (retryEngine != null) {
         releaseConnection = false;
         httpEngine = retryEngine;
@@ -503,7 +503,7 @@ private boolean execute(boolean readResponse) throws IOException {
       throw toThrow;
     } catch (IOException e) {
       // An attempt to communicate with a server failed. The request may have been sent.
-      HttpEngine retryEngine = httpEngine.recover(e);
+      HttpEngine retryEngine = httpEngine.recover(e, false);
       if (retryEngine != null) {
         releaseConnection = false;
         httpEngine = retryEngine;

okhttp/src/main/java/okhttp3/RealCall.java

@@ -245,7 +245,7 @@ Response getResponse(Request request, boolean forWebSocket) throws IOException {
         throw e.getCause();
       } catch (RouteException e) {
         // The attempt to connect via a route failed. The request will not have been sent.
-        HttpEngine retryEngine = engine.recover(e.getLastConnectException(), null);
+        HttpEngine retryEngine = engine.recover(e.getLastConnectException(), true, null);
         if (retryEngine != null) {
           releaseConnection = false;
           engine = retryEngine;
@@ -255,7 +255,7 @@ Response getResponse(Request request, boolean forWebSocket) throws IOException {
         throw e.getLastConnectException();
       } catch (IOException e) {
         // An attempt to communicate with a server failed. The request may have been sent.
-        HttpEngine retryEngine = engine.recover(e, null);
+        HttpEngine retryEngine = engine.recover(e, false, null);
         if (retryEngine != null) {
           releaseConnection = false;
           engine = retryEngine;

okhttp/src/main/java/okhttp3/internal/http/HttpEngine.java

@@ -185,7 +185,7 @@ public HttpEngine(OkHttpClient client, Request request, boolean bufferRequestBod
    * @throws RouteException if the was a problem during connection via a specific route. Sometimes
    * recoverable. See {@link #recover}.
    * @throws IOException if there was a problem while making a request. Sometimes recoverable. See
-   * {@link #recover(IOException)}.
+   * {@link #recover(IOException, boolean)}.
    */
   public void sendRequest() throws RequestException, RouteException, IOException {
     if (cacheStrategy != null) return; // Already sent.
@@ -349,8 +349,8 @@ public Connection getConnection() {
    * engine that should be used for the retry if {@code e} is recoverable, or null if the failure is
    * permanent. Requests with a body can only be recovered if the body is buffered.
    */
-  public HttpEngine recover(IOException e, Sink requestBodyOut) {
-    if (!streamAllocation.recover(e, requestBodyOut)) {
+  public HttpEngine recover(IOException e, boolean routeException, Sink requestBodyOut) {
+    if (!streamAllocation.recover(e, routeException, requestBodyOut)) {
       return null;
     }
 
@@ -365,8 +365,8 @@ public HttpEngine recover(IOException e, Sink requestBodyOut) {
         forWebSocket, streamAllocation, (RetryableSink) requestBodyOut, priorResponse);
   }
 
-  public HttpEngine recover(IOException e) {
-    return recover(e, requestBodyOut);
+  public HttpEngine recover(IOException e, boolean routeException) {
+    return recover(e, routeException, requestBodyOut);
   }
 
   private void maybeCache() throws IOException {

okhttp/src/main/java/okhttp3/internal/http/StreamAllocation.java

@@ -309,31 +309,31 @@ private void release(RealConnection connection) {
     throw new IllegalStateException();
   }
 
-  public boolean recover(IOException e, Sink requestBodyOut) {
+  public boolean recover(IOException e, boolean routeException, Sink requestBodyOut) {
     if (connection != null) {
       connectionFailed(e);
     }
 
     boolean canRetryRequestBody = requestBodyOut == null || requestBodyOut instanceof RetryableSink;
     if ((routeSelector != null && !routeSelector.hasNext()) // No more routes to attempt.
-        || !isRecoverable(e)
+        || !isRecoverable(e, routeException)
         || !canRetryRequestBody) {
       return false;
     }
 
     return true;
   }
 
-  private boolean isRecoverable(IOException e) {
+  private boolean isRecoverable(IOException e, boolean routeException) {
     // If there was a protocol problem, don't recover.
     if (e instanceof ProtocolException) {
       return false;
     }
 
-    // If there was an interruption don't recover, but if there was a timeout
+    // If there was an interruption don't recover, but if there was a timeout connecting to a route
     // we should try the next route (if there is one).
     if (e instanceof InterruptedIOException) {
-      return e instanceof SocketTimeoutException;
+      return e instanceof SocketTimeoutException && routeException;
     }
 
     // Look for known client-side or negotiation errors that are unlikely to be fixed by trying