[Grid] A new createClient method instead of duplicating the Factory.

This allows us to set timeouts per connection.
Still room for improvement.

Author: diemol

java/client/src/org/openqa/selenium/remote/http/HttpClient.java

@@ -61,19 +61,6 @@ static Factory createDefault() {
       }
     }
 
-    static Factory createConfigured(int connectionTimeout, int readTimeout) {
-      String defaultFactory = System.getProperty("webdriver.http.factory", "okhttp");
-      switch (defaultFactory) {
-        case "apache":
-          return new ApacheHttpClient.Factory(connectionTimeout, readTimeout);
-
-        case "okhttp":
-        default:
-          return new OkHttpClient.Factory(Duration.ofSeconds(connectionTimeout),
-                                          Duration.ofSeconds(readTimeout));
-      }
-    }
-
     /**
      * Creates a HTTP client that will send requests to the given URL.
      *
@@ -82,6 +69,16 @@ static Factory createConfigured(int connectionTimeout, int readTimeout) {
      */
     HttpClient createClient(URL url);
 
+    /**
+     * Creates a HTTP client that will send requests to the given URL.
+     *
+     * @param url URL
+     * @param connectionTimeout int
+     * @param readTimeout int
+     * @return HttpClient
+     */
+    HttpClient createClient(URL url, Duration connectionTimeout, Duration readTimeout);
+
     /**
      * Closes idle clients.
      */

java/client/src/org/openqa/selenium/remote/internal/ApacheHttpClient.java

@@ -45,6 +45,7 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.time.Duration;
 import java.util.AbstractMap;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -264,26 +265,28 @@ public Factory(HttpClientFactory clientFactory) {
       this.clientFactory = checkNotNull(clientFactory, "null HttpClientFactory");
     }
 
-    public Factory(int connectionTimeout, int socketTimeout) {
-      int connectionTimeoutToMillis = (int) SECONDS.toMillis(connectionTimeout);
-      int socketTimeoutToMillis = (int) SECONDS.toMillis(socketTimeout);
-      this.clientFactory = new HttpClientFactory(connectionTimeoutToMillis, socketTimeoutToMillis);
+    @Override
+    public org.openqa.selenium.remote.http.HttpClient createClient(URL url) {
+      return createClient(url, Duration.ofMinutes(2), Duration.ofHours(3));
     }
 
     @Override
-    public org.openqa.selenium.remote.http.HttpClient createClient(URL url) {
+    public org.openqa.selenium.remote.http.HttpClient createClient(URL url,
+                                                                   Duration connectionTimeout,
+                                                                   Duration readTimeout) {
       checkNotNull(url, "null URL");
       HttpClient client;
       if (url.getUserInfo() != null) {
         StringTokenizer tokens = new StringTokenizer(url.getUserInfo(), ":");
         UsernamePasswordCredentials credentials =
             new UsernamePasswordCredentials(tokens.nextToken(), tokens.nextToken());
-        client = clientFactory.createHttpClient(credentials);
+        client = clientFactory.createHttpClient(credentials, (int) connectionTimeout.toMillis(),
+                                                (int) readTimeout.toMillis());
       } else {
-        client = clientFactory.getHttpClient();
+        client = clientFactory.createHttpClient(null, (int) connectionTimeout.toMillis(),
+                                                (int) readTimeout.toMillis());
       }
-      return new ApacheHttpClient(client, url);
-    }
+      return new ApacheHttpClient(client, url);    }
 
     @Override
     public void cleanupIdleClients() {

java/client/src/org/openqa/selenium/remote/internal/OkHttpClient.java

@@ -116,8 +116,8 @@ public HttpResponse execute(HttpRequest request) throws IOException {
   public static class Factory implements HttpClient.Factory {
 
     private final ConnectionPool pool = new ConnectionPool();
-    private final long connectionTimeout;
-    private final long readTimeout;
+    private final Duration connectionTimeout;
+    private final Duration readTimeout;
 
     public Factory() {
       this(Duration.ofMinutes(2), Duration.ofHours(3));
@@ -127,18 +127,23 @@ public Factory(Duration connectionTimeout, Duration readTimeout) {
       Objects.requireNonNull(connectionTimeout, "Connection timeout cannot be null");
       Objects.requireNonNull(readTimeout, "Read timeout cannot be null");
 
-      this.connectionTimeout = connectionTimeout.toMillis();
-      this.readTimeout = readTimeout.toMillis();
+      this.connectionTimeout = connectionTimeout;
+      this.readTimeout = readTimeout;
     }
 
     @Override
     public HttpClient createClient(URL url) {
+      return createClient(url, this.connectionTimeout, this.readTimeout);
+    }
+
+    @Override
+    public HttpClient createClient(URL url, Duration connectionTimeout, Duration readTimeout) {
       okhttp3.OkHttpClient.Builder client = new okhttp3.OkHttpClient.Builder()
           .connectionPool(pool)
           .followRedirects(true)
           .followSslRedirects(true)
-          .readTimeout(readTimeout, MILLISECONDS)
-          .connectTimeout(connectionTimeout, MILLISECONDS);
+          .readTimeout(readTimeout.toMillis(), MILLISECONDS)
+          .connectTimeout(connectionTimeout.toMillis(), MILLISECONDS);
 
       String info = url.getUserInfo();
       if (!Strings.isNullOrEmpty(info)) {

java/server/src/org/openqa/grid/internal/BaseGridRegistry.java

@@ -21,17 +21,18 @@
 import org.openqa.selenium.remote.http.HttpClient;
 
 import java.net.URL;
+import java.time.Duration;
 
 public abstract class BaseGridRegistry implements GridRegistry {
-  protected HttpClient.Factory httpClientFactory;
+  protected final HttpClient.Factory httpClientFactory;
 
   // The following needs to be volatile because we expose a public setters
   protected volatile Hub hub;
 
   public BaseGridRegistry(Hub hub) {
+    this.httpClientFactory = HttpClient.Factory.createDefault();
     this.hub = hub;
   }
-
   /**
    * @see GridRegistry#getHub()
    */
@@ -52,17 +53,12 @@ public void setHub(Hub hub) {
    */
   @Override
   public HttpClient getHttpClient(URL url) {
-    if (httpClientFactory == null) {
-      httpClientFactory = HttpClient.Factory.createDefault();
-    }
     return httpClientFactory.createClient(url);
   }
 
   @Override
   public HttpClient getHttpClient(URL url, int connectionTimeout, int readTimeout) {
-    if (httpClientFactory == null) {
-      httpClientFactory = HttpClient.Factory.createConfigured(connectionTimeout, readTimeout);
-    }
-    return httpClientFactory.createClient(url);
+    return httpClientFactory.createClient(url, Duration.ofSeconds(connectionTimeout),
+                                          Duration.ofSeconds(readTimeout));
   }
 }

java/server/src/org/openqa/grid/internal/BaseRemoteProxy.java

@@ -428,15 +428,24 @@ public int getTimeOut() {
     return config.timeout * 1000;
   }
 
+  /**
+   * @return the {@link HttpClient.Factory} to use.
+   * @deprecated use {@link BaseRemoteProxy#getHttpClient(URL, int, int)}
+   */
   public HttpClient getHttpClient(URL url) {
-    return getRegistry().getHttpClient(url, config.browserTimeout, config.browserTimeout);
+    return getRegistry().getHttpClient(url);
+  }
+
+  public HttpClient getHttpClient(URL url, int connectionTimeout, int readTimeout) {
+    return getRegistry().getHttpClient(url, connectionTimeout, readTimeout);
   }
 
   public Map<String, Object> getProxyStatus() {
     String url = getRemoteHost().toExternalForm() + "/wd/hub/status";
 
     HttpRequest r = new HttpRequest(GET, url);
-    HttpClient client = getHttpClient(getRemoteHost());
+    HttpClient client = getHttpClient(getRemoteHost(), config.nodeStatusCheckTimeout,
+                                      config.nodeStatusCheckTimeout);
     HttpResponse response;
     String existingName = Thread.currentThread().getName();
     try {

java/server/src/org/openqa/grid/internal/RemoteProxy.java

@@ -158,9 +158,19 @@ default TestSlot createTestSlot(SeleniumProtocol protocol, Map<String, Object> c
 
   /**
    * @return an {@link HttpClient} for a particular {@link URL}.
+   * @deprecated use {@link RemoteProxy#getHttpClient(URL, int, int)}
    */
   HttpClient getHttpClient(URL url);
 
+  /**
+   *
+   * @param url URL
+   * @param connectionTimeout int
+   * @param readTimeout int
+   * @return an {@link HttpClient} for a particular {@link URL}.
+   */
+  HttpClient getHttpClient(URL url, int connectionTimeout, int readTimeout);
+
     /**
    * Renders the status of the node as JSON.  Useful for APIs.
    *

java/server/src/org/openqa/grid/internal/TestSession.java

@@ -201,7 +201,8 @@ public String toString() {
 
 
   private HttpClient getClient(URL url) {
-    return slot.getProxy().getHttpClient(url);
+    Integer browserTimeout = slot.getProxy().getConfig().browserTimeout;
+    return slot.getProxy().getHttpClient(url, browserTimeout, browserTimeout);
   }
 
   /*

java/server/src/org/openqa/grid/web/servlet/NodeSessionsServlet.java

@@ -102,7 +102,9 @@ private Map<String, Object> extractSessionInfo(RemoteProxy proxy) {
     try {
       URL url = proxy.getRemoteHost();
       HttpRequest req = new HttpRequest(HttpMethod.GET, "/wd/hub/sessions");
-      HttpResponse rsp = proxy.getHttpClient(url).execute(req);
+      Integer nodeStatusCheckTimeout = proxy.getConfig().nodeStatusCheckTimeout;
+      HttpResponse rsp = proxy.getHttpClient(url, nodeStatusCheckTimeout, nodeStatusCheckTimeout)
+          .execute(req);
 
       try (InputStream in = new ByteArrayInputStream(rsp.getContent());
            Reader reader = new InputStreamReader(in, rsp.getContentEncoding());

java/server/test/org/openqa/grid/e2e/node/BrowserTimeOutTest.java

@@ -19,7 +19,6 @@
 
 import org.junit.After;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.openqa.grid.common.GridRole;
 import org.openqa.grid.e2e.utils.GridTestHelper;

java/server/test/org/openqa/grid/internal/listener/CommandListenerTest.java

@@ -87,7 +87,7 @@ public URL getRemoteHost() {
     }
 
     @Override
-    public HttpClient getHttpClient(URL url) {
+    public HttpClient getHttpClient(URL url, int connectionTimeout, int readTimeout) {
       // Create mocks for network traffic
       HttpClient client = mock(HttpClient.class);
       HttpResponse response = mock(HttpResponse.class);