[java] Use one HttpClient.Factory inside one RemoteWebDriver instance

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

@@ -42,54 +42,83 @@
 public class HttpCommandExecutor implements CommandExecutor {
 
   private final URL remoteServer;
-  public final HttpClient client;
-  protected final HttpClient.Factory httpClientFactory;
   protected final Map<String, CommandInfo> additionalCommands;
+  protected final HttpClient client;
   protected @Nullable CommandCodec<HttpRequest> commandCodec;
   protected @Nullable ResponseCodec<HttpResponse> responseCodec;
 
-  private static class DefaultClientFactoryHolder {
-    static HttpClient.Factory defaultClientFactory = HttpClient.Factory.createDefault();
-  }
-
+  @Deprecated(forRemoval = true, since = "4.50.0")
   public static HttpClient.Factory getDefaultClientFactory() {
-    return DefaultClientFactoryHolder.defaultClientFactory;
+    return RemoteWebDriver.DEFAULT_CLIENT_FACTORY;
   }
 
+  @Deprecated(forRemoval = true, since = "4.50.0")
   public HttpCommandExecutor(URL addressOfRemoteServer) {
     this(emptyMap(), Require.nonNull("Server URL", addressOfRemoteServer));
   }
 
+  @Deprecated(forRemoval = true, since = "4.50.0")
   public HttpCommandExecutor(ClientConfig config) {
     this(
         emptyMap(),
         Require.nonNull("HTTP client configuration", config),
-        getDefaultClientFactory());
+        HttpClient.Factory.createDefault());
+  }
+
+  /**
+   * Creates an {@link HttpCommandExecutor} that supports only standard commands.
+   *
+   * @param httpClient the HttpClient to execute commands with
+   * @param addressOfRemoteServer URL of remote end Selenium server
+   */
+  public HttpCommandExecutor(HttpClient httpClient, URL addressOfRemoteServer) {
+    this(httpClient, Map.of(), addressOfRemoteServer);
   }
 
   /**
    * Creates an {@link HttpCommandExecutor} that supports non-standard {@code additionalCommands} in
    * addition to the standard.
    *
+   * @param httpClient the HttpClient to execute commands with
    * @param additionalCommands additional commands to allow the command executor to process
    * @param addressOfRemoteServer URL of remote end Selenium server
    */
+  public HttpCommandExecutor(
+      HttpClient httpClient,
+      Map<String, CommandInfo> additionalCommands,
+      URL addressOfRemoteServer) {
+    this.client = httpClient;
+    this.additionalCommands =
+        new HashMap<>(Require.nonNull("Additional commands", additionalCommands));
+    this.remoteServer = addressOfRemoteServer;
+  }
+
+  /**
+   * Creates an {@link HttpCommandExecutor} that supports non-standard {@code additionalCommands} in
+   * addition to the standard.
+   *
+   * @param additionalCommands additional commands to allow the command executor to process
+   * @param addressOfRemoteServer URL of remote end Selenium server
+   */
+  @Deprecated(forRemoval = true, since = "4.50.0")
   public HttpCommandExecutor(
       Map<String, CommandInfo> additionalCommands, URL addressOfRemoteServer) {
     this(
         Require.nonNull("Additional commands", additionalCommands),
         Require.nonNull("Server URL", addressOfRemoteServer),
-        getDefaultClientFactory());
+        HttpClient.Factory.createDefault());
   }
 
+  @Deprecated(forRemoval = true, since = "4.50.0")
   public HttpCommandExecutor(
       Map<String, CommandInfo> additionalCommands, URL addressOfRemoteServer, ClientConfig config) {
     this(
         additionalCommands,
         config.baseUrl(Require.nonNull("Server URL", addressOfRemoteServer)),
-        getDefaultClientFactory());
+        HttpClient.Factory.createDefault());
   }
 
+  @Deprecated(forRemoval = true, since = "4.50.0")
   public HttpCommandExecutor(
       Map<String, CommandInfo> additionalCommands,
       URL addressOfRemoteServer,
@@ -100,15 +129,12 @@ public HttpCommandExecutor(
         httpClientFactory);
   }
 
+  @Deprecated(forRemoval = true, since = "4.50.0")
   public HttpCommandExecutor(
       Map<String, CommandInfo> additionalCommands,
       ClientConfig config,
       HttpClient.Factory httpClientFactory) {
-    remoteServer = Require.nonNull("HTTP client configuration", config).baseUrl();
-    this.additionalCommands =
-        new HashMap<>(Require.nonNull("Additional commands", additionalCommands));
-    this.httpClientFactory = Require.nonNull("HTTP client factory", httpClientFactory);
-    this.client = this.httpClientFactory.createClient(config);
+    this(httpClientFactory.createClient(config), additionalCommands, config.baseUrl());
   }
 
   /**
@@ -147,6 +173,7 @@ protected void defineCommand(String commandName, CommandInfo info) {
     commandCodec.defineCommand(commandName, info.getMethod(), info.getUrl());
   }
 
+  @Deprecated(forRemoval = true, since = "4.50.0")
   public URL getAddressOfRemoteServer() {
     return remoteServer;
   }
@@ -204,7 +231,6 @@ public Response execute(Command command) throws IOException {
       }
       if (QUIT.equals(command.getName())) {
         client.close();
-        httpClientFactory.cleanupIdleClients();
       }
       return response;
     } catch (UnsupportedCommandException e) {

java/src/org/openqa/selenium/remote/RemoteWebDriver.java

@@ -19,7 +19,6 @@
 
 import static java.util.Collections.singleton;
 import static java.util.Objects.requireNonNull;
-import static java.util.Objects.requireNonNullElseGet;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static java.util.logging.Level.SEVERE;
 import static org.openqa.selenium.remote.CapabilityType.PLATFORM_NAME;
@@ -38,7 +37,6 @@
 import java.util.ArrayList;
 import java.util.Base64;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
@@ -102,7 +100,6 @@
 import org.openqa.selenium.remote.http.jdk.ConnectionException;
 import org.openqa.selenium.remote.service.DriverCommandExecutor;
 import org.openqa.selenium.remote.tracing.TracedHttpClient;
-import org.openqa.selenium.remote.tracing.Tracer;
 import org.openqa.selenium.remote.tracing.opentelemetry.OpenTelemetryTracer;
 import org.openqa.selenium.virtualauthenticator.Credential;
 import org.openqa.selenium.virtualauthenticator.HasVirtualAuthenticator;
@@ -127,13 +124,15 @@ public class RemoteWebDriver
   }
 
   private static final Logger LOG = Logger.getLogger(RemoteWebDriver.class.getName());
+  static final HttpClient.Factory DEFAULT_CLIENT_FACTORY = HttpClient.Factory.createDefault();
 
   /** Boolean system property that defines whether the tracing is enabled or not. */
   private static final String WEBDRIVER_REMOTE_ENABLE_TRACING = "webdriver.remote.enableTracing";
 
   private final ElementLocation elementLocation = new ElementLocation();
   private Level level = Level.FINE;
   private ErrorHandler errorHandler = new ErrorHandler();
+  private final HttpClient.Factory clientFactory;
   private final ClientConfig clientConfig;
   private CommandExecutor executor;
   protected Capabilities capabilities;
@@ -155,19 +154,17 @@ public class RemoteWebDriver
   @SuppressWarnings("DataFlowIssue")
   protected RemoteWebDriver() {
     this.capabilities = new ImmutableCapabilities();
+    this.clientFactory = DEFAULT_CLIENT_FACTORY;
     this.clientConfig = ClientConfig.defaultConfig();
     this.executor = null;
   }
 
   public RemoteWebDriver(Capabilities capabilities) {
-    this(
-        getDefaultServerURL(),
-        Require.nonNull("Capabilities", capabilities),
-        Boolean.parseBoolean(System.getProperty(WEBDRIVER_REMOTE_ENABLE_TRACING, "true")));
+    this(getDefaultServerURL(), capabilities);
   }
 
   public RemoteWebDriver(Capabilities capabilities, boolean enableTracing) {
-    this(getDefaultServerURL(), Require.nonNull("Capabilities", capabilities), enableTracing);
+    this(getDefaultServerURL(), capabilities, enableTracing);
   }
 
   public RemoteWebDriver(URL remoteAddress, Capabilities capabilities) {
@@ -176,12 +173,30 @@ public RemoteWebDriver(URL remoteAddress, Capabilities capabilities) {
 
   public RemoteWebDriver(URL remoteAddress, Capabilities capabilities, ClientConfig clientConfig) {
     this(
-        createExecutor(
-            Require.nonNull("Server URL", remoteAddress),
-            Boolean.parseBoolean(System.getProperty(WEBDRIVER_REMOTE_ENABLE_TRACING, "true")),
-            clientConfig),
-        Require.nonNull("Capabilities", capabilities),
-        clientConfig.baseUrl(remoteAddress));
+        remoteAddress,
+        capabilities,
+        clientConfig,
+        Boolean.parseBoolean(System.getProperty(WEBDRIVER_REMOTE_ENABLE_TRACING, "true")));
+  }
+
+  // private to ensure the clientFactory is already wrapped into a TracedHttpClient.Factory in case
+  // the enableTracing is true
+  private RemoteWebDriver(
+      Capabilities capabilities,
+      HttpClient.Factory clientFactory,
+      ClientConfig clientConfig,
+      boolean enableTracing) {
+    this(
+        enableTracing
+            ? new TracedCommandExecutor(
+                new HttpCommandExecutor(
+                    clientFactory.createClient(clientConfig), clientConfig.baseUrl()),
+                OpenTelemetryTracer.getInstance())
+            : new HttpCommandExecutor(
+                clientFactory.createClient(clientConfig), clientConfig.baseUrl()),
+        capabilities,
+        clientFactory,
+        clientConfig);
   }
 
   public RemoteWebDriver(URL remoteAddress, Capabilities capabilities, boolean enableTracing) {
@@ -194,20 +209,36 @@ public RemoteWebDriver(
       ClientConfig clientConfig,
       boolean enableTracing) {
     this(
-        createExecutor(Require.nonNull("Server URL", remoteAddress), enableTracing, clientConfig),
-        Require.nonNull("Capabilities", capabilities),
-        clientConfig.baseUrl(remoteAddress));
+        capabilities,
+        enableTracing
+            ? new TracedHttpClient.Factory(
+                OpenTelemetryTracer.getInstance(), DEFAULT_CLIENT_FACTORY)
+            : DEFAULT_CLIENT_FACTORY,
+        clientConfig.baseUrl(remoteAddress),
+        enableTracing);
   }
 
+  // ignore WEBDRIVER_REMOTE_ENABLE_TRACING here, to allow extended external control
   public RemoteWebDriver(CommandExecutor executor, Capabilities capabilities) {
-    this(executor, capabilities, ClientConfig.defaultConfig());
+    this(executor, capabilities, DEFAULT_CLIENT_FACTORY, ClientConfig.defaultConfig());
   }
 
+  // ignore WEBDRIVER_REMOTE_ENABLE_TRACING here, to allow extended external control
   public RemoteWebDriver(
       CommandExecutor executor, Capabilities capabilities, ClientConfig clientConfig) {
+    this(executor, capabilities, DEFAULT_CLIENT_FACTORY, clientConfig);
+  }
+
+  // ignore WEBDRIVER_REMOTE_ENABLE_TRACING here, to allow extended external control
+  public RemoteWebDriver(
+      CommandExecutor executor,
+      Capabilities capabilities,
+      HttpClient.Factory clientFactory,
+      ClientConfig clientConfig) {
+    this.clientFactory = Require.nonNull("Client factory", clientFactory);
     this.clientConfig = Require.nonNull("Client config", clientConfig);
     this.executor = Require.nonNull("Command executor", executor);
-    this.capabilities = requireNonNullElseGet(capabilities, () -> new ImmutableCapabilities());
+    this.capabilities = Require.nonNull("Capabilities", capabilities);
 
     try {
       startSession(capabilities);
@@ -230,22 +261,6 @@ private static URL getDefaultServerURL() {
     }
   }
 
-  private static CommandExecutor createExecutor(
-      URL remoteAddress, boolean enableTracing, ClientConfig clientConfig) {
-    ClientConfig config = clientConfig.baseUrl(remoteAddress);
-    if (enableTracing) {
-      Tracer tracer = OpenTelemetryTracer.getInstance();
-      CommandExecutor executor =
-          new HttpCommandExecutor(
-              Collections.emptyMap(),
-              config,
-              new TracedHttpClient.Factory(tracer, HttpClient.Factory.createDefault()));
-      return new TracedCommandExecutor(executor, tracer);
-    } else {
-      return new HttpCommandExecutor(config);
-    }
-  }
-
   @Beta
   public static RemoteWebDriverBuilder builder() {
     return new RemoteWebDriverBuilder();
@@ -464,9 +479,8 @@ private Optional<BiDi> createBiDi() {
       LOG.warning("BiDi was requested but the remote end did not return a valid webSocketUrl.");
       return Optional.empty();
     }
-    HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
     ClientConfig wsConfig = this.clientConfig.baseUri(wsUri);
-    HttpClient wsClient = clientFactory.createClient(wsConfig);
+    HttpClient wsClient = this.clientFactory.createClient(wsConfig);
     try {
       Connection biDiConnection = new Connection(wsClient, wsUri.toString());
       return Optional.of(new BiDi(biDiConnection, wsConfig.wsTimeout()));

java/src/org/openqa/selenium/remote/RemoteWebDriverBuilder.java

@@ -397,7 +397,7 @@ private WebDriver getRemoteDriver() {
                 new AddWebDriverSpecHeaders()
                     .andThen(new ErrorFilter())
                     .andThen(new DumpHttpExchangeFilter())
-                    .andThen(new CloseHttpClientFilter(clientFactory, client)));
+                    .andThen(new CloseHttpClientFilter(client)));
 
     Either<SessionNotCreatedException, ProtocolHandshake.Result> result;
     try {
@@ -540,11 +540,9 @@ private NewSessionPayload getPayload() {
 
   private static class CloseHttpClientFilter implements Filter {
 
-    private final HttpClient.Factory factory;
     private final HttpClient client;
 
-    CloseHttpClientFilter(HttpClient.Factory factory, HttpClient client) {
-      this.factory = Require.nonNull("Http client factory", factory);
+    CloseHttpClientFilter(HttpClient client) {
       this.client = Require.nonNull("Http client", client);
     }
 
@@ -564,7 +562,6 @@ public HttpHandler apply(HttpHandler next) {
                         } catch (Exception e) {
                           LOG.log(WARNING, "Exception swallowed while closing http client", e);
                         }
-                        factory.cleanupIdleClients();
                       }
                     });
           }

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

@@ -91,7 +91,12 @@ default HttpClient createClient(URL url) {
 
     HttpClient createClient(ClientConfig config);
 
-    /** Closes idle clients. */
+    /**
+     * Closes idle clients.
+     *
+     * @deprecated the client knows the Factory and could call a cleanup method after close
+     */
+    @Deprecated(forRemoval = true, since = "4.50.0")
     default void cleanupIdleClients() {
       // do nothing by default.
     }

java/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java

@@ -78,13 +78,14 @@ public class JdkHttpClient implements HttpClient {
   private static final AtomicInteger POOL_COUNTER = new AtomicInteger(0);
   private final JdkHttpMessages messages;
   private final HttpHandler handler;
+  private final HttpClient.Factory factory;
   private java.net.http.HttpClient client;
   private final List<WebSocket> websockets;
   private final ExecutorService executorService;
   private final Duration readTimeout;
   private final Duration connectTimeout;
 
-  JdkHttpClient(ClientConfig config) {
+  JdkHttpClient(HttpClient.Factory factory, ClientConfig config) {
     Objects.requireNonNull(config, "Client config must be set");
 
     this.messages = new JdkHttpMessages(config);
@@ -146,6 +147,7 @@ protected PasswordAuthentication getPasswordAuthentication() {
       builder.version(Version.valueOf(version));
     }
 
+    this.factory = factory;
     this.client = builder.build();
   }
 
@@ -599,6 +601,7 @@ public void close() {
     }
     this.client = null;
     executorService.shutdown();
+    this.factory.cleanupIdleClients();
   }
 
   @AutoService(HttpClient.Factory.class)
@@ -608,7 +611,7 @@ public static class Factory implements HttpClient.Factory {
     @Override
     public HttpClient createClient(ClientConfig config) {
       Objects.requireNonNull(config, "Client config must be set");
-      return new JdkHttpClient(config);
+      return new JdkHttpClient(this, config);
     }
   }