Move connect to be closer to connection.

This eliminates some awkwardness in OkHttpClient, where it needs to make a bunch of internal calls to Connection.
peterkir · Jun 28, 2014 · 7bb06e7 · 7bb06e7
1 parent 2cf3885
commit 7bb06e7
Show file tree

Hide file tree

Showing 8 changed files with 250 additions and 266 deletions.
diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/OkHttpClientTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/OkHttpClientTest.java
@@ -108,14 +108,14 @@ public final class OkHttpClientTest {
 
     // Values should be non-null.
     OkHttpClient a = client.clone().copyWithDefaults();
-    assertNotNull(a.getRoutesDatabase());
+    assertNotNull(a.routeDatabase());
     assertNotNull(a.getDispatcher());
     assertNotNull(a.getConnectionPool());
     assertNotNull(a.getSslSocketFactory());
 
     // Multiple clients share the instances.
     OkHttpClient b = client.clone().copyWithDefaults();
-    assertSame(a.getRoutesDatabase(), b.getRoutesDatabase());
+    assertSame(a.routeDatabase(), b.routeDatabase());
     assertSame(a.getDispatcher(), b.getDispatcher());
     assertSame(a.getConnectionPool(), b.getConnectionPool());
     assertSame(a.getSslSocketFactory(), b.getSslSocketFactory());

diff --git a/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/RouteSelectorTest.java b/okhttp-tests/src/test/java/com/squareup/okhttp/internal/http/RouteSelectorTest.java
diff --git a/okhttp/src/main/java/com/squareup/okhttp/Address.java b/okhttp/src/main/java/com/squareup/okhttp/Address.java
@@ -17,7 +17,6 @@
 
 import com.squareup.okhttp.internal.Util;
 import java.net.Proxy;
-import java.net.UnknownHostException;
 import java.util.List;
 import javax.net.SocketFactory;
 import javax.net.ssl.HostnameVerifier;
@@ -47,8 +46,7 @@ public final class Address {
 
   public Address(String uriHost, int uriPort, SocketFactory socketFactory,
       SSLSocketFactory sslSocketFactory, HostnameVerifier hostnameVerifier,
-      Authenticator authenticator, Proxy proxy, List<Protocol> protocols)
-      throws UnknownHostException {
+      Authenticator authenticator, Proxy proxy, List<Protocol> protocols) {
     if (uriHost == null) throw new NullPointerException("uriHost == null");
     if (uriPort <= 0) throw new IllegalArgumentException("uriPort <= 0: " + uriPort);
     if (authenticator == null) throw new IllegalArgumentException("authenticator == null");

diff --git a/okhttp/src/main/java/com/squareup/okhttp/Connection.java b/okhttp/src/main/java/com/squareup/okhttp/Connection.java
@@ -30,6 +30,8 @@
 import java.net.URL;
 import javax.net.ssl.SSLSocket;
 
+import static com.squareup.okhttp.internal.Util.getDefaultPort;
+import static com.squareup.okhttp.internal.Util.getEffectivePort;
 import static java.net.HttpURLConnection.HTTP_OK;
 import static java.net.HttpURLConnection.HTTP_PROXY_AUTH;
 
@@ -155,6 +157,59 @@ void connect(int connectTimeout, int readTimeout, int writeTimeout, Request tunn
     connected = true;
   }
 
+  /**
+   * Connects this connection if it isn't already. This creates tunnels, shares
+   * the connection with the connection pool, and configures timeouts.
+   */
+  void connectAndSetOwner(OkHttpClient client, Object owner, Request request) throws IOException {
+    setOwner(owner);
+
+    if (!isConnected()) {
+      Request tunnelRequest = tunnelRequest(request);
+      connect(client.getConnectTimeout(), client.getReadTimeout(),
+          client.getWriteTimeout(), tunnelRequest);
+      if (isSpdy()) {
+        client.getConnectionPool().share(this);
+      }
+      client.routeDatabase().connected(getRoute());
+    }
+
+    setTimeouts(client.getReadTimeout(), client.getWriteTimeout());
+  }
+
+  /**
+   * Returns a request that creates a TLS tunnel via an HTTP proxy, or null if
+   * no tunnel is necessary. Everything in the tunnel request is sent
+   * unencrypted to the proxy server, so tunnels include only the minimum set of
+   * headers. This avoids sending potentially sensitive data like HTTP cookies
+   * to the proxy unencrypted.
+   */
+  private Request tunnelRequest(Request request) throws IOException {
+    if (!route.requiresTunnel()) return null;
+
+    String host = request.url().getHost();
+    int port = getEffectivePort(request.url());
+    String authority = (port == getDefaultPort("https")) ? host : (host + ":" + port);
+    Request.Builder result = new Request.Builder()
+        .url(new URL("https", host, port, "/"))
+        .header("Host", authority)
+        .header("Proxy-Connection", "Keep-Alive"); // For HTTP/1.0 proxies like Squid.
+
+    // Copy over the User-Agent header if it exists.
+    String userAgent = request.header("User-Agent");
+    if (userAgent != null) {
+      result.header("User-Agent", userAgent);
+    }
+
+    // Copy over the Proxy-Authorization header if it exists.
+    String proxyAuthorization = request.header("Proxy-Authorization");
+    if (proxyAuthorization != null) {
+      result.header("Proxy-Authorization", proxyAuthorization);
+    }
+
+    return result.build();
+  }
+
   /**
    * Create an {@code SSLSocket} and perform the TLS handshake and certificate
    * validation.

diff --git a/okhttp/src/main/java/com/squareup/okhttp/OkHttpClient.java b/okhttp/src/main/java/com/squareup/okhttp/OkHttpClient.java
@@ -67,10 +67,6 @@ public final class OkHttpClient implements Cloneable {
         return connection.recycleCount();
       }
 
-      @Override public Object getOwner(Connection connection) {
-        return connection.getOwner();
-      }
-
       @Override public void setProtocol(Connection connection, Protocol protocol) {
         connection.setProtocol(protocol);
       }
@@ -79,24 +75,6 @@ public final class OkHttpClient implements Cloneable {
         connection.setOwner(httpEngine);
       }
 
-      @Override public void connect(Connection connection, int connectTimeout, int readTimeout,
-          int writeTimeout, Request request) throws IOException {
-        connection.connect(connectTimeout, readTimeout, writeTimeout, request);
-      }
-
-      @Override public boolean isConnected(Connection connection) {
-        return connection.isConnected();
-      }
-
-      @Override public boolean isSpdy(Connection connection) {
-        return connection.isSpdy();
-      }
-
-      @Override public void setTimeouts(Connection connection, int readTimeout, int writeTimeout)
-          throws IOException {
-        connection.setTimeouts(readTimeout, writeTimeout);
-      }
-
       @Override public boolean isReadable(Connection pooled) {
         return pooled.isReadable();
       }
@@ -117,13 +95,14 @@ public final class OkHttpClient implements Cloneable {
         pool.recycle(connection);
       }
 
-      @Override public void share(ConnectionPool connectionPool, Connection connection) {
-        connectionPool.share(connection);
-      }
-
       @Override public RouteDatabase routeDatabase(OkHttpClient client) {
         return client.routeDatabase;
       }
+
+      @Override public void connectAndSetOwner(OkHttpClient client, Connection connection,
+          HttpEngine owner, Request request) throws IOException {
+        connection.connectAndSetOwner(client, owner, request);
+      }
     };
   }
 
@@ -385,7 +364,7 @@ public boolean getFollowRedirects() {
     return followRedirects;
   }
 
-  RouteDatabase getRoutesDatabase() {
+  RouteDatabase routeDatabase() {
     return routeDatabase;
   }
 

diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/Internal.java b/okhttp/src/main/java/com/squareup/okhttp/internal/Internal.java
@@ -42,22 +42,10 @@ public abstract Transport newTransport(Connection connection, HttpEngine httpEng
 
   public abstract int recycleCount(Connection connection);
 
-  public abstract Object getOwner(Connection connection);
-
   public abstract void setProtocol(Connection connection, Protocol protocol);
 
   public abstract void setOwner(Connection connection, HttpEngine httpEngine);
 
-  public abstract void connect(Connection connection,
-      int connectTimeout, int readTimeout, int writeTimeout, Request request) throws IOException;
-
-  public abstract boolean isConnected(Connection connection);
-
-  public abstract boolean isSpdy(Connection connection);
-
-  public abstract void setTimeouts(Connection connection, int readTimeout, int writeTimeout)
-      throws IOException;
-
   public abstract boolean isReadable(Connection pooled);
 
   public abstract void addLine(Headers.Builder builder, String line);
@@ -68,7 +56,8 @@ public abstract void setTimeouts(Connection connection, int readTimeout, int wri
 
   public abstract void recycle(ConnectionPool pool, Connection connection);
 
-  public abstract void share(ConnectionPool connectionPool, Connection connection);
-
   public abstract RouteDatabase routeDatabase(OkHttpClient client);
+
+  public abstract void connectAndSetOwner(OkHttpClient client, Connection connection,
+      HttpEngine owner, Request request) throws IOException;
 }
diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java b/okhttp/src/main/java/com/squareup/okhttp/internal/http/HttpEngine.java
@@ -17,7 +17,6 @@
 
 package com.squareup.okhttp.internal.http;
 
-import com.squareup.okhttp.Address;
 import com.squareup.okhttp.Connection;
 import com.squareup.okhttp.Headers;
 import com.squareup.okhttp.MediaType;
@@ -38,14 +37,11 @@
 import java.net.ProtocolException;
 import java.net.Proxy;
 import java.net.URL;
-import java.net.UnknownHostException;
 import java.security.cert.CertificateException;
 import java.util.Date;
 import java.util.List;
 import java.util.Map;
-import javax.net.ssl.HostnameVerifier;
 import javax.net.ssl.SSLHandshakeException;
-import javax.net.ssl.SSLSocketFactory;
 import okio.Buffer;
 import okio.BufferedSink;
 import okio.BufferedSource;
@@ -241,11 +237,6 @@ public void sendRequest() throws IOException {
         connect(networkRequest);
       }
 
-      // Blow up if we aren't the current owner of the connection.
-      if (Internal.instance.getOwner(connection) != this && !Internal.instance.isSpdy(connection)) {
-        throw new AssertionError();
-      }
-
       transport = Internal.instance.newTransport(connection, this);
 
       // Create a request body if we don't have one already. We'll already have
@@ -297,35 +288,10 @@ private void connect(Request request) throws IOException {
     if (connection != null) throw new IllegalStateException();
 
     if (routeSelector == null) {
-      String uriHost = request.url().getHost();
-      if (uriHost == null || uriHost.length() == 0) {
-        throw new UnknownHostException(request.url().toString());
-      }
-      SSLSocketFactory sslSocketFactory = null;
-      HostnameVerifier hostnameVerifier = null;
-      if (request.isHttps()) {
-        sslSocketFactory = client.getSslSocketFactory();
-        hostnameVerifier = client.getHostnameVerifier();
-      }
-      Address address = new Address(uriHost, getEffectivePort(request.url()),
-          client.getSocketFactory(), sslSocketFactory, hostnameVerifier, client.getAuthenticator(),
-          client.getProxy(), client.getProtocols());
-      routeSelector = new RouteSelector(address, request.uri(), client.getProxySelector(),
-          client.getConnectionPool(), Dns.DEFAULT, Internal.instance.routeDatabase(client));
+      routeSelector = RouteSelector.get(request, client, Dns.DEFAULT);
     }
 
-    connection = routeSelector.next(request.method());
-    Internal.instance.setOwner(connection, this);
-
-    if (!Internal.instance.isConnected(connection)) {
-      Internal.instance.connect(connection, client.getConnectTimeout(), client.getReadTimeout(),
-          client.getWriteTimeout(), tunnelRequest(connection, request));
-      if (Internal.instance.isSpdy(connection)) {
-        Internal.instance.share(client.getConnectionPool(), connection);
-      }
-      Internal.instance.routeDatabase(client).connected(connection.getRoute());
-    }
-    Internal.instance.setTimeouts(connection, client.getReadTimeout(), client.getWriteTimeout());
+    connection = routeSelector.next(this);
     route = connection.getRoute();
   }
 
@@ -780,39 +746,6 @@ private static Headers combine(Headers cachedHeaders, Headers networkHeaders) th
     return result.build();
   }
 
-  /**
-   * Returns a request that creates a TLS tunnel via an HTTP proxy, or null if
-   * no tunnel is necessary. Everything in the tunnel request is sent
-   * unencrypted to the proxy server, so tunnels include only the minimum set of
-   * headers. This avoids sending potentially sensitive data like HTTP cookies
-   * to the proxy unencrypted.
-   */
-  private Request tunnelRequest(Connection connection, Request request) throws IOException {
-    if (!connection.getRoute().requiresTunnel()) return null;
-
-    String host = request.url().getHost();
-    int port = getEffectivePort(request.url());
-    String authority = (port == getDefaultPort("https")) ? host : (host + ":" + port);
-    Request.Builder result = new Request.Builder()
-        .url(new URL("https", host, port, "/"))
-        .header("Host", authority)
-        .header("Proxy-Connection", "Keep-Alive"); // For HTTP/1.0 proxies like Squid.
-
-    // Copy over the User-Agent header if it exists.
-    String userAgent = request.header("User-Agent");
-    if (userAgent != null) {
-      result.header("User-Agent", userAgent);
-    }
-
-    // Copy over the Proxy-Authorization header if it exists.
-    String proxyAuthorization = request.header("Proxy-Authorization");
-    if (proxyAuthorization != null) {
-      result.header("Proxy-Authorization", proxyAuthorization);
-    }
-
-    return result.build();
-  }
-
   public void receiveHeaders(Headers headers) throws IOException {
     CookieHandler cookieHandler = client.getCookieHandler();
     if (cookieHandler != null) {