Refactoring HttpClient to stop using deprecated methods of Apache HC

Author: barancev

java/client/src/org/openqa/selenium/remote/HttpCommandExecutor.java

@@ -49,6 +49,7 @@ public class HttpCommandExecutor implements CommandExecutor, NeedsLocalLogs {
 
   private final URL remoteServer;
   private final HttpClient client;
+  private final HttpClient.Factory httpClientFactory;
   private final Map<String, CommandInfo> additionalCommands;
   private CommandCodec<HttpRequest> commandCodec;
   private ResponseCodec<HttpResponse> responseCodec;
@@ -85,6 +86,7 @@ public HttpCommandExecutor(
     }
 
     this.additionalCommands = additionalCommands;
+    this.httpClientFactory = httpClientFactory;
     this.client = httpClientFactory.createClient(remoteServer);
   }
 
@@ -171,7 +173,7 @@ public Response execute(Command command) throws IOException {
         }
       }
       if (QUIT.equals(command.getName())) {
-    	  client.close();
+        httpClientFactory.cleanupIdleClients();
       }
       return response;
     } catch (UnsupportedCommandException e) {

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

@@ -34,12 +34,14 @@ public interface HttpClient {
    * @throws IOException if an I/O error occurs.
    */
   HttpResponse execute(HttpRequest request, boolean followRedirects) throws IOException;
-  
+
   /**
-	* Closes the connections associated with this client. 
-	*
-	* @throws  IOException  if an I/O error occurs.
-	*/
+	 * Closes the connections associated with this client.
+	 *
+	 * @throws  IOException  if an I/O error occurs.
+   * @deprecated This responsibility moved to Factory
+	 */
+  @Deprecated
   void close() throws IOException;
 
   interface Factory {
@@ -51,5 +53,10 @@ interface Factory {
      * @return HttpClient
      */
     HttpClient createClient(URL url);
+
+    /**
+     * Closes idle clients.
+     */
+    void cleanupIdleClients();
   }
 }

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

@@ -44,7 +44,7 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
-import java.util.concurrent.TimeUnit;
+import java.util.StringTokenizer;
 
 public class ApacheHttpClient implements org.openqa.selenium.remote.http.HttpClient {
 
@@ -136,22 +136,14 @@ private org.apache.http.HttpResponse fallBackExecute(
       HttpContext context, HttpUriRequest httpMethod) throws IOException {
     try {
       return client.execute(targetHost, httpMethod, context);
-    } catch (BindException e) {
+    } catch (BindException | NoHttpResponseException e) {
       // If we get this, there's a chance we've used all the local ephemeral sockets
       // Sleep for a bit to let the OS reclaim them, then try the request again.
       try {
         Thread.sleep(2000);
       } catch (InterruptedException ie) {
         throw new RuntimeException(ie);
       }
-    } catch (NoHttpResponseException e) {
-      // If we get this, there's a chance we've used all the remote ephemeral sockets
-      // Sleep for a bit to let the OS reclaim them, then try the request again.
-      try {
-        Thread.sleep(2000);
-      } catch (InterruptedException ie) {
-        throw new RuntimeException(ie);
-      }
     }
     return client.execute(targetHost, httpMethod, context);
   }
@@ -228,15 +220,21 @@ public org.openqa.selenium.remote.http.HttpClient createClient(URL url) {
       checkNotNull(url, "null URL");
       HttpClient client;
       if (url.getUserInfo() != null) {
+        StringTokenizer tokens = new StringTokenizer(url.getUserInfo(), ":");
         UsernamePasswordCredentials credentials =
-            new UsernamePasswordCredentials(url.getUserInfo());
+            new UsernamePasswordCredentials(tokens.nextToken(), tokens.nextToken());
         client = clientFactory.createHttpClient(credentials);
       } else {
         client = clientFactory.getHttpClient();
       }
       return new ApacheHttpClient(client, url);
     }
 
+    @Override
+    public void cleanupIdleClients() {
+      clientFactory.cleanupIdleClients();
+    }
+
     private static synchronized HttpClientFactory getDefaultHttpClientFactory() {
       if (defaultClientFactory == null) {
         defaultClientFactory = new HttpClientFactory();
@@ -245,8 +243,8 @@ private static synchronized HttpClientFactory getDefaultHttpClientFactory() {
     }
   }
 
+  @Deprecated
   @Override
   public void close() throws IOException {
-    client.getConnectionManager().closeIdleConnections(0, TimeUnit.SECONDS);
   }
 }

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

@@ -47,6 +47,7 @@
 
 import java.io.IOException;
 import java.net.ProxySelector;
+import java.util.concurrent.TimeUnit;
 
 public class HttpClientFactory {
 
@@ -156,6 +157,10 @@ public void close() {
     gridClientConnectionManager.shutdown();
   }
 
+  void cleanupIdleClients() {
+    gridClientConnectionManager.closeIdleConnections(0, TimeUnit.SECONDS);
+  }
+
   static class MyRedirectHandler implements RedirectStrategy {
 
     public boolean isRedirected(HttpRequest request, HttpResponse response, HttpContext context)

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

@@ -113,9 +113,9 @@ public HttpResponse execute(HttpRequest request, boolean followRedirects) throws
     }
   }
 
+  @Deprecated
   @Override
   public void close() throws IOException {
-
   }
 
   public static class Factory implements HttpClient.Factory {
@@ -124,5 +124,9 @@ public static class Factory implements HttpClient.Factory {
     public HttpClient createClient(URL url) {
       return new JreHttpClient(Objects.requireNonNull(url, "Base URL must be set"));
     }
+
+    @Override
+    public void cleanupIdleClients() {
+    }
   }
 }