Skip to content

Conversation

cgoldberg
Copy link
Contributor

@cgoldberg cgoldberg commented Aug 23, 2025

User description

🔗 Related Issues

#16270

💥 What does this PR do?

This PR allows the values for WebSocket timeout and wait interval to be set via the ClientConfig class in the selenium.webdriver.remote.client_config module.

The class initializer is also where the default values are stored. Previously the values were set in private attributes in the WebSocketConnection class and not easily configurable by the user.

These settings are used for browser communication via WebSockets for both BiDi and CDP.

example usage:

setting the WebSocket timeout and wait interval on an existing local WebDriver:

from selenium import webdriver

driver = webdriver.Chrome()
driver.command_executor.client_config.websocket_timeout = 10
driver.command_executor.client_config.websocket_interval = 0.2

starting a remote WebDriver using a custom ClientConfig that sets the WebSocket timeout and wait interval:

from selenium import webdriver
from selenium.webdriver.remote.client_config import ClientConfig

client_config = ClientConfig(
    remote_server_addr="http://localhost:4444",
    websocket_timeout=10,
    websocket_interval=0.2,
)
options = webdriver.ChromeOptions()
driver = webdriver.Remote(options=options, client_config=client_config)

🔗 Related Issues

Fixes #16260

🔧 Implementation Notes

This follows the implementation discussed in the 08/21/2025 Selenium TLC call.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Add WebSocket timeout and interval configuration to ClientConfig

  • Update WebSocketConnection to accept configurable timeout/interval parameters

  • Enable WebSocket settings for both BiDi and CDP communication

  • Minor documentation formatting improvements


Diagram Walkthrough

flowchart LR
  A["ClientConfig"] -- "adds websocket_timeout & websocket_interval" --> B["WebSocketConnection"]
  B -- "configures timeout/interval" --> C["BiDi Communication"]
  B -- "configures timeout/interval" --> D["CDP Communication"]
Loading

File Walkthrough

Relevant files
Enhancement
client_config.py
Add WebSocket configuration properties                                     

py/selenium/webdriver/remote/client_config.py

  • Add websocket_timeout and websocket_interval properties with
    descriptors
  • Update constructor to accept WebSocket configuration parameters with
    defaults
  • Minor documentation formatting improvements for existing properties
+12/-7   
webdriver.py
Pass WebSocket config to connections                                         

py/selenium/webdriver/remote/webdriver.py

  • Update start_devtools() to pass WebSocket config from client_config
  • Update _start_bidi() to pass WebSocket config from client_config
  • Add blank line formatting improvement
+7/-2     
websocket_connection.py
Accept configurable timeout and interval                                 

py/selenium/webdriver/remote/websocket_connection.py

  • Remove hardcoded timeout/interval class attributes
  • Update constructor to accept timeout and interval parameters
  • Use instance attributes instead of class attributes for timing
  • Add blank line formatting improvement
+7/-7     

@selenium-ci selenium-ci added the C-py Python Bindings label Aug 23, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Type Hints

The new constructor and attributes for WebSocket timing parameters are untyped; adding explicit types (e.g., url: str, timeout: float, interval: float) would improve readability and tooling support.

def __init__(self, url, timeout, interval):
    self.callbacks = {}
    self.session_id = None
    self.url = url
    self.response_wait_timeout = timeout
    self.response_wait_interval = interval
None Safety

Accessing client_config.websocket_timeout/interval assumes client_config is always present; verify code paths where WebDriver may be constructed without a client_config to avoid AttributeError or pass-through of None values.

self._websocket_connection = WebSocketConnection(
    ws_url, self.client_config.websocket_timeout, self.client_config.websocket_interval
)
Doc Consistency

Updated docstrings switched to "with the driver/server" phrasing; ensure consistency across all properties and confirm Sphinx rendering remains correct after reflows.

"""Gets and Sets the proxy used for communicating with the driver/server."""
ignore_certificates = _ClientConfigDescriptor("_ignore_certificates")
"""Gets and Sets the ignore certificate check value."""
init_args_for_pool_manager = _ClientConfigDescriptor("_init_args_for_pool_manager")
"""Gets and Sets the ignore certificate check."""
timeout = _ClientConfigDescriptor("_timeout")
"""Gets and Sets the timeout (in seconds) used for communicating with the driver/server."""
ca_certs = _ClientConfigDescriptor("_ca_certs")
"""Gets and Sets the path to bundle of CA certificates."""
username = _ClientConfigDescriptor("_username")
"""Gets and Sets the username used for basic authentication to the remote."""
password = _ClientConfigDescriptor("_password")
"""Gets and Sets the password used for basic authentication to the remote."""

Copy link
Contributor

qodo-merge-pro bot commented Aug 23, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Validate WebSocket timing inputs

Ensure time values are non-negative and reasonable before passing to the
WebSocket layer. Clamp or raise if invalid to prevent infinite loops or
immediate timeouts.

py/selenium/webdriver/remote/webdriver.py [1215-1217]

-self._websocket_connection = WebSocketConnection(
-    ws_url, self.client_config.websocket_timeout, self.client_config.websocket_interval
-)
+timeout = max(0.0, float(getattr(self.client_config, "websocket_timeout", 30.0)))
+interval = max(0.001, float(getattr(self.client_config, "websocket_interval", 0.1)))
+self._websocket_connection = WebSocketConnection(ws_url, timeout, interval)
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good suggestion for input validation, as user-configurable timeout and interval values could be negative, leading to incorrect behavior in the wait loop.

Medium
Learned
best practice
Validate WebSocket constructor inputs
Suggestion Impact:The commit added constructor parameter validation for timeout and interval (type checks and positivity), raising errors when invalid, aligning with the suggestion’s intent. It did not validate url nor cast to float, and used WebDriverException instead of ValueError.

code diff:

+        if not isinstance(timeout, (int, float)) or timeout < 0:
+            raise WebDriverException("timeout must be a positive number")
+        if not isinstance(interval, (int, float)) or timeout < 0:
+            raise WebDriverException("interval must be a positive number")
+
         self.url = url
         self.response_wait_timeout = timeout
         self.response_wait_interval = interval

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 processed]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add null checks and validation for parameters and variables before using them to prevent runtime errors.

Low
Possible issue
Handle missing client configuration

Guard against client_config being None to avoid attribute access errors. Provide
sensible fallbacks when not configured.

py/selenium/webdriver/remote/webdriver.py [1215-1217]

-self._websocket_connection = WebSocketConnection(
-    ws_url, self.client_config.websocket_timeout, self.client_config.websocket_interval
-)
+timeout = getattr(self.client_config, "websocket_timeout", 30.0)
+interval = getattr(self.client_config, "websocket_interval", 0.1)
+self._websocket_connection = WebSocketConnection(ws_url, timeout, interval)
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion to use getattr provides a minor robustness improvement, but self.client_config is unlikely to be None in normal operation, making this a very low-impact change.

Low
  • Update

@cgoldberg cgoldberg requested review from navin772 and shbenzer August 23, 2025 17:17
@@ -1211,7 +1212,9 @@ def start_devtools(self):
return self._devtools, self._websocket_connection
if self.caps["browserName"].lower() == "firefox":
raise RuntimeError("CDP support for Firefox has been removed. Please switch to WebDriver BiDi.")
self._websocket_connection = WebSocketConnection(ws_url)
self._websocket_connection = WebSocketConnection(
ws_url, self.client_config.websocket_timeout, self.client_config.websocket_interval
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ws_url, self.client_config.websocket_timeout, self.client_config.websocket_interval
ws_url,
self.command_executor.client_config.websocket_timeout,
self.command_executor.client_config.websocket_interval,

All BiDi tests are failing because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll take a look and fix it... I didn't see anywhere else in the code this was getting called, but I must have missed something.

@@ -1260,7 +1263,9 @@ def _start_bidi(self):
else:
raise WebDriverException("Unable to find url to connect to from capabilities")

self._websocket_connection = WebSocketConnection(ws_url)
self._websocket_connection = WebSocketConnection(
ws_url, self.client_config.websocket_timeout, self.client_config.websocket_interval
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ws_url, self.client_config.websocket_timeout, self.client_config.websocket_interval
ws_url,
self.command_executor.client_config.websocket_timeout,
self.command_executor.client_config.websocket_interval,

Same here

@navin772
Copy link
Member

I think we should also add a test that actually modifies the timeout in addition to asserting default values test.

@cgoldberg
Copy link
Contributor Author

sure... I'll add another test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: [py] CDP WebSocket connection timeout is too short
3 participants