Skip to content

Conversation

titusfortner
Copy link
Member

@titusfortner titusfortner commented Oct 22, 2025

User description

🔗 Related Issues

Addresses Ruby portion of #16269

💥 What does this PR do?

  • Creates config class for customizing everything currently customizable about Default http client
  • Allows passing in the client_config into the driver constructors which passes it to the Bridge constructor
  • Bridge Constructor has to manage all the ways the parameters can be in conflict

🔧 Implementation Notes

Went with a Data instance based on its goal of immutability. I overrode the constructor so that it only allows keywords and they are all optional
PORO seemed overkill and Struct is too loose

💡 Additional Considerations

  1. My first thought was that we would be deprecating the existing http_client approach, but that only makes sense if net-http can do everything everyone needs it to do, and apparently there is a lot it can't do, so I guess we continue to allow people to also pass in an http_client? Or we figure out some kind of factory to use this client config with the various options?
  2. There are several additional things that can be modified here; we could add more to the client config:
    • max redirects
    • retries
    • keep-alive timeout
    • http version
  3. Finally, update [rb] allow setting socket timeout and interval from http client #16472 to use this approach to support websocket values as well

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Introduces ClientConfig immutable data class for HTTP client customization

  • Allows passing client_config to driver constructors and bridge initialization

  • Implements validation to prevent conflicting http_client and client_config parameters

  • Refactors bridge initialization with URL normalization and server URL handling

  • Adds comprehensive test coverage for new client config functionality


Diagram Walkthrough

flowchart LR
  Driver["Driver Constructor"]
  Bridge["Bridge Initialize"]
  ClientConfig["ClientConfig Data Class"]
  HttpClient["Http::Default Client"]
  
  Driver -->|"client_config param"| Bridge
  Bridge -->|"validates params"| ClientConfig
  ClientConfig -->|"builds from config"| HttpClient
  Bridge -->|"uses http_client"| HttpClient
Loading

File Walkthrough

Relevant files
Enhancement
4 files
client_config.rb
Create immutable ClientConfig data class                                 
+46/-0   
driver.rb
Update create_bridge to accept client_config                         
+2/-2     
bridge.rb
Refactor initialization with client_config support             
+44/-6   
common.rb
Add server_url? predicate method                                                 
+4/-0     
Configuration changes
1 files
common.rb
Add ClientConfig require statement                                             
+1/-0     
Tests
1 files
bridge_spec.rb
Add comprehensive bridge initialization tests                       
+57/-2   
Documentation
5 files
client_config.rbs
Add type signature for ClientConfig class                               
+28/-0   
driver.rbs
Update driver type signatures for client_config                   
+1/-1     
bridge.rbs
Update bridge type signatures with new methods                     
+9/-1     
common.rbs
Update HTTP common type signatures                                             
+6/-4     
default.rbs
Improve type annotations for HTTP default                               
+8/-8     

@titusfortner titusfortner requested review from aguspe and p0deje October 22, 2025 06:01
@selenium-ci selenium-ci added the C-rb Ruby Bindings label Oct 22, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 22, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #5678
🔴 Investigate and resolve "Error: ConnectFailure (Connection refused)" occurring on
subsequent ChromeDriver instantiations on Ubuntu 16.04.4 with Selenium 3.9.0, Chrome 65,
and ChromeDriver 2.35.
Provide steps or fixes ensuring multiple ChromeDriver instances can be created without
connection refusal after the first instance.
🟡
🎫 #1234
🔴 Ensure Selenium 2.48 triggers JavaScript in link href on click() in Firefox 42 (regression
from 2.47.1).
Provide fix or behavior parity for click() event handling invoking href JavaScript.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 22, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid using shared global state

Set extra_headers and user_agent on the http instance instead of the
Http::Common class to avoid race conditions with concurrent drivers.

rb/lib/selenium/webdriver/remote/bridge.rb [676-688]

 def build_http_client(client_config)
   if client_config
     http = Http::Default.new(open_timeout: client_config.open_timeout,
                              read_timeout: client_config.read_timeout)
     http.proxy = client_config.proxy if client_config.proxy
 
-    Http::Common.extra_headers = client_config.extra_headers if client_config.extra_headers
-    Http::Common.user_agent = client_config.user_agent if client_config.user_agent
+    http.extra_headers = client_config.extra_headers if client_config.extra_headers
+    http.user_agent = client_config.user_agent if client_config.user_agent
   else
     http = Http::Default.new
   end
   http
 end
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical race condition caused by setting extra_headers and user_agent on a class variable, which is shared state across all driver instances.

High
High-level
Avoid using global HTTP configuration

The configuration for extra_headers and user_agent is currently applied globally
to the Http::Common class. This should be changed to apply to the specific HTTP
client instance to prevent race conditions when using multiple drivers.

Examples:

rb/lib/selenium/webdriver/remote/bridge.rb [682-683]
            Http::Common.extra_headers = client_config.extra_headers if client_config.extra_headers
            Http::Common.user_agent = client_config.user_agent if client_config.user_agent

Solution Walkthrough:

Before:

# File: rb/lib/selenium/webdriver/remote/bridge.rb

def build_http_client(client_config)
  if client_config
    http = Http::Default.new(...)
    http.proxy = client_config.proxy

    # These are class-level attributes, creating global state
    Http::Common.extra_headers = client_config.extra_headers
    Http::Common.user_agent = client_config.user_agent
  else
    http = Http::Default.new
  end
  http
end

After:

# File: rb/lib/selenium/webdriver/remote/bridge.rb

def build_http_client(client_config)
  if client_config
    http = Http::Default.new(...)
    http.proxy = client_config.proxy

    # These should be instance-level attributes
    http.extra_headers = client_config.extra_headers
    http.user_agent = client_config.user_agent
  else
    http = Http::Default.new
  end
  http
end

# File: rb/lib/selenium/webdriver/remote/http/common.rb
# ... remove class-level accessors and add instance-level ones
Suggestion importance[1-10]: 9

__

Why: This suggestion correctly identifies a critical design flaw where per-driver configurations are set on global class variables, which will cause race conditions and incorrect behavior in common parallel execution scenarios.

High
General
Improve clarity of error message

Update the error message in validate_server_url_args to correctly refer to
client_config instead of the misleading http_client.

rb/lib/selenium/webdriver/remote/bridge.rb [690-696]

 def validate_server_url_args(url, client_config)
   if url && client_config&.server_url
-    raise Error::WebDriverError, 'Cannot specify url in both keyword and http_client'
+    raise Error::WebDriverError, 'Cannot specify url in both keyword and client_config'
   elsif url.nil? && !client_config&.server_url
     raise Error::WebDriverError, 'No server URL provided'
   end
 end
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out a misleading error message and proposes a clearer alternative, which improves developer experience.

Low
  • Update

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.

3 participants