From 46a0852c202acf68cd890867a35f8e51b81c2948 Mon Sep 17 00:00:00 2001 From: jwilson Date: Thu, 16 Jan 2014 00:46:09 -0500 Subject: [PATCH] Don't attempt TLS modes we know will fail. https://github.com/square/okhttp/issues/442 --- .../okhttp/internal/spdy/ByteArrayPoolTest.java | 2 +- okhttp/src/main/java/com/squareup/okhttp/Route.java | 5 ----- .../main/java/com/squareup/okhttp/RouteDatabase.java | 10 +--------- .../squareup/okhttp/internal/http/RouteSelector.java | 12 +++++++++++- .../okhttp/internal/http/HttpOverSpdyTest.java | 6 ------ .../okhttp/internal/http/RouteSelectorTest.java | 7 +++++-- 6 files changed, 18 insertions(+), 24 deletions(-) diff --git a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/ByteArrayPoolTest.java b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/ByteArrayPoolTest.java index 23df68407..30132d5c2 100644 --- a/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/ByteArrayPoolTest.java +++ b/okhttp-protocols/src/test/java/com/squareup/okhttp/internal/spdy/ByteArrayPoolTest.java @@ -18,9 +18,9 @@ package com.squareup.okhttp.internal.spdy; import org.junit.Test; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; public class ByteArrayPoolTest { @Test public void testReusesBuffer() { diff --git a/okhttp/src/main/java/com/squareup/okhttp/Route.java b/okhttp/src/main/java/com/squareup/okhttp/Route.java index 08963ad86..a08a4699c 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/Route.java +++ b/okhttp/src/main/java/com/squareup/okhttp/Route.java @@ -75,11 +75,6 @@ public class Route { return modernTls; } - /** Returns a copy of this route with flipped TLS mode. */ - Route flipTlsMode() { - return new Route(address, proxy, inetSocketAddress, !modernTls); - } - @Override public boolean equals(Object obj) { if (obj instanceof Route) { Route other = (Route) obj; diff --git a/okhttp/src/main/java/com/squareup/okhttp/RouteDatabase.java b/okhttp/src/main/java/com/squareup/okhttp/RouteDatabase.java index 9cbeaa73f..4177c0fb1 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/RouteDatabase.java +++ b/okhttp/src/main/java/com/squareup/okhttp/RouteDatabase.java @@ -15,10 +15,8 @@ */ package com.squareup.okhttp; -import java.io.IOException; import java.util.LinkedHashSet; import java.util.Set; -import javax.net.ssl.SSLHandshakeException; /** * A blacklist of failed routes to avoid when creating a new connection to a @@ -31,14 +29,8 @@ public final class RouteDatabase { private final Set failedRoutes = new LinkedHashSet(); /** Records a failure connecting to {@code failedRoute}. */ - public synchronized void failed(Route failedRoute, IOException failure) { + public synchronized void failed(Route failedRoute) { failedRoutes.add(failedRoute); - - if (!(failure instanceof SSLHandshakeException)) { - // If the problem was not related to SSL then it will also fail with - // a different TLS mode therefore we can be proactive about it. - failedRoutes.add(failedRoute.flipTlsMode()); - } } /** Records success connecting to {@code failedRoute}. */ diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/RouteSelector.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/RouteSelector.java index 1055e4f09..3f1dc9a2d 100644 --- a/okhttp/src/main/java/com/squareup/okhttp/internal/http/RouteSelector.java +++ b/okhttp/src/main/java/com/squareup/okhttp/internal/http/RouteSelector.java @@ -33,6 +33,7 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.NoSuchElementException; +import javax.net.ssl.SSLHandshakeException; import static com.squareup.okhttp.internal.Util.getEffectivePort; @@ -148,7 +149,16 @@ public final class RouteSelector { proxySelector.connectFailed(uri, failedRoute.getProxy().address(), failure); } - routeDatabase.failed(failedRoute, failure); + routeDatabase.failed(failedRoute); + + // If the previously returned route's problem was not related to TLS, and + // the next route only changes the TLS mode, we shouldn't even attempt it. + // This suppresses it in both this selector and also in the route database. + if (hasNextTlsMode() && !(failure instanceof SSLHandshakeException)) { + boolean modernTls = nextTlsMode() == TLS_MODE_MODERN; + Route routeToSuppress = new Route(address, lastProxy, lastInetSocketAddress, modernTls); + routeDatabase.failed(routeToSuppress); + } } /** Resets {@link #nextProxy} to the first option. */ diff --git a/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java b/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java index 72e1461a9..e2994a9e3 100644 --- a/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java +++ b/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java @@ -15,7 +15,6 @@ */ package com.squareup.okhttp.internal.http; - import com.squareup.okhttp.HttpResponseCache; import com.squareup.okhttp.OkHttpClient; import com.squareup.okhttp.internal.RecordingAuthenticator; @@ -25,9 +24,7 @@ import com.squareup.okhttp.mockwebserver.MockResponse; import com.squareup.okhttp.mockwebserver.MockWebServer; import com.squareup.okhttp.mockwebserver.RecordedRequest; import com.squareup.okhttp.mockwebserver.SocketPolicy; - import java.io.ByteArrayOutputStream; -import java.io.DataInputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -46,9 +43,6 @@ import java.util.UUID; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.LinkedBlockingQueue; -import java.util.concurrent.ThreadPoolExecutor; -import java.util.concurrent.TimeUnit; import java.util.zip.GZIPOutputStream; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLContext; diff --git a/okhttp/src/test/java/com/squareup/okhttp/internal/http/RouteSelectorTest.java b/okhttp/src/test/java/com/squareup/okhttp/internal/http/RouteSelectorTest.java index 27812a910..bbb36d42a 100644 --- a/okhttp/src/test/java/com/squareup/okhttp/internal/http/RouteSelectorTest.java +++ b/okhttp/src/test/java/com/squareup/okhttp/internal/http/RouteSelectorTest.java @@ -107,7 +107,7 @@ public final class RouteSelectorTest { dns.inetAddresses = makeFakeAddresses(255, 1); Connection connection = routeSelector.next("GET"); RouteDatabase routeDatabase = new RouteDatabase(); - routeDatabase.failed(connection.getRoute(), new IOException()); + routeDatabase.failed(connection.getRoute()); routeSelector = new RouteSelector(address, uri, proxySelector, pool, dns, routeDatabase); assertConnection(routeSelector.next("GET"), address, NO_PROXY, dns.inetAddresses[0], uriPort, false); @@ -281,6 +281,7 @@ public final class RouteSelectorTest { assertFalse(routeSelector.hasNext()); } + // https://github.com/square/okhttp/issues/442 @Test public void nonSslErrorAddsAllTlsModesToFailedRoute() throws Exception { Address address = new Address(uriHost, uriPort, socketFactory, hostnameVerifier, authenticator, Proxy.NO_PROXY, transports); @@ -292,6 +293,7 @@ public final class RouteSelectorTest { Connection connection = routeSelector.next("GET"); routeSelector.connectFailed(connection, new IOException("Non SSL exception")); assertTrue(routeDatabase.failedRoutesCount() == 2); + assertFalse(routeSelector.hasNext()); } @Test public void sslErrorAddsOnlyFailedTlsModeToFailedRoute() throws Exception { @@ -305,6 +307,7 @@ public final class RouteSelectorTest { Connection connection = routeSelector.next("GET"); routeSelector.connectFailed(connection, new SSLHandshakeException("SSL exception")); assertTrue(routeDatabase.failedRoutesCount() == 1); + assertTrue(routeSelector.hasNext()); } @Test public void multipleProxiesMultipleInetAddressesMultipleTlsModes() throws Exception { @@ -372,7 +375,7 @@ public final class RouteSelectorTest { // Check that we do indeed have more than one route. assertTrue(regularRoutes.size() > 1); // Add first regular route as failed. - routeDatabase.failed(regularRoutes.get(0).getRoute(), new SSLHandshakeException("none")); + routeDatabase.failed(regularRoutes.get(0).getRoute()); // Reset selector routeSelector = new RouteSelector(address, uri, proxySelector, pool, dns, routeDatabase);