make augmentation of HasBiDi/HasDevTools lazy-loaded

[asolntsev]

User description

When CDP url is not accessible, the command new Augmenter().augment(remoteWebDriver) fails immediately - even if I don't want to use CDP or BiDi.

🔗 Related Issues

This PR fixes the problem described in #9803, #10132, selenide/selenide#2797, selenide/selenide#3107 etc.

💥 What does this PR do?

This PR fixes DevToolsProvider and BiDiProvider, so that augmentation of HasBiDi and HasDevTools interfaces is now lazy-loaded.

Context:

1. We run a remote Chrome driver in TestContainer / Selenoid / Moon / Selenium Grid
2. The CDP url is not accessible (because it contains the port which is accessible only inside the docker)
3. BUT I don't want to use CDP or BiDi.
4. I use Augmenter to cast my webdriver, say, to JavascriptExecutor:

WebDriver wd = new Augmenter().augment(remoteWebDriver);  // FAILS
JavascriptExecutor js = (JavascriptExecutor) wd;

This code fails because of CDP connectivity issue during augmentation:

org.openqa.selenium.remote.http.ConnectionFailedException: JdkWebSocket initial request execution error
Build info: version: '4.35.0', revision: '1c58e5028b'
System info: os.name: 'Mac OS X', os.arch: 'aarch64', os.version: '26.0', java.version: '17.0.14'
Driver info: driver.version: unknown
	at org.openqa.selenium.remote.http.jdk.JdkHttpClient.openSocket(JdkHttpClient.java:253)
	at org.openqa.selenium.devtools.Connection.<init>(Connection.java:89)
	at org.openqa.selenium.devtools.SeleniumCdpConnection.<init>(SeleniumCdpConnection.java:36)
	at org.openqa.selenium.devtools.SeleniumCdpConnection.lambda$create$2(SeleniumCdpConnection.java:103)
	at java.base/java.util.Optional.map(Optional.java:260)
	at org.openqa.selenium.devtools.SeleniumCdpConnection.create(SeleniumCdpConnection.java:103)
	at org.openqa.selenium.devtools.SeleniumCdpConnection.create(SeleniumCdpConnection.java:49)
	at org.openqa.selenium.devtools.DevToolsProvider.getImplementation(DevToolsProvider.java:50)
	at org.openqa.selenium.devtools.DevToolsProvider.getImplementation(DevToolsProvider.java:29)
	at org.openqa.selenium.remote.Augmenter.augment(Augmenter.java:202)
	at org.openqa.selenium.remote.Augmenter.augment(Augmenter.java:173)

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix

---

Description

• Make HasBiDi and HasDevTools augmentation lazy-loaded

• Fix connection failures when CDP/BiDi URLs are inaccessible

• Enable augmentation to succeed even without CDP/BiDi access

• Implement double-checked locking pattern for thread safety

---

Diagram Walkthrough

flowchart LR
  A["Augmenter.augment()"] --> B["DevToolsProvider/BiDiProvider"]
  B --> C["Lazy Implementation"]
  C --> D["Connection Created Only When Accessed"]
  D --> E["No Immediate Connection Failure"]

Loading

File Walkthrough

Relevant files

Bug fix

BiDiProvider.java Implement lazy BiDi connection initialization                        java/src/org/openqa/selenium/bidi/BiDiProvider.java Replace immediate BiDi connection creation with lazy-loaded implementation Add double-checked locking pattern for thread-safe initialization Move connection logic inside maybeGetBiDi() method Make getBiDiUrl() method static +20/-7    DevToolsProvider.java Implement lazy DevTools connection initialization                java/src/org/openqa/selenium/devtools/DevToolsProvider.java Replace immediate DevTools connection creation with lazy-loaded implementation Add double-checked locking pattern for thread-safe initialization Move CDP connection logic inside maybeGetDevTools() method Defer version detection and connection creation until first access +18/-6

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 9803 - Partially compliant Compliant requirements: Avoid premature failures when DevTools is not intended to be used. Ensure augmentation works so DevTools can be obtained when supported. Non-compliant requirements: Allow using DevTools with RemoteWebDriver in Grid without ClassCastException when casting to HasDevTools. Requires further human verification: Validate at runtime that casting to HasDevTools on an augmented RemoteWebDriver succeeds in a Selenium Grid and DevTools session can be created when CDP is reachable. Confirm no connection attempt happens during augmentation, only upon first use, by running in a Docker/grid with inaccessible CDP endpoint. 10132 - Partially compliant Compliant requirements: Do not require CDP connectivity at augmentation time. Non-compliant requirements: Fix ClassCastException when casting augmented RemoteWebDriver to HasDevTools with Augmenter. Ensure DevTools acquisition works with selenium/standalone-chrome. Requires further human verification: Run the provided snippet against selenium/standalone-chrome and verify casting and devTools.createSession() succeed when CDP is available, and that augmentation itself does not fail when CDP is not available. 2797 - Not compliant Non-compliant requirements: WebDriverEventListener afterScript should include the return value of executed scripts. 3107 - Not compliant Non-compliant requirements: Handle IE alert interrupting test execution and proceed without interruption. 5678 - Not compliant Non-compliant requirements: Resolve ChromeDriver connection failures on subsequent instantiations (ConnectFailure). 1234 - Not compliant Non-compliant requirements: Ensure Selenium 2.48 triggers javascript in link href on click() in Firefox.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The lambda captures capabilities and constructs a new WebSocket client each time HasBiDi#getBiDi() is called; verify this does not create multiple connections per driver instance or leak resources. Consider memoizing the created BiDi instance. public HasBiDi getImplementation(Capabilities caps, ExecuteMethod executeMethod) { 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)); }; Behavior Change DevTools creation is deferred; ensure that exceptions for unsupported CDP are still clear and that repeated calls do not recreate connections. Consider caching the Optional to avoid redundant connection attempts. public HasDevTools getImplementation(Capabilities caps, ExecuteMethod executeMethod) { 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; };

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Prevent resource leaks with caching Suggestion Impact: The commit implemented caching of a single BiDi instance with double-checked locking and a separate lock object, avoiding repeated client/connection creation. It also split maybeGetBiDi and getBiDi methods and stored BiDi directly instead of Optional. code diff: @Override public HasBiDi getImplementation(Capabilities caps, ExecuteMethod executeMethod) { return new HasBiDi() { - private volatile Optional<BiDi> biDi; + private volatile BiDi biDi; + private final Object lock = new Object(); @Override public Optional<BiDi> maybeGetBiDi() { + return Optional.ofNullable(biDi); + } + + @Override + public BiDi getBiDi() { if (biDi == null) { - synchronized (this) { + synchronized (lock) { if (biDi == null) { URI wsUri = - getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + 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)); + biDi = new BiDi(connection); } } } 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 processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant issue where new HTTP clients and connections are created on each call, which can lead to resource exhaustion and performance degradation. The proposed caching solution is a robust fix for this problem.	Medium
	✅ Prevent connection recreation with caching Suggestion Impact: The commit introduces caching of a single DevTools instance with double-checked locking and separates maybeGetDevTools and getDevTools, preventing repeated creation. This aligns with the suggestion’s intent to cache the DevTools connection. code diff: return new HasDevTools() { - private volatile Optional<DevTools> devTools; + private volatile DevTools devTools; + private final Object lock = new Object(); @Override public Optional<DevTools> maybeGetDevTools() { + return Optional.ofNullable(devTools); + } + + @Override + public DevTools getDevTools() { if (devTools == null) { - synchronized (this) { + synchronized (lock) { if (devTools == null) { Object cdpVersion = caps.getCapability("se:cdpVersion"); String version = - cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); + 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)); + SeleniumCdpConnection.create(caps) + .map(conn -> new DevTools(info::getDomains, conn)) + .orElseThrow(() -> new DevToolsException("Unable to create DevTools connection"));; } } } 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 processed] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out that repeatedly creating DevTools connections and performing version matching is inefficient and can lead to resource leaks. The proposed caching mechanism is an effective solution to ensure the connection is created only once.	Medium
Learned best practice	Ensure resources are properly closed Ensure created clients/connections are closed when no longer needed by using try-with-resources or by wiring them to a closeable BiDi that disposes underlying resources. 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)); + BiDi bidi = new BiDi(connection) { + @Override + public void close() { + super.close(); + try { + wsClient.close(); + } catch (Exception ignored) { + } + } + }; + return Optional.of(bidi); }; Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always close or dispose resources; ensure network clients and connections are shut down to prevent leaks.	Low
	Dispose DevTools connections properly Tie the lifecycle of the created DevTools to its underlying connection and ensure the connection is closed (e.g., implement close() or attach shutdown hook) to avoid leaks. 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 SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn) { + @Override + public void close() { + super.close(); + try { + conn.close(); + } catch (Exception ignored) { + } + } + }); }; Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Always close or dispose resources; ensure connections/listeners are shut down to prevent leaks.	Low
Update

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 3107 - Partially compliant Compliant requirements: Allow test execution to continue when certain browser APIs/features are unavailable or unreachable. Non-compliant requirements: Avoid automation flow interruptions caused by unexpected alerts/popups in IE11 by ensuring tests can proceed without being blocked. Requires further human verification: Validate in real environments (IE11/legacy setups) that augmentation no longer fails when CDP/BiDi endpoints are unreachable and does not introduce new interruptions. 2797 - Not compliant Non-compliant requirements: AfterScript event listener should include the return value of executed scripts to improve logging. 10132 - PR Code Verified Compliant requirements: Using HasDevTools with RemoteWebDriver/Augmenter should not throw ClassCastException or fail when CDP is inaccessible. Allow DevTools session creation when supported, but do not break augmentation if DevTools/CDP is unreachable. Requires further human verification: Run against Selenium Grid / dockerized Chrome to ensure augmentation no longer attempts immediate CDP connection and casting works when not using DevTools. 9803 - PR Code Verified Compliant requirements: RemoteWebDriver should be usable with HasDevTools without ClassCastException; augmentation should support lazy DevTools connection. Requires further human verification: Verify in Grid setup that HasDevTools casting succeeds and DevTools connection is deferred until first use.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Lazy Init Behavior Ensure the returned lambda does not capture a failed Optional early and that exceptions from SeleniumCdpConnection.create are only raised when getDevTools() is invoked, not during augmentation. 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; }; Error Messaging Confirm that when BiDi is unsupported or the websocket URL is missing/unreachable, the lazy supplier throws a clear BiDiException only upon invocation, and that augmenting other interfaces remains unaffected. 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)); };

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Prevent multiple connection creation Suggestion Impact: The commit implemented caching and lazy initialization of the BiDi instance with double-checked locking, preventing multiple connections. It changed from Optional to a single BiDi field, added maybeGetBiDi(), and synchronized on a lock object. code diff: return new HasBiDi() { - private volatile Optional<BiDi> biDi; + private volatile BiDi biDi; + private final Object lock = new Object(); @Override public Optional<BiDi> maybeGetBiDi() { + return Optional.ofNullable(biDi); + } + + @Override + public BiDi getBiDi() { if (biDi == null) { - synchronized (this) { + synchronized (lock) { if (biDi == null) { URI wsUri = - getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + 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)); + biDi = new BiDi(connection); } } } 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 processed] Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant issue where the PR's change to lazy-load the connection would create a new connection on every call, leading to resource leaks. The proposed fix using double-checked locking correctly implements a cached, lazy-initialized connection, which is a critical improvement for correctness and performance.	High
Learned best practice	Ensure connection is closed Ensure the created client/connection is properly closed when the BiDi instance is no longer needed by wiring a close hook or using try-with-resources where possible. java/src/org/openqa/selenium/bidi/BiDiProvider.java [50-55] 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)); +BiDi bidi = new BiDi(connection); +bidi.onClose(() -> { + try { + connection.close(); + } finally { + wsClient.close(); + } +}); +return Optional.of(bidi); Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always close or dispose resources using try-with-resources or ensure explicit shutdown to prevent leaks.	Low
More

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 9803 - PR Code Verified Compliant requirements: RemoteWebDriver should be usable with HasDevTools without ClassCastException or premature connection attempts. Augmentation should not fail when CDP endpoint is inaccessible, unless DevTools is actually used. Support using RemoteWebDriver in Grid environments where CDP URL may be unreachable. Requires further human verification: Validate on real Grid/Selenoid/Moon setups that augmentation no longer fails and casting works. Confirm that calling getDevTools() still connects properly when CDP is reachable. 10132 - PR Code Verified Compliant requirements: Avoid ClassCastException when casting augmented RemoteWebDriver to HasDevTools. Ensure augmenter returns a proxy implementing HasDevTools even if CDP is not yet connected. Defer CDP connection until DevTools is first accessed. Requires further human verification: Run an end-to-end test using selenium/standalone-chrome with remote augmentation to confirm cast succeeds and session creation works when reachable. 2797 - Not compliant Non-compliant requirements: afterScript event should include the return value of executed script. 3107 - Not compliant Non-compliant requirements: Handle IE alert interrupting automation; allow test to proceed without interruption.

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Thread Safety Double-checked locking uses a volatile Optional field but returns the same reference; ensure no visibility/race issues and that Optional is always non-null after initialization. return new HasDevTools() { private volatile Optional<DevTools> devTools; @Override public Optional<DevTools> maybeGetDevTools() { if (devTools == null) { synchronized (this) { if (devTools == 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); this.devTools = SeleniumCdpConnection.create(caps).map(conn -> new DevTools(info::getDomains, conn)); } } } return devTools; } }; Lazy Initialization Behavior maybeGetBiDi throws BiDiException if no URL capability is present; confirm this matches prior behavior and that augmenting alone does not trigger the exception unless accessed. return new HasBiDi() { private volatile Optional<BiDi> biDi; @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; } }; } Capability Handling Version detection now deferred; verify fallback to browserVersion is correct and works when se:cdpVersion is absent, and that NoOpCdpInfo is acceptable for older/unknown versions. 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)); } }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	✅ Simplify lazy-loading with a memoized supplier Suggestion Impact: The commit replaced the manual double-checked locking with a dedicated lazy initialization utility (org.openqa.selenium.concurrent.Lazy), simplifying the code and achieving the same goal as using Suppliers.memoize. code diff: return new HasBiDi() { - private volatile Optional<BiDi> biDi; + private final Lazy<BiDi> biDi = lazy(() -> establishBiDiConnection(caps)); @Override public Optional<BiDi> maybeGetBiDi() { - if (biDi == null) { - synchronized (this) { - if (biDi == null) { - URI wsUri = - getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported")); + return biDi.getIfInitialized(); + } - 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; + @Override + public BiDi getBiDi() { + return biDi.get(); } }; + } + + private BiDi establishBiDiConnection(Capabilities caps) { + 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 new BiDi(connection); } 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 processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the manual double-checked locking can be replaced by a simpler and more robust library utility (Suppliers.memoize), significantly improving code readability and maintainability.	Medium
Learned best practice	✅ Use initialized Optional with DCL Suggestion Impact: Instead of initializing an Optional and using DCL, the commit refactored to use Lazy initialization for DevTools, eliminating null checks and DCL entirely. This addresses the same concurrency and initialization concerns the suggestion targeted. code diff: + final Lazy<DevTools> devTools = lazy(() -> establishDevToolsConnection(caps)); + return new HasDevTools() { - private volatile Optional<DevTools> devTools; + @Override + public Optional<DevTools> maybeGetDevTools() { + return devTools.getIfInitialized(); + } @Override - public Optional<DevTools> maybeGetDevTools() { - if (devTools == null) { - synchronized (this) { - if (devTools == 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); - this.devTools = - SeleniumCdpConnection.create(caps) - .map(conn -> new DevTools(info::getDomains, conn)); - } - } - } - return devTools; + public DevTools getDevTools() { + return devTools.get(); } }; + } + + private DevTools establishDevToolsConnection(Capabilities caps) { + Object cdpVersion = caps.getCapability("se:cdpVersion"); + String version = + cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion(); + + CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new); + return SeleniumCdpConnection.create(caps) + .map(conn -> new DevTools(info::getDomains, conn)) + .orElseThrow(() -> new DevToolsException("Unable to create DevTools connection")); 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; } }; [Suggestion processed] Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Initialize attributes to sane defaults to avoid null/partially-initialized states, especially when using double-checked locking.	Low
More

[joerg1985]

I have some concerns about this PR:

• At least for BDI the URI should only be inside the capabilities when requested by the user, so for BiDi this should not needed.
  In case a feature the client requested is not working, the augmentation should fail in my mind.
• I guess most people have a retry on startup error implemented, so the retry coukd have handled this in the past and now the exception happens later and might not handled by this any more.

[asolntsev]

• At least for BDI the URI should only be inside the capabilities when requested by the user, so for BiDi this should not needed.

@joerg1985
Maybe for BiDi, yes. But for HasDevTools, it's automatically enabled for all Chromium browsers (at least). Users cannot avoid it (even if they don't need to use DevTools). That's the root problem.

In case a feature the client requested is not working, the augmentation should fail in my mind.

No, it should not. The goal of augmentation is NOT to check all the connectivity issues. The goal is to CAST RemoteWebDriver to the needed interface. Only to CAST, not to call its methods.

• I guess most people have a retry on startup error implemented, so the retry coukd have handled this in the past and now the exception happens later and might not handled by this any more.

But the augmentation is NOT automatically called on startup.
The augmentation happens only user explicitly calls new Augmenter().augment(remoteWebDriver). So there is no automated retry out-of-the-box anyway.

AND if I really want to check BiDi connectivity, I would explicitly call some BiDi method on startup.

One argument for this PR:

1. All other AugmenterProviders ARE lazy. See AddHasPermissions, AddHasCdp, AddHasExtensions, AddHasLogEvents, AddHasContext, AddHasNetworkConditions - they all have getImplementation of form:

  public HasDebugger getImplementation(Capabilities capabilities, ExecuteMethod executeMethod) {
    return () -> { ... ALL THE WORK ONLY HERE ...}
  }

  // or

  public HasPermissions getImplementation(Capabilities capabilities, ExecuteMethod executeMethod) {
    return new HasPermissions() {
          ... ALL THE WORK ONLY HERE ...
    };
  }

2. Those people need to check connectivity on startup, they have an option: call any BiDi method.
   But those people who don't need BIdi or DevTools (like in my case), just cannot avoid it. They have no way to avoid this connectivity issue.

[joerg1985]

@asolntsev i only wanted to point to this change in behaviour, i think having the same behaviour for all augmentations is a good idea. So i only have one comment left, all other areas rely on external synchronizations in case multiple threads are involved.
Is there a specific need to sychronize here or could this be removed?

[asolntsev]

Is there a specific need to sychronize here or could this be removed?

If I understand your question correctly, then yes, the synchronization is needed in this case.
This is what I mean:

@Test void augmenterThreadSafety() {
  RemoteWebDriver driver = new RemoteWebDriver(gridUrl(), new ChromeOptions());
  WebDriver webDriver = new Augmenter().augment(driver);
  HasDevTools devTools = (HasDevTools) webDriver;

    for (int i = 0; i < 10; i++) {
      new Thread(() -> {
        devTools.getDevTools().getDomains(); // would create multiple instances of `DevTools` without synchronization
      }).start();
   }
}

[joerg1985]

@asolntsev in my mind the client code has to take care about synchronization, in your example:

  RemoteWebDriver driver = new RemoteWebDriver(gridUrl(), new ChromeOptions());
  WebDriver webDriver = new Augmenter().augment(driver);
  HasDevTools devTools = (HasDevTools) webDriver;

    for (int i = 0; i < 10; i++) {
      new Thread(() -> {
	synchronized (devTools) {
        	devTools.getDevTools().getDomains(); 
	}
      }).start();
   }
}

Other areas of the selenium code do not expect the client code will use different threads.
So why expecting it inside the HasBiDi / HasDevTools implementation?

e.g. the code below will end in chaos without synchronization in the client code:

  RemoteWebDriver driver = new RemoteWebDriver(gridUrl(), new ChromeOptions());

    for (int i = 0; i < 10; i++) {
      new Thread(() -> {
	driver.findElement(By.id("x")).click();
      }).start();
   }
}

[asolntsev]

in my mind the client code has to take care about synchronization

1. You cannot expect from all Selenium users that they can do synchronization properly.
2. I sincerely don't understand why should we intentionally make Selenium less secure. This synchronization is very easy to do, it's cheap, it's safe. I see zero problems with it.

the code below will end in chaos without synchronization in the client code:

Yes, maybe this code wouldn't do anything reasonable, but it's safe. I mean, it doesn't cause connection leak etc.
While the code above will cause connection leak.

[joerg1985]

@asolntsev lets see what others think about this

[nvborisenko]

Easy, should be thread-safe.

[asolntsev]

Easy, should be thread-safe.

@nvborisenko Great! Then can we already merge this PR?

[asolntsev]

Please.
Code.
Review.

Please?
Code??
Review??

Please!
Code!!
Review!!

@mkurz @diemol @iampopovich @VietND96 @cgoldberg @joerg1985 @navin772 @vicky-iv @giulong @pujagani

[titusfortner]

If you don't have socket support...
By default webSocketUrl is false, so BiDi is not augmented
CDP is on by default, so you can toggle it off with "se:cdpEnabled": false

Please explain why a user should not have to set the capabilities to match the behavior they get?

As for the code itself, I'm a little concerned about doing synchronization things inside this method, but not in other methods. I'd have to think through the implications. If Diego or Joerg say it is fine, then I'll go with their expertise, but in general I'd say not to add the extra code if it isn't needed.

[asolntsev]

If you don't have socket support... By default webSocketUrl is false, so BiDi is not augmented CDP is on by default, so you can toggle it off with "se:cdpEnabled": false

Please explain why a user should not have to set the capabilities to match the behavior they get?

Look, we care about simplicity for our users, right?
We want Selenium to be simple to use as possible, right?
For user, the most simple is just to call new Augmenter().augment(webdriver) without setting any capabilities.
Users who doesn't need CDP should be able to just start browser without setting any CDP capabilities. Right?

Honestly, I don't understand why are you trying to make your users more work than needed.

I'm a little concerned about doing synchronization things inside this method.

I don't see problems here. Synchronized are exactly those methods which establish a new connection. Exactly as it should be.

[nvborisenko]

.NET binding never establishes new network connection if it is not required.

[titusfortner]

I think if we decide to lazy load DevTools we should do the same for BiDi. Eventually that's going to be default.

I took at deeper look at code and noted some things.
Maybe we can log more info when getDevTools / getBiDi fail
Let's at least log that we're lazy loading since it's a change.
Can we add a test for these?

I'm still deferring to @diemol if he says no. 😄

[asolntsev]

@titusfortner I've extracted the "synchronization" code to a separate class Lazy.
I hope it make DevToolsProvider and BiDiProvider much simpler, and not-so-scary.
Now they use lazy function (similar to Kotlin lazy idiom):

  @Override
  public HasBiDi getImplementation(Capabilities caps, ExecuteMethod executeMethod) {
    return new HasBiDi() {
      private final Lazy<BiDi> biDi = lazy(() -> establishBiDiConnection(caps));

      @Override
      public Optional<BiDi> maybeGetBiDi() {
        return biDi.getIfInitialized();
      }

      @Override
      public BiDi getBiDi() {
        return biDi.get();
      }
    };
  }

[titusfortner]

I know it is just an abstraction, but the new code feels so much better to me. Thanks also for the tests.
I still think we should add logging that we're doing it lazy now instead of on startup (maybe in the getImplementation() methods?). We can also add a warning entry for URISyntaxException, which Chrome & Firefox implementations do.

To clarify for @diemol the use case that made me think we should do this now is that a user might need to run their code on a remote grid that doesn't support sockets, the grid config would need to change not just the client code. So the user can't just toggle this from their capabilities and ensure it works as I previously suggested.
Also, if this is the right approach for CDP, I think it should also be used for BiDi, because at some point we're going to be turning on BiDi by default, and this would make that transition easier (refer to previous conversations with Simon).

[asolntsev]

I still think we should add logging that we're doing it lazy now instead of on startup

Sure, we can add the logging.
Just to clarify: it was never done on "startup", it was done when user called new Augmenter().augment(webdriver).

So you are suggesting that during new Augmenter().augment(webdriver) call, Selenium will write done two new logs:

1. INFO [BiDiProvider] Add BiDi implementation - but BiDi connection will be establish only on a first usage
2. INFO [DevToolsProvider] Add DevTools implementation - but DevTools connection will be establish only on a first usage

Right? Do the texts sound good? Should it be "INFO" or "WARNING" or "DEBUG"?

P.S. If you really assume some users want to check-up all connections during augmentation, we could add an optional parameter to augment method:

Augmenter augmenter = new Augmenter();
WebDriver wd1 = augmenter.augment(webdriver); // lazy-initialized
WebDriver wd1 = augmenter.augment(webdriver, LAZY); // lazy-initialized
WebDriver wd2 = augmenter.augment(webdriver, EAGER); // checks all connections right now

// where the second parameter is:
enum InitializationMode {LAZY, EAGER}

[titusfortner]

I was thinking of when you use webdriver builder, it automatically augments and that's on startup.

The get implementation method is only called during augmentation and we already know it matched on is applicable. Info is good for this now since this is a behavior change. I only do warnings if I think it's something the user needs to change.

I would say something like "driver augmented with bidi/devtools interface, but the connection is not verified until first use."

If someone really needs to check right away, they can cast and call the get bidi command, right? No need to make the implementation more complicated with an optional parameter.

[asolntsev]

@titusfortner Ok, added the log:

LOG.log(INFO, "WebDriver augmented with DevTools interface; connection will not be verified until first use.");
LOG.log(INFO, "WebDriver augmented with BiDi interface; connection will not be verified until first use.");

[diemol]

Please run ./scripts/format.sh so we can get this merged. Thanks for the effort to explain your approach.

[asolntsev]

Please run ./scripts/format.sh so we can get this merged

Strange... I am sure I already executed ./scripts/format.sh before...

UPD Now I see the problem: this script always fails on my machine:

INFO: Running command line: bazel-bin/buildifier.bash
./.bazelbsp/aspects/rules/android/android_info.bzl:3:30: unexpected input character '$'
./.bazelbsp/aspects/rules/android/android_info.bzl # reformat

[titusfortner]

Looks good, thank you for the updates and your patience 😄