[java] Add web socket timeout for java binding#16410
[java] Add web socket timeout for java binding#16410iampopovich wants to merge 9 commits intoSeleniumHQ:trunkfrom
Conversation
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
|
based on the test in python implementation i added properties with duration seconds and milliseconds should it be both of milliseconds ? |
titusfortner
left a comment
titusfortner
left a comment
There was a problem hiding this comment.
We need to keep these parameters contained to the client config class. The BiDi spec doesn't manage any timeouts, so we don't need to have options that get sent to the browser, and we don't want the driver juggling this directly.
Check out #13190 where we create a BiDi command executor to manage all of this. I'm not sure how close to ready that PR is I haven't looked at it in a while, but we actually want to pull this code out of the individual drivers.
In Python, both values are in seconds. (defaults are
In Python, we have Here is an example of instantiating a |
|
We need to get the ClientConfig all they way down to the drivers. I would like to avoid this approach. |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for configuring WebSocket timeout and interval settings for BiDi connections in Java bindings. It introduces new configuration options in ChromiumOptions and FirefoxOptions, propagates these settings through driver constructors, and extends the HTTP client infrastructure to support these parameters.
- Adds
webSocketTimeoutandwebSocketIntervalconfiguration fields with default values (30 seconds and 100 milliseconds respectively) to options classes - Extracts and propagates WebSocket configuration from capabilities through driver constructors to BiDi connection setup
- Extends
ClientConfigandJdkHttpClientto support and apply WebSocket timeout settings
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| ChromiumOptions.java | Adds WebSocket timeout/interval fields, setters/getters, and capability export with proper merge logic |
| ChromiumDriver.java | Extracts WebSocket configuration from capabilities and passes to BiDi connection creation |
| FirefoxOptions.java | Adds WebSocket timeout/interval fields, setters/getters, and capability export |
| FirefoxDriver.java | Extracts WebSocket configuration from capabilities and passes to BiDi connection creation |
| ClientConfig.java | Extends configuration to support WebSocket timeout/interval with builder pattern methods |
| JdkHttpClient.java | Stores WebSocket configuration and applies timeout to WebSocket operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /** | ||
| * Gets the WebSocket response wait interval (in seconds) used for communicating with the browser. |
There was a problem hiding this comment.
The documentation states the parameter is "in seconds" but the default value is Duration.ofMillis(100), which is 100 milliseconds, not seconds. The documentation should be corrected to say "used for communicating with the browser" without specifying the unit, or should correctly indicate it accepts a Duration object.
| } | ||
|
|
||
| /** | ||
| * Gets the WebSocket response wait timeout (in seconds) used for communicating with the browser. |
There was a problem hiding this comment.
The documentation states the parameter is "in seconds" but the default value is Duration.ofSeconds(30), which is correct. However, the getter documentation also says "in seconds" which is misleading since it returns a Duration object. Consider updating the documentation to simply say "WebSocket response wait timeout" without specifying the unit since Duration handles that.
| } | ||
|
|
||
| /** | ||
| * Sets the WebSocket response wait timeout (in seconds) used for communicating with the browser. |
There was a problem hiding this comment.
The documentation states the parameter is "in seconds" but the default value is Duration.ofSeconds(30), which is correct. However, the getter documentation also says "in seconds" which is misleading since it returns a Duration object. Consider updating the documentation to simply say "WebSocket response wait timeout" without specifying the unit since Duration handles that.
| /** | ||
| * Sets the WebSocket response wait timeout (in seconds) used for communicating with the browser. | ||
| * | ||
| * @param timeout WebSocket response wait timeout | ||
| * @return A self reference. | ||
| */ | ||
| public T setWebSocketTimeout(Duration timeout) { | ||
| this.webSocketTimeout = Require.nonNull("WebSocket timeout", timeout); | ||
| return (T) this; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the WebSocket response wait timeout (in seconds) used for communicating with the browser. | ||
| * | ||
| * @return WebSocket response wait timeout | ||
| */ | ||
| public Duration getWebSocketTimeout() { | ||
| return webSocketTimeout; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the WebSocket response wait interval (in seconds) used for communicating with the browser. | ||
| * | ||
| * @param interval WebSocket response wait interval | ||
| * @return A self reference. | ||
| */ | ||
| public T setWebSocketInterval(Duration interval) { | ||
| this.webSocketInterval = Require.nonNull("WebSocket interval", interval); | ||
| return (T) this; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the WebSocket response wait interval (in seconds) used for communicating with the browser. | ||
| * | ||
| * @return WebSocket response wait interval | ||
| */ | ||
| public Duration getWebSocketInterval() { | ||
| return webSocketInterval; | ||
| } |
There was a problem hiding this comment.
The new WebSocket timeout and interval configuration features lack test coverage. Consider adding unit tests to verify: 1) that the setter and getter methods work correctly, 2) that the capabilities are properly exported with the correct keys ("se:webSocketTimeout" and "se:webSocketInterval"), 3) that the values are properly converted to milliseconds, and 4) that the merge behavior works correctly in mergeInPlace.
| /** | ||
| * Sets the WebSocket timeout for BiDi connections. | ||
| * | ||
| * @param webSocketTimeout the WebSocket timeout duration | ||
| * @return A self reference. | ||
| */ | ||
| public FirefoxOptions setWebSocketTimeout(Duration webSocketTimeout) { | ||
| this.webSocketTimeout = Require.nonNull("WebSocket timeout", webSocketTimeout); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the WebSocket timeout for BiDi connections. | ||
| * | ||
| * @return the WebSocket timeout duration | ||
| */ | ||
| public Duration getWebSocketTimeout() { | ||
| return webSocketTimeout; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the WebSocket interval for BiDi connections. | ||
| * | ||
| * @param webSocketInterval the WebSocket interval duration | ||
| * @return A self reference. | ||
| */ | ||
| public FirefoxOptions setWebSocketInterval(Duration webSocketInterval) { | ||
| this.webSocketInterval = Require.nonNull("WebSocket interval", webSocketInterval); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the WebSocket interval for BiDi connections. | ||
| * | ||
| * @return the WebSocket interval duration | ||
| */ | ||
| public Duration getWebSocketInterval() { | ||
| return webSocketInterval; | ||
| } |
There was a problem hiding this comment.
The new WebSocket timeout and interval configuration features lack test coverage. Consider adding unit tests to verify: 1) that the setter and getter methods work correctly, 2) that the capabilities are properly exported with the correct keys ("se:webSocketTimeout" and "se:webSocketInterval"), and 3) that the values are properly converted to milliseconds.
| if (options.webSocketTimeout != null) { | ||
| setWebSocketTimeout(options.webSocketTimeout); | ||
| } | ||
| if (options.webSocketInterval != null) { | ||
| setWebSocketInterval(options.webSocketInterval); | ||
| } |
There was a problem hiding this comment.
The null checks for webSocketTimeout and webSocketInterval are unnecessary because these fields are initialized with non-null default values (Duration.ofSeconds(30) and Duration.ofMillis(100)) in the field declarations. These fields can never be null unless explicitly set via reflection. Consider removing these null checks or use Optional.ofNullable() pattern for consistency with the other optional fields in this method.
| if (options.webSocketTimeout != null) { | |
| setWebSocketTimeout(options.webSocketTimeout); | |
| } | |
| if (options.webSocketInterval != null) { | |
| setWebSocketInterval(options.webSocketInterval); | |
| } | |
| setWebSocketTimeout(options.webSocketTimeout); | |
| setWebSocketInterval(options.webSocketInterval); |
| private final Duration readTimeout; | ||
| private final Duration connectTimeout; | ||
| private final Duration webSocketTimeout; | ||
| private final Duration webSocketInterval; |
There was a problem hiding this comment.
The webSocketInterval field is stored but never actually used in the WebSocket implementation. According to the related issue #16270, this interval should be used to check for a response to a sent message, but it's not applied anywhere in the WebSocket send logic or connection establishment. Consider implementing the intended behavior or removing this unused field.
| } | ||
|
|
||
| /** | ||
| * Sets the WebSocket response wait interval (in seconds) used for communicating with the browser. |
There was a problem hiding this comment.
The documentation states the parameter is "in seconds" but the default value is Duration.ofMillis(100), which is 100 milliseconds, not seconds. The documentation should be corrected to say "used for communicating with the browser" without specifying the unit, or should correctly indicate it accepts a Duration object.
| Duration.ofSeconds( | ||
| Long.parseLong(System.getProperty("webdriver.httpclient.readTimeout", "180"))), | ||
| Duration.ofSeconds( | ||
| Long.parseLong(System.getProperty("webdriver.httpclient.webSocketTimeout", "30"))), |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
| Duration.ofSeconds( | ||
| Long.parseLong(System.getProperty("webdriver.httpclient.webSocketTimeout", "30"))), | ||
| Duration.ofMillis( | ||
| Long.parseLong(System.getProperty("webdriver.httpclient.webSocketInterval", "100"))), |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
…fox drivers to use ClientConfig
@titusfortner Let's wait for #13190 to be merged into trunk, and then use the BiDi executor in this PR for timeouts. |
|
What's the status of this PR? I created another PR #16796 which is somewhat simpler: it just adds Can we maybe merge my PR and then continue with all these bigger historical changes? @iampopovich @titusfortner |
|
@asolntsev I don't mind if we use your changes going forward, as long as they're simpler and already ready. You can close my pull request. |
User description
🔗 Related Issues
fixes #16270
relates #13190
💥 What does this PR do?
This pull request adds support for configuring WebSocket timeout and interval settings for BiDi (Bidirectional) connections in both Chromium and Firefox drivers. It introduces new options in
ChromiumOptionsandFirefoxOptionsto allow users to customize these settings, and propagates these configurations through the driver and HTTP client layers. The changes also ensure these settings are merged and exported as capabilities, making them available throughout the driver lifecycle.🔧 Implementation Notes
WebSocket Timeout and Interval Configuration Support
Added
webSocketTimeoutandwebSocketIntervalfields, along with corresponding setter and getter methods, to bothChromiumOptionsandFirefoxOptions, allowing users to customize WebSocket communication timing. These are also included in the set of extra capability names and exported as capabilities (se:webSocketTimeoutandse:webSocketInterval).Updated the constructors of
ChromiumDriverandFirefoxDriverto extract the new WebSocket timeout and interval capabilities and pass them to the BiDi connection logic, ensuring the driver uses the configured values.Modified the
createBiDimethods in both drivers to accept and use the WebSocket timeout and interval values when creating the WebSocket client configuration.HTTP Client Configuration Enhancements
ClientConfigto supportwebSocketTimeoutandwebSocketIntervalfields, with associated builder methods and propagation through all configuration methods. Updated default values and ensured these settings are applied when constructing HTTP clients for WebSocket communication.Other Minor Updates
Durationin relevant files. [💡 Additional Considerations
🔄 Types of changes
PR Type
Enhancement
Description
Add WebSocket timeout and interval configuration support for BiDi connections in Java bindings
Extend
ChromiumOptionsandFirefoxOptionswith WebSocket timing setters/getters and capability exportPropagate WebSocket configuration through driver constructors and HTTP client layers
Update
ClientConfigandJdkHttpClientto support and apply WebSocket timeout/interval settingsDiagram Walkthrough
File Walkthrough
ChromiumDriver.java
Extract and apply WebSocket configuration in ChromiumDriverjava/src/org/openqa/selenium/chromium/ChromiumDriver.java
values
createBiDimethodClientConfigChromiumOptions.java
Add WebSocket configuration fields and capabilities to ChromiumOptionsjava/src/org/openqa/selenium/chromium/ChromiumOptions.java
webSocketTimeoutandwebSocketIntervalfields with default valuesse:webSocketTimeoutandse:webSocketIntervalcapabilitiesmergeInPlacemethodFirefoxDriver.java
Extract and apply WebSocket configuration in FirefoxDriverjava/src/org/openqa/selenium/firefox/FirefoxDriver.java
createBiDimethodClientConfigwith WebSocket timeout and interval for BiDiconnection
FirefoxOptions.java
Add WebSocket configuration fields and capabilities to FirefoxOptionsjava/src/org/openqa/selenium/firefox/FirefoxOptions.java
webSocketTimeoutandwebSocketIntervalfields with default valuesgetExtraCapabilitygetExtraCapabilityNamesClientConfig.java
Extend ClientConfig with WebSocket timeout and interval supportjava/src/org/openqa/selenium/remote/http/ClientConfig.java
webSocketTimeoutandwebSocketIntervalfields to configurationJdkHttpClient.java
Apply WebSocket timeout configuration in JdkHttpClientjava/src/org/openqa/selenium/remote/http/jdk/JdkHttpClient.java
webSocketTimeoutandwebSocketIntervalfields fromClientConfigwebSocketTimeoutinstead ofreadTimeoutfor WebSocket operationsmessage sending