.pr_agent_auto_best_practices

Pattern 1: Ensure resource cleanup and cancellation by using context managers/try-finally and disposing/closing handles for sockets, HTTP responses, listeners, scheduled tasks, and drivers, even on exceptions.

Example code before:

response = urllib.request.urlopen(url)
data = response.read()
# response may leak if exceptions occur

Example code after:

with urllib.request.urlopen(url) as response:
    data = response.read()

Relevant past accepted suggestions:

---

Suggestion 1:

Add robust test cleanup

Ensure the tab and user context are cleaned up even if assertions fail by wrapping them in try/finally blocks.

py/test/selenium/webdriver/common/bidi_emulation_tests.py [347-363]

 def test_set_locale_override_with_user_contexts(driver, pages, value):
     """Test setting locale override with user contexts."""
     user_context = driver.browser.create_user_context()
+    try:
+        context_id = driver.browsing_context.create(type=WindowTypes.TAB, user_context=user_context)
+        try:
+            driver.switch_to.window(context_id)
+            driver.emulation.set_locale_override(locale=value, user_contexts=[user_context])
+            driver.browsing_context.navigate(context_id, pages.url("formPage.html"), wait="complete")
+            current_locale = get_browser_locale(driver)
+            assert current_locale == value, f"Expected locale {value}, got {current_locale}"
+        finally:
+            driver.browsing_context.close(context_id)
+    finally:
+        driver.browser.remove_user_context(user_context)
 
-    context_id = driver.browsing_context.create(type=WindowTypes.TAB, user_context=user_context)
-
-    driver.switch_to.window(context_id)
-
-    driver.emulation.set_locale_override(locale=value, user_contexts=[user_context])
-
-    driver.browsing_context.navigate(context_id, pages.url("formPage.html"), wait="complete")
-
-    current_locale = get_browser_locale(driver)
-    assert current_locale == value, f"Expected locale {value}, got {current_locale}"
-
-    driver.browsing_context.close(context_id)
-    driver.browser.remove_user_context(user_context)
-

---

Suggestion 2:

Close HTTP response via context manager

Ensure the HTTP response object is properly closed by using a context manager. This prevents potential resource leaks under heavy usage or exceptions.

py/selenium/webdriver/common/utils.py [150-151]

-res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status")
-return res.getcode() == 200
+with urllib.request.urlopen(f"{scheme}://{host}:{port}/status") as res:
+    return res.getcode() == 200

---

Suggestion 3:

Cancel tasks and free resources

Ensure scheduled tasks are cancelled and resources are released on close; cancel per-node futures and clear maps to prevent thread and memory leaks

java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java [535-539]

 @Override
 public void close() {
   LOG.info("Shutting down LocalNodeRegistry");
+  Lock writeLock = lock.writeLock();
+  writeLock.lock();
+  try {
+    scheduledHealthChecks.values().forEach(f -> {
+      if (f != null) {
+        f.cancel(false);
+      }
+    });
+    scheduledHealthChecks.clear();
+    allChecks.clear();
+    nodes.values().forEach(n -> {
+      if (n instanceof RemoteNode) {
+        try {
+          ((RemoteNode) n).close();
+        } catch (Exception e) {
+          LOG.log(Level.WARNING, "Unable to close node properly: " + e.getMessage());
+        }
+      }
+    });
+    nodes.clear();
+  } finally {
+    writeLock.unlock();
+  }
 }

---

Suggestion 4:

Cancel tasks and close nodes

Ensure scheduled tasks are cancelled and nodes are closed on shutdown. Cancel futures in scheduledHealthChecks, clear listeners if needed, and close any RemoteNode instances to prevent resource leaks.

java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java [532-535]

 @Override
 public void close() {
   LOG.info("Shutting down LocalNodeRegistry");
+  Lock writeLock = lock.writeLock();
+  writeLock.lock();
+  try {
+    // Cancel scheduled health checks
+    for (ScheduledFuture<?> future : scheduledHealthChecks.values()) {
+      if (future != null) {
+        future.cancel(false);
+      }
+    }
+    scheduledHealthChecks.clear();
+    allChecks.clear();
+    // Close remote nodes
+    for (Node node : nodes.values()) {
+      if (node instanceof RemoteNode) {
+        try {
+          ((RemoteNode) node).close();
+        } catch (Exception e) {
+          LOG.log(Level.WARNING, "Unable to close node properly: " + e.getMessage());
+        }
+      }
+    }
+    nodes.clear();
+  } finally {
+    writeLock.unlock();
+  }
 }

---

Suggestion 5:

Add proper resource disposal handling

The IPv4 fallback listener is not properly disposed if an exception occurs during its operation. Wrap the fallback listener in a using statement or try-finally block to ensure proper resource cleanup.

dotnet/src/webdriver/Internal/PortUtilities.cs [45-53]

 catch (SocketException)
 {
     // If IPv6Any is not supported, fallback to IPv4
     var listener = new TcpListener(IPAddress.Any, 0);
-    listener.Start();
-    int port = ((IPEndPoint)listener.LocalEndpoint).Port;
-    listener.Stop();
-    return port;
+    try
+    {
+        listener.Start();
+        int port = ((IPEndPoint)listener.LocalEndpoint).Port;
+        return port;
+    }
+    finally
+    {
+        listener.Stop();
+    }
 }

---

Suggestion 6:

Use proper resource disposal patterns

The TcpListener resources are not properly disposed in case of exceptions. Use using statements or try-finally blocks to ensure proper cleanup. This prevents resource leaks if exceptions occur between Start() and Stop() calls.

dotnet/src/webdriver/Internal/PortUtilities.cs [38-43]

-var listener = new TcpListener(IPAddress.IPv6Any, 0);
+using var listener = new TcpListener(IPAddress.IPv6Any, 0);
 listener.Server.DualMode = true; // Listen on both IPv4 and IPv6
 listener.Start();
 int port = ((IPEndPoint)listener.LocalEndpoint).Port;
-listener.Stop();
 return port;

---

Suggestion 7:

Ensure proper socket cleanup

The socket resource is not properly cleaned up if an exception occurs after the IPv6 socket creation or during listen/getsockname operations. Use a try-finally block to ensure the socket is always closed, preventing resource leaks.

py/selenium/webdriver/common/utils.py [45-47]

 free_socket = None
 try:
     # IPv4
     free_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
     free_socket.bind(("127.0.0.1", 0))
 except OSError:
     if free_socket:
         free_socket.close()
     # IPv6
     free_socket = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
     free_socket.bind(("::1", 0))
-free_socket.listen(5)
-port: int = free_socket.getsockname()[1]
-free_socket.close()
 
+try:
+    free_socket.listen(5)
+    port: int = free_socket.getsockname()[1]
+finally:
+    free_socket.close()
+

---

Suggestion 8:

Prevent socket resource leak

The IPv4 socket should be closed if binding fails to prevent resource leaks. Currently, if IPv4 socket creation succeeds but binding fails, the socket remains open when the exception is caught.

py/selenium/webdriver/common/utils.py [34-44]

+free_socket = None
 try:
     # IPv4
     free_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
     free_socket.bind(("127.0.0.1", 0))
 except OSError:
+    if free_socket:
+        free_socket.close()
     # IPv6
     free_socket = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
     free_socket.bind(("::1", 0))
 free_socket.listen(5)
 port: int = free_socket.getsockname()[1]
 free_socket.close()

---

Suggestion 9:

Properly stop driver before nullifying

Setting the global variable to None without properly stopping the driver can lead to resource leaks. The driver should be stopped before nullifying the reference to ensure proper cleanup.

py/conftest.py [344-345]

 if request.node.get_closest_marker("no_driver_after_test"):
+    if selenium_driver is not None:
+        selenium_driver.stop_driver()
     selenium_driver = None

---

Pattern 2: Guard external API and I/O operations with targeted validation and fallbacks (e.g., WebSocket ctor args, regex match results, IPv4/IPv6 binding) to avoid crashes and surface clear errors.

Example code before:

def __init__(self, url, timeout, interval):
    self.url = url
    self.response_wait_timeout = timeout
    self.response_wait_interval = interval

Example code after:

def __init__(self, url, timeout, interval):
    if not url or timeout is None or timeout <= 0 or interval is None or interval <= 0:
        raise ValueError("Invalid WebSocket parameters")
    self.url = url
    self.response_wait_timeout = float(timeout)
    self.response_wait_interval = float(interval)

Relevant past accepted suggestions:

---

Suggestion 1:

Validate WebSocket constructor inputs

Validate the constructor parameters to ensure they are present and of the expected types/ranges. Default or raise clear errors for invalid values to prevent subtle runtime issues.

py/selenium/webdriver/remote/websocket_connection.py [31-39]

 class WebSocketConnection:
     _max_log_message_size = 9999
 
     def __init__(self, url, timeout, interval):
+        if not url:
+            raise ValueError("WebSocket URL must be provided")
+        if timeout is None or timeout <= 0:
+            raise ValueError("timeout must be a positive number")
+        if interval is None or interval <= 0:
+            raise ValueError("interval must be a positive number")
+
         self.callbacks = {}
         self.session_id = None
         self.url = url
-        self.response_wait_timeout = timeout
-        self.response_wait_interval = interval
+        self.response_wait_timeout = float(timeout)
+        self.response_wait_interval = float(interval)

---

Suggestion 2:

Handle potential None from regex

The regex search could return None if the pattern doesn't match, causing an AttributeError when accessing index [1]. Add a check to handle cases where the URL format doesn't match the expected pattern.

scripts/update_multitool_binaries.py [40-42]

-version = re.search(f"download/(.*?)/{tool}", data[tool]["binaries"][0]["url"])[
-    1
-]
+version_match = re.search(f"download/(.*?)/{tool}", data[tool]["binaries"][0]["url"])
+if not version_match:
+    continue
+version = version_match[1]

---

Suggestion 3:

Handle IPv6 binding failures gracefully

Add a try-except block around the IPv6 socket creation and binding to handle cases where IPv6 is also unavailable, preventing potential unhandled exceptions that could crash the function.

py/selenium/webdriver/common/utils.py [34-44]

 free_socket = None
 try:
     # IPv4
     free_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
     free_socket.bind(("127.0.0.1", 0))
 except OSError:
     if free_socket:
         free_socket.close()
-    # IPv6
-    free_socket = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
-    free_socket.bind(("::1", 0))
+    try:
+        # IPv6
+        free_socket = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
+        free_socket.bind(("::1", 0))
+    except OSError:
+        raise RuntimeError("Unable to bind to both IPv4 and IPv6")

---

Suggestion 4:

Add fallback for None reason

The response.reason might be None in some cases, which would result in "None" being displayed. Add a fallback to handle cases where the reason is not available.

py/selenium/webdriver/remote/remote_connection.py [443]

-return {"status": statuscode, "value": f"{response.reason}" if not data else data.strip()}
+return {"status": statuscode, "value": f"{response.reason or statuscode}" if not data else data.strip()}

---

Suggestion 5:

Add parameter null validation

Validate inputs before using them. Add null checks for nodeUri, id, and availability to avoid potential NullPointerExceptions during logging and model updates.

java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java [292-304]

 public void updateNodeAvailability(URI nodeUri, NodeId id, Availability availability) {
+  Require.nonNull("Node URI", nodeUri);
+  Require.nonNull("Node ID", id);
+  Require.nonNull("Availability", availability);
   Lock writeLock = lock.writeLock();
   writeLock.lock();
   try {
     LOG.log(
         getDebugLogLevel(),
         String.format("Health check result for %s was %s", nodeUri, availability));
     model.setAvailability(id, availability);
     model.updateHealthCheckCount(id, availability);
   } finally {
     writeLock.unlock();
   }
 }

---

Suggestion 6:

Add missing coordinates/error validation

Add validation to ensure at least one of coordinates or error is specified. Currently, both can be null which would result in an invalid geolocation override request.

java/src/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideParameters.java [41-50]

 if (this.coordinates != null && this.error != null) {
   throw new IllegalArgumentException("Cannot specify both coordinates and error");
+}
+if (this.coordinates == null && this.error == null) {
+  throw new IllegalArgumentException("Must specify either coordinates or error");
 }
 if (this.contexts != null && this.userContexts != null) {
   throw new IllegalArgumentException("Cannot specify both contexts and userContexts");
 }
 
 if (this.contexts == null && this.userContexts == null) {
   throw new IllegalArgumentException("Must specify either contexts or userContexts");
 }

---

Suggestion 7:

Add null validation for directory

The currentDirectory variable should be validated for null before being used in Path.Combine() calls. Add a null check to prevent potential NullReferenceException if AppContext.BaseDirectory returns null.

dotnet/src/webdriver/SeleniumManager.cs [82-89]

+ArgumentNullException.ThrowIfNull(currentDirectory);
+
 binaryFullPath = platform switch
 {
     SupportedPlatform.Windows => Path.Combine(currentDirectory, "runtimes", "win", "native", "selenium-manager.exe"),
     SupportedPlatform.Linux => Path.Combine(currentDirectory, "runtimes", "linux", "native", "selenium-manager"),
     SupportedPlatform.MacOS => Path.Combine(currentDirectory, "runtimes", "osx", "native", "selenium-manager"),
     _ => throw new PlatformNotSupportedException(
         $"Selenium Manager doesn't support your runtime platform: {RuntimeInformation.OSDescription}"),
 };

---

Pattern 3: Clamp numeric configuration values to safe bounds before casting or combining (e.g., heartbeat intervals, timeouts) to prevent overflow and enforce reasonable ranges.

Example code before:

int_ivl = int(duration.to_millis())
socket.setHeartbeatIvl(int_ivl)
socket.setHeartbeatTimeout(int_ivl * 3)

Example code after:

ms = duration.to_millis()
ms = max(MIN_MS, min(ms, MAX_MS))
ivl = int(ms)
socket.setHeartbeatIvl(ivl)
socket.setHeartbeatTimeout(ivl * 3)

Relevant past accepted suggestions:

---

Suggestion 1:

Prevent heartbeat interval overflow

** * Configures ZeroMQ heartbeat settings on a socket to prevent stale connections.

• • The heartbeat interval is clamped between 1 second and ~24 days to prevent integer overflow

• • and ensure reasonable values. If the provided duration is outside this range, it will be
• • adjusted and a warning will be logged.
  • @param socket The ZMQ socket to configure
  • @param heartbeatPeriod The heartbeat interval duration @@ -39,14 +46,58 @@ */ static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) { if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {

•  int heartbeatIvl = (int) heartbeatPeriod.toMillis();
  

•  long heartbeatMs = heartbeatPeriod.toMillis();
  

•  long clampedHeartbeatMs = clampHeartbeatInterval(heartbeatMs, socketType);
  

•  // Safe to cast to int now
  

•  int heartbeatIvl = (int) clampedHeartbeatMs;
  

•  int heartbeatTimeout = heartbeatIvl * 3;
  

•  int heartbeatTtl = heartbeatIvl * 6;
  

•  socket.setHeartbeatIvl(heartbeatIvl);
  

•  socket.setHeartbeatTimeout(heartbeatIvl * 3);
  

•  socket.setHeartbeatTtl(heartbeatIvl * 6);
  

•  socket.setHeartbeatTimeout(heartbeatTimeout);
  

•  socket.setHeartbeatTtl(heartbeatTtl);
  

•  LOG.info(
       String.format(
  

•          "ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
  

•          socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
  

•          "ZMQ %s socket heartbeat configured: interval=%ds, timeout=%ds, ttl=%ds",
  

•          socketType, heartbeatIvl / 1000, heartbeatTimeout / 1000, heartbeatTtl / 1000));
  

  } }
• /**
• • Clamps the heartbeat interval to safe bounds and logs warnings if adjustments are made.
• • @param heartbeatMs The heartbeat interval in milliseconds
• • @param socketType The socket type for logging
• • @return The clamped heartbeat interval
• */
• private static long clampHeartbeatInterval(long heartbeatMs, String socketType) {
• if (heartbeatMs < MIN_HEARTBEAT_MS) {

•  logHeartbeatClampWarning(socketType, heartbeatMs, MIN_HEARTBEAT_MS, "below minimum");
  

•  return MIN_HEARTBEAT_MS;
  

• }
• if (heartbeatMs > MAX_HEARTBEAT_MS) {

•  logHeartbeatClampWarning(socketType, heartbeatMs, MAX_HEARTBEAT_MS, "exceeds maximum");
  

•  return MAX_HEARTBEAT_MS;
  

• }
• return heartbeatMs;
• }
• /**
• • Logs a warning when the heartbeat interval is clamped.
• • @param socketType The socket type
• • @param originalMs The original interval value in milliseconds
• • @param clampedMs The clamped interval value in milliseconds
• • @param reason The reason for clamping
• */
• private static void logHeartbeatClampWarning(

•  String socketType, long originalMs, long clampedMs, String reason) {
  

• LOG.log(

•    Level.WARNING,
  

•    String.format(
  

•        "ZMQ %s socket heartbeat interval %ds %s %ds, clamping to %ds",
  

•        socketType, originalMs / 1000, reason, clampedMs / 1000, clampedMs / 1000));
  

• }

**In configureHeartbeat, guard against integer overflow by clamping the heartbeat interval in milliseconds to a safe maximum before casting to int and setting socket options. Also, enforce a sensible minimum value.**

[java/src/org/openqa/selenium/events/zeromq/ZmqUtils.java [40-52]](https://github.com/SeleniumHQ/selenium/pull/16444/files#diff-f9f1bd9c11ff8d0995d73085dc4b3e906a8aa15c1a5ba90fc6b9059a3a8080a1R40-R52)

```diff
 static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
-  if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
-    int heartbeatIvl = (int) heartbeatPeriod.toMillis();
-    socket.setHeartbeatIvl(heartbeatIvl);
-    socket.setHeartbeatTimeout(heartbeatIvl * 3);
-    socket.setHeartbeatTtl(heartbeatIvl * 6);
-    LOG.info(
-        String.format(
-            "ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
-            socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+  if (heartbeatPeriod == null || heartbeatPeriod.isZero() || heartbeatPeriod.isNegative()) {
+    return;
   }
+  long ivlMsLong = heartbeatPeriod.toMillis();
+  // Enforce sane bounds: at least 100ms and at most Integer.MAX_VALUE / 6 to avoid overflow below
+  long min = 100L;
+  long max = Integer.MAX_VALUE / 6L;
+  if (ivlMsLong < min) {
+    ivlMsLong = min;
+  } else if (ivlMsLong > max) {
+    ivlMsLong = max;
+  }
+  int heartbeatIvl = (int) ivlMsLong;
+  int heartbeatTimeout = heartbeatIvl * 3;
+  int heartbeatTtl = heartbeatIvl * 6;
+  socket.setHeartbeatIvl(heartbeatIvl);
+  socket.setHeartbeatTimeout(heartbeatTimeout);
+  socket.setHeartbeatTtl(heartbeatTtl);
+  LOG.info(
+      String.format(
+          "ZMQ %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
+          socketType, heartbeatIvl, heartbeatTimeout, heartbeatTtl));
 }

---

Suggestion 2:

Refactor duplicated code into a utility method

Extract the duplicated configureHeartbeat method from BoundZmqEventBus and UnboundZmqEventBus into a static helper method in a new utility class to avoid code repetition and improve maintainability.

java/src/org/openqa/selenium/events/zeromq/BoundZmqEventBus.java [84-97]

-private void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
-  if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
-    int heartbeatIvl = (int) heartbeatPeriod.toMillis();
-    socket.setHeartbeatIvl(heartbeatIvl);
-    // Set heartbeat timeout to 3x the interval
-    socket.setHeartbeatTimeout(heartbeatIvl * 3);
-    // Set heartbeat TTL to 6x the interval
-    socket.setHeartbeatTtl(heartbeatIvl * 6);
-    LOG.info(
-        String.format(
-            "Event bus %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
-            socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+// In a new file: java/src/org/openqa/selenium/events/zeromq/ZmqUtils.java
+package org.openqa.selenium.events.zeromq;
+
+import java.time.Duration;
+import java.util.logging.Logger;
+import org.zeromq.ZMQ;
+
+class ZmqUtils {
+  private static final Logger LOG = Logger.getLogger(ZmqUtils.class.getName());
+
+  private ZmqUtils() {
+    // Utility class
+  }
+
+  static void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
+    if (heartbeatPeriod != null && !heartbeatPeriod.isZero() && !heartbeatPeriod.isNegative()) {
+      long periodMillis = heartbeatPeriod.toMillis();
+      if (periodMillis > Integer.MAX_VALUE) {
+        LOG.warning(
+            String.format(
+                "Heartbeat period %dms is too large. Capping at %dms.",
+                periodMillis, Integer.MAX_VALUE));
+        periodMillis = Integer.MAX_VALUE;
+      }
+      int heartbeatIvl = (int) periodMillis;
+      socket.setHeartbeatIvl(heartbeatIvl);
+      // Set heartbeat timeout to 3x the interval
+      socket.setHeartbeatTimeout(heartbeatIvl * 3);
+      // Set heartbeat TTL to 6x the interval
+      socket.setHeartbeatTtl(heartbeatIvl * 6);
+      LOG.info(
+          String.format(
+              "Event bus %s socket heartbeat configured: interval=%dms, timeout=%dms, ttl=%dms",
+              socketType, heartbeatIvl, heartbeatIvl * 3, heartbeatIvl * 6));
+    }
   }
 }
 
+// In BoundZmqEventBus.java, replace the configureHeartbeat method with:
+private void configureHeartbeat(ZMQ.Socket socket, Duration heartbeatPeriod, String socketType) {
+  ZmqUtils.configureHeartbeat(socket, heartbeatPeriod, socketType);
+}
+

---

Suggestion 3:

Simplify heartbeat timeout math

Use consistent and clearer time calculations; avoid chained multiply/divide on Duration which can be error-prone. Replace with a single multiplication by half the multiplier to match intent and readability.

java/src/org/openqa/selenium/grid/distributor/local/LocalGridModel.java [245-247]

 Instant lostTime =
-    lastTouched.plus(
-        node.getHeartbeatPeriod().multipliedBy(PURGE_TIMEOUT_MULTIPLIER).dividedBy(2));
+    lastTouched.plus(node.getHeartbeatPeriod().multipliedBy(PURGE_TIMEOUT_MULTIPLIER / 2));

---

Suggestion 4:

Fix integer division in timing

The division PURGE_TIMEOUT_MULTIPLIER / 2 performs integer division; for odd values it truncates and may degrade timing logic. Compute using long arithmetic to preserve intended scaling, e.g., multiply first or use exact constants.

java/src/org/openqa/selenium/grid/distributor/local/LocalGridModel.java [245-248]

 Instant lostTime =
-    lastTouched.plus(node.getHeartbeatPeriod().multipliedBy(PURGE_TIMEOUT_MULTIPLIER / 2));
+    lastTouched.plus(node.getHeartbeatPeriod().multipliedBy((long) PURGE_TIMEOUT_MULTIPLIER).dividedBy(2));
 Instant deadTime =
-    lastTouched.plus(node.getHeartbeatPeriod().multipliedBy(PURGE_TIMEOUT_MULTIPLIER));
+    lastTouched.plus(node.getHeartbeatPeriod().multipliedBy((long) PURGE_TIMEOUT_MULTIPLIER));

---

Pattern 4: Cache/memoize expensive or connection-creating operations so they are initialized once and reused, using thread-safe patterns or suppliers.

Example code before:

def get_connection():
    return Connection(create_client(), uri)

Example code after:

from functools import lru_cache
@lru_cache(maxsize=1)
def get_connection():
    return Connection(create_client(), uri)

Relevant past accepted suggestions:

---

Suggestion 1:

Prevent resource leaks with caching

Cache the BiDi instance to prevent creating a new HTTP client and connection on every invocation. This avoids potential resource leaks and performance issues.

java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-56]

-return () -> {
-  URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
-
-  HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
-  ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
-  HttpClient wsClient = clientFactory.createClient(wsConfig);
-  Connection connection = new Connection(wsClient, wsUri.toString());
-
-  return Optional.of(new BiDi(connection));
+return new HasBiDi() {
+  private volatile Optional<BiDi> cachedBiDi;
+  
+  @Override
+  public Optional<BiDi> getBiDi() {
+    if (cachedBiDi == null) {
+      synchronized (this) {
+        if (cachedBiDi == null) {
+          URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
+          HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
+          ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
+          HttpClient wsClient = clientFactory.createClient(wsConfig);
+          Connection connection = new Connection(wsClient, wsUri.toString());
+          cachedBiDi = Optional.of(new BiDi(connection));
+        }
+      }
+    }
+    return cachedBiDi;
+  }
 };

---

Suggestion 2:

Prevent connection recreation with caching

Cache the DevTools instance to avoid recreating connections and performing version matching on every call. This prevents potential resource leaks and unnecessary overhead.

java/src/org/openqa/selenium/devtools/DevToolsProvider.java [45-53]

-return () -> {
-  Object cdpVersion = caps.getCapability("se:cdpVersion");
-  String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion();
-
-  CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new);
-  Optional<DevTools> devTools =
-    SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn));
-  return devTools;
+return new HasDevTools() {
+  private volatile Optional<DevTools> cachedDevTools;
+  
+  @Override
+  public Optional<DevTools> getDevTools() {
+    if (cachedDevTools == null) {
+      synchronized (this) {
+        if (cachedDevTools == null) {
+          Object cdpVersion = caps.getCapability("se:cdpVersion");
+          String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion();
+          CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new);
+          cachedDevTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn));
+        }
+      }
+    }
+    return cachedDevTools;
+  }
 };

---

Suggestion 3:

Prevent multiple connection creation

To prevent resource leaks from multiple connections, cache the BiDi instance so it is created only once. The current implementation creates a new connection every time getBiDi() is called.

java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-56]

-return () -> {
-  URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
-
-  HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
-  ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
-  HttpClient wsClient = clientFactory.createClient(wsConfig);
-  Connection connection = new Connection(wsClient, wsUri.toString());
-
-  return Optional.of(new BiDi(connection));
+return new HasBiDi() {
+  private volatile Optional<BiDi> biDi;
+  
+  @Override
+  public Optional<BiDi> getBiDi() {
+    if (biDi == null) {
+      synchronized (this) {
+        if (biDi == null) {
+          URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
+          HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
+          ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
+          HttpClient wsClient = clientFactory.createClient(wsConfig);
+          Connection connection = new Connection(wsClient, wsUri.toString());
+          biDi = Optional.of(new BiDi(connection));
+        }
+      }
+    }
+    return biDi;
+  }
 };

---

Suggestion 4:

Simplify lazy-loading with a memoized supplier

Replace the manual double-checked locking implementation with Guava's Suppliers.memoize to simplify the code and improve robustness for lazy initialization.

java/src/org/openqa/selenium/bidi/BiDiProvider.java [47-68]

 return new HasBiDi() {
-  private volatile Optional<BiDi> biDi;
+  private final Supplier<Optional<BiDi>> biDiSupplier =
+      Suppliers.memoize(
+          () -> {
+            URI wsUri =
+                getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
+
+            HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
+            ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
+            HttpClient wsClient = clientFactory.createClient(wsConfig);
+            Connection connection = new Connection(wsClient, wsUri.toString());
+
+            return Optional.of(new BiDi(connection));
+          });
 
   @Override
   public Optional<BiDi> maybeGetBiDi() {
-    if (biDi == null) {
-      synchronized (this) {
-        if (biDi == null) {
-          URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
-
-          HttpClient.Factory clientFactory = HttpClient.Factory.createDefault();
-          ClientConfig wsConfig = ClientConfig.defaultConfig().baseUri(wsUri);
-          HttpClient wsClient = clientFactory.createClient(wsConfig);
-          Connection connection = new Connection(wsClient, wsUri.toString());
-
-          biDi = Optional.of(new BiDi(connection));
-        }
-      }
-    }
-    return biDi;
+    return biDiSupplier.get();
   }
 };

---

Suggestion 5:

Use initialized Optional with DCL

Initialize devTools to Optional.empty() and use a local variable in the double-checked locking to avoid null checks and publication races.

java/src/org/openqa/selenium/devtools/DevToolsProvider.java [45-64]

 return new HasDevTools() {
-  private volatile Optional<DevTools> devTools;
+  private volatile Optional<DevTools> devTools = Optional.empty();
 
   @Override
   public Optional<DevTools> maybeGetDevTools() {
-    if (devTools == null) {
+    Optional<DevTools> local = devTools;
+    if (local.isEmpty()) {
       synchronized (this) {
-        if (devTools == null) {
+        local = devTools;
+        if (local.isEmpty()) {
           Object cdpVersion = caps.getCapability("se:cdpVersion");
           String version =
             cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion();
 
           CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new);
-          this.devTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn));
+          local = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn));
+          devTools = local;
         }
       }
     }
-    return devTools;
+    return local;
   }
 };

---

Pattern 5: Enforce accurate and consistent documentation and naming (docstrings/Javadoc/event names/param names) to match actual behavior and APIs, aiding tooling and maintainability.

Example code before:

def web_socket_url() -> bool:
    """Returns the WebSocket URL."""
    ...

Example code after:

def web_socket_url() -> str:
    """Returns the WebSocket URL string."""

Relevant past accepted suggestions:

---

Suggestion 1:

Correct docstring return type

Correct the return type in the web_socket_url docstring from bool to str to accurately reflect that it returns a URL.

py/selenium/webdriver/common/options.py [316-328]

 web_socket_url = _BaseOptionsDescriptor("webSocketUrl")
 """Gets and Sets WebSocket URL.
 
 Usage:
     - Get: `self.web_socket_url`
     - Set: `self.web_socket_url = value`
 
 Args:
     value: str
 
 Returns:
-    bool when getting, None when setting.
+    str when getting, None when setting.
 """

---

Suggestion 2:

Fix incorrect Javadoc reference

The Javadoc text references TargetLocator#alert(), which is unrelated to element screenshots, and misuses "@see" in a @param. Update the description to reflect WebElement.getScreenshotAs and fix parameter docs and punctuation.

java/src/org/openqa/selenium/support/events/WebDriverListener.java [613-622]

 /**
- * This action will be performed each time after {@link WebDriver.TargetLocator#alert()} is
- * called.
+ * This method will be called after {@link WebElement#getScreenshotAs(OutputType)} is called.
  *
- * @param element - decorated WebElement instance
- * @param target - target type, @see OutputType
- * @param result - object in which is stored information about the screenshot.
- * @param <X> - return type for getScreenshotAs.
+ * @param element the decorated WebElement instance
+ * @param target the target type, {@link OutputType}
+ * @param result the screenshot result
+ * @param <X> the return type for getScreenshotAs
  */
 default <X> void afterGetScreenshotAs(WebElement element, OutputType<X> target, X result) {}

---

Suggestion 3:

Correct Javadoc parameter links

**

• • This action will be performed each time after {@link WebDriver.TargetLocator#alert()} is
• • called.

• • This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
  • @param element - decorated WebElement instance
  • @param target - target type, @see OutputType

**In Javadoc, avoid using "@see" within @param descriptions; link the type directly with {@link}. This improves clarity and tooling parsing. Update the parameter docs to use proper links and consistent punctuation.**

[java/src/org/openqa/selenium/support/events/WebDriverListener.java [335-343]](https://github.com/SeleniumHQ/selenium/pull/16233/files#diff-27fd3dec4abffb9b78d39c4f8ca918bcb5cd04926e3d252d2398f561f5ad61ffR335-R343)

```diff
 /**
- * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is
- * called.
+ * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
  *
- * @param driver - decorated WebDriver instance
- * @param target - target type, @see OutputType
- * @param <X> - return type for getScreenshotAs.
+ * @param driver the decorated WebDriver instance
+ * @param target the target type, {@link OutputType}
+ * @param <X> the return type for getScreenshotAs
  */
 default <X> void beforeGetScreenshotAs(WebDriver driver, OutputType<X> target) {}

---

Suggestion 4:

Clean up Javadoc references

** * This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called. *

• • @param driver - decorated WebDriver instance
• • @param target - target type, @see OutputType
• • @param result - object in which is stored information about the screenshot.
• • @param - return type for getScreenshotAs.

• • @param driver decorated WebDriver instance
• • @param target target type, see {@link OutputType}
• • @param result object that stores the screenshot information
• • @param return type for getScreenshotAs */ default void afterGetScreenshotAs(WebDriver driver, OutputType target, X result) {}

@@ -607,19 +607,19 @@ * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is * called. *

• • @param element - decorated WebElement instance
• • @param target - target type, @see OutputType
• • @param - return type for getScreenshotAs.

• • @param element decorated WebElement instance
• • @param target target type, see {@link OutputType}
• • @param return type for getScreenshotAs */ default void beforeGetScreenshotAs(WebElement element, OutputType target) {}

/** * This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called. *

• • @param element - decorated WebElement instance
• • @param target - target type, @see OutputType
• • @param result - object in which is stored information about the screenshot.
• • @param - return type for getScreenshotAs.

• • @param element decorated WebElement instance
• • @param target target type, see {@link OutputType}
• • @param result result object that stores the screenshot information
• • @param return type for getScreenshotAs

**Replace "@see" inline in @param descriptions with proper Javadoc links or move them to @see tags. Also clarify generic return type wording and remove trailing periods in parameter descriptions for consistency.**

[java/src/org/openqa/selenium/support/events/WebDriverListener.java [335-624]](https://github.com/SeleniumHQ/selenium/pull/16233/files#diff-27fd3dec4abffb9b78d39c4f8ca918bcb5cd04926e3d252d2398f561f5ad61ffR335-R624)

```diff
 /**
- * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is
- * called.
+ * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
  *
- * @param driver - decorated WebDriver instance
- * @param target - target type, @see OutputType
- * @param <X> - return type for getScreenshotAs.
+ * @param driver decorated WebDriver instance
+ * @param target target type, see {@link OutputType}
+ * @param <X> return type for getScreenshotAs
  */
 default <X> void beforeGetScreenshotAs(WebDriver driver, OutputType<X> target) {}
 
 /**
  * This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
  *
- * @param driver - decorated WebDriver instance
- * @param target - target type, @see OutputType
- * @param result - object in which is stored information about the screenshot.
- * @param <X> - return type for getScreenshotAs.
+ * @param driver decorated WebDriver instance
+ * @param target target type, see {@link OutputType}
+ * @param result object that stores the screenshot information
+ * @param <X> return type for getScreenshotAs
  */
 default <X> void afterGetScreenshotAs(WebDriver driver, OutputType<X> target, X result) {}
 
 /**
- * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is
- * called.
+ * This method will be called before {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
  *
- * @param element - decorated WebElement instance
- * @param target - target type, @see OutputType
- * @param <X> - return type for getScreenshotAs.
+ * @param element decorated WebElement instance
+ * @param target target type, see {@link OutputType}
+ * @param <X> return type for getScreenshotAs
  */
 default <X> void beforeGetScreenshotAs(WebElement element, OutputType<X> target) {}
 
 /**
  * This method will be called after {@link TakesScreenshot#getScreenshotAs(OutputType)} is called.
  *
- * @param element - decorated WebElement instance
- * @param target - target type, @see OutputType
- * @param result - object in which is stored information about the screenshot.
- * @param <X> - return type for getScreenshotAs.
+ * @param element decorated WebElement instance
+ * @param target target type, see {@link OutputType}
+ * @param result object that stores the screenshot information
+ * @param <X> return type for getScreenshotAs
  */
 default <X> void afterGetScreenshotAs(WebElement element, OutputType<X> target, X result) {}

---

Suggestion 5:

Fix misleading parameter name

The parameter name driver for WebElement callbacks is misleading and contradicts the interface, risking confusion and incorrect usage. Rename it to element to align with WebDriverListener method signatures and improve test clarity.

java/test/org/openqa/selenium/support/events/EventFiringDecoratorTest.java [476-483]

-public <X> void beforeGetScreenshotAs(WebElement driver, OutputType<X> target) {
+public <X> void beforeGetScreenshotAs(WebElement element, OutputType<X> target) {
   acc.append("beforeGetScreenshotAs ").append(target).append("\n");
 }
 
 @Override
-public <X> void afterGetScreenshotAs(WebElement driver, OutputType<X> target, X result) {
+public <X> void afterGetScreenshotAs(WebElement element, OutputType<X> target, X result) {
   acc.append("afterGetScreenshotAs ").append(target).append(" ").append(result).append("\n");
 }

---

Suggestion 6:

Fix inconsistent handler naming

In BrowsingContextInspector, rename the downloadWillEndMapper and downloadWillEndEvent fields to downloadEndMapper and downloadEndEvent to match the event name browsingContext.downloadEnd, and update their usages accordingly.

java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java [68-186]

-private final Function<Map<String, Object>, DownloadEnded> downloadWillEndMapper =
+private final Function<Map<String, Object>, DownloadEnded> downloadEndMapper =
     params -> {
       try (StringReader reader = new StringReader(JSON.toJson(params));
-          JsonInput input = JSON.newInput(reader)) {
+           JsonInput input = JSON.newInput(reader)) {
         return input.read(DownloadEnded.class);
       }
     };
 
-...
-
-private final Event<DownloadEnded> downloadWillEndEvent =
-    new Event<>("browsingContext.downloadEnd", downloadWillEndMapper);
-
-...
+private final Event<DownloadEnded> downloadEndEvent =
+    new Event<>("browsingContext.downloadEnd", downloadEndMapper);
 
 public void onDownloadEnd(Consumer<DownloadEnded> consumer) {
   if (browsingContextIds.isEmpty()) {
-    this.bidi.addListener(downloadWillEndEvent, consumer);
+    this.bidi.addListener(downloadEndEvent, consumer);
   } else {
-    this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer);
+    this.bidi.addListener(browsingContextIds, downloadEndEvent, consumer);
   }
 }
 
+@Override
+public void close() {
+  this.bidi.clearListener(browsingContextCreated);
+  this.bidi.clearListener(browsingContextDestroyed);
+  this.bidi.clearListener(userPromptOpened);
+  this.bidi.clearListener(userPromptClosed);
+  this.bidi.clearListener(historyUpdated);
+  this.bidi.clearListener(downloadWillBeginEvent);
+  this.bidi.clearListener(downloadEndEvent);
+
+  navigationEventSet.forEach(this.bidi::clearListener);
+}
+

---

Suggestion 7:

Use accurate event naming

Rename downloadWillEndEvent to reflect the actual event name (downloadEnd) for clarity and consistency with other events. This avoids confusion and aligns with event semantics.

java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java [97-247]

-private final Event<DownloadEnded> downloadWillEndEvent =
+private final Event<DownloadEnded> downloadEndEvent =
     new Event<>("browsingContext.downloadEnd", downloadWillEndMapper);
 
 ...
 
 public void onDownloadEnd(Consumer<DownloadEnded> consumer) {
   if (browsingContextIds.isEmpty()) {
-    this.bidi.addListener(downloadWillEndEvent, consumer);
+    this.bidi.addListener(downloadEndEvent, consumer);
   } else {
-    this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer);
+    this.bidi.addListener(browsingContextIds, downloadEndEvent, consumer);
   }
 }
 
 ...
 
 public void close() {
   this.bidi.clearListener(browsingContextCreated);
   this.bidi.clearListener(browsingContextDestroyed);
   this.bidi.clearListener(userPromptOpened);
   this.bidi.clearListener(userPromptClosed);
   this.bidi.clearListener(historyUpdated);
   this.bidi.clearListener(downloadWillBeginEvent);
-  this.bidi.clearListener(downloadWillEndEvent);
+  this.bidi.clearListener(downloadEndEvent);
 
   navigationEventSet.forEach(this.bidi::clearListener);
 }

---

[Auto-generated best practices - 2025-10-29]