Skip to content

make augmentation of HasBiDi/HasDevTools lazy-loaded #16338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
+55 −12
Open

make augmentation of HasBiDi/HasDevTools lazy-loaded #16338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f36cdfa
make augmentation of HasBiDi/HasDevTools lazy-loaded
asolntsev Sep 20, 2025
425fab0
avoid establishing CDP connection in `maybeGet*()` method
asolntsev Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 27 additions & 6 deletions java/src/org/openqa/selenium/bidi/BiDiProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.net.URISyntaxException;
import java.util.Optional;
import java.util.function.Predicate;

import org.openqa.selenium.Capabilities;
import org.openqa.selenium.remote.AugmenterProvider;
import org.openqa.selenium.remote.ExecuteMethod;
Expand All @@ -44,15 +45,35 @@ public Class<HasBiDi> getDescribedInterface() {

@Override
public HasBiDi getImplementation(Capabilities caps, ExecuteMethod executeMethod) {
return new HasBiDi() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure about the BiDi part because the user is expressing the intention to use BiDi by setting webSocketUrl: true. I think it makes sense for CDP because we are doing that implicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right: users can enable or disable BiDi.
But it doesn't matter. Anyway, it's not needed to establish BiDi connection while no BiDi methods are called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that, but if the user has set the capability, then it means they want to use BiDi methods. I would prefer to avoid this change. The CDP one seems fine.

Copy link
Contributor Author

@asolntsev asolntsev Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why? If users want to use BiDi methods, they will use BiDi methods.
I still don't see any reasons to perform side effects during augmentation.

  • Augmenter should only augment (create instance of interfaces).
  • Augmenter should not perform any other actions (establish CDP connection, establish BiDi connection etc.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the user has set the capability, then it means they want to use BiDi methods

This is big mistake. If I set some capability, it means I set capability. I don't know who is BiDi.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nvborisenko Diego meant a very specific capability "se:cdpEnabled" which allows to disable the BiDi connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure about the BiDi part because the user is expressing the intention to use BiDi by setting webSocketUrl: true. I think it makes sense for CDP because we are doing that implicitly.

@diemol What can I do to get this PR merged?
At least you agree that it makes sense for CDP, right?
Do you want me to revert the BiDiProvider part and keep only DevToolsProvider change? If I do so, will you merge the PR?

(just be worried that then BiDiProvider and DevToolsProvider will behave differently: one would be lazy-loaded and the other eager. One has side effect, and all the other providers don't.)

private volatile BiDi biDi;
private final Object lock = new Object();

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

URI wsUri = getBiDiUrl(caps).orElseThrow(() -> new BiDiException("BiDi not supported"));
@Override
public BiDi getBiDi() {
if (biDi == null) {
synchronized (lock) {
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());
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 = new BiDi(connection);
}
}
}
return biDi;
}
};
}

private Optional<URI> getBiDiUrl(Capabilities caps) {
Expand Down
34 changes: 28 additions & 6 deletions java/src/org/openqa/selenium/devtools/DevToolsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.net.URI;
import java.util.Optional;
import java.util.function.Predicate;

import org.openqa.selenium.Capabilities;
import org.openqa.selenium.devtools.noop.NoOpCdpInfo;
import org.openqa.selenium.remote.AugmenterProvider;
Expand All @@ -42,14 +43,35 @@ public Class<HasDevTools> getDescribedInterface() {

@Override
public HasDevTools getImplementation(Capabilities caps, ExecuteMethod executeMethod) {
Object cdpVersion = caps.getCapability("se:cdpVersion");
String version = cdpVersion instanceof String ? (String) cdpVersion : caps.getBrowserVersion();
return new HasDevTools() {
private volatile DevTools devTools;
private final Object lock = new Object();

@Override
public Optional<DevTools> maybeGetDevTools() {
return Optional.ofNullable(devTools);
}

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

return () -> devTools;
CdpInfo info = new CdpVersionFinder().match(version).orElseGet(NoOpCdpInfo::new);
this.devTools =
SeleniumCdpConnection.create(caps)
.map(conn -> new DevTools(info::getDomains, conn))
.orElseThrow(() -> new DevToolsException("Unable to create DevTools connection"));;
}
}
}
return devTools;
}
};
}

private String getCdpUrl(Capabilities caps) {
Expand Down
Loading