Skip to content

[rb] Add websocket-port parameter to firefox service#15458

Merged
aguspe merged 22 commits intoSeleniumHQ:trunkfrom
aguspe:rb_add_argument_for_websocket_port
Apr 29, 2025
Merged

[rb] Add websocket-port parameter to firefox service#15458
aguspe merged 22 commits intoSeleniumHQ:trunkfrom
aguspe:rb_add_argument_for_websocket_port

Conversation

@aguspe
Copy link
Copy Markdown
Contributor

@aguspe aguspe commented Mar 19, 2025 •
edited
Loading

User description

Motivation and Context

This change is needed about mozilla/geckodriver#2218

By having the --websocket-port parameter by default we avoid port collisions when using BiDi with the geckodriver

The general issue for all the selenium bindings is #15451

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests

Description

  • Added --websocket-port parameter to Firefox service to avoid port collisions.

  • Introduced Support::Sockets.free_port method to dynamically allocate free ports.

  • Updated unit and integration tests to validate websocket port behavior.

  • Added type signature for Support::Sockets.free_port in RBS file.

Changes walkthrough 📝

Relevant files
Enhancement
service.rb
Add `--websocket-port` parameter to Firefox service           

rb/lib/selenium/webdriver/firefox/service.rb

  • Added --websocket-port parameter to service initialization.
  • Dynamically allocated free ports using Support::Sockets.free_port.
  • Ensured compatibility with existing arguments like --connect-existing.
    		• +9/-0     
    support.rb
    Add `Support::Sockets` requirement for port management     

    rb/lib/selenium/webdriver/support.rb

    • Required selenium/webdriver/support/sockets for port management.
    		 +1/-0     
    sockets.rb
    Introduce `Support::Sockets` module for port management   

    rb/lib/selenium/webdriver/support/sockets.rb

  • Added Support::Sockets module for managing free ports.
  • Implemented free_port method to find and return an available port.
    		• +35/-0   
    sockets.rbs
    Add type signature for `Support::Sockets.free_port`           

    rb/sig/selenium/web_driver/support/sockets.rbs

    • Added type signature for Support::Sockets.free_port method.
    		 +9/-0     
    Tests
    service_spec.rb
    Add integration tests for websocket port behavior               

    rb/spec/integration/selenium/webdriver/firefox/service_spec.rb

  • Added integration test to validate unique websocket ports for multiple
    services.
  • Ensured BiDi compatibility with websocket port allocation.
    		• +15/-0   
    service_spec.rb
    Update unit tests for websocket port parameter                     

    rb/spec/unit/selenium/webdriver/firefox/service_spec.rb

  • Updated unit tests to validate --websocket-port parameter behavior.
  • Tested scenarios with and without --connect-existing argument.
    		• +19/-4   
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 19, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    2218 - Fully compliant

    Compliant requirements:

    • Avoid port collisions when using BiDi with geckodriver

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

    Error Handling

    The code doesn't handle potential errors when trying to allocate a free port. If port allocation fails, there's no fallback mechanism or error handling.

     
      args << '--websocket-port'
  args << Support::Sockets.free_port.to_s
end
    Port Race Condition

    There's a potential race condition in the free_port method. The port is released after server.close and could be taken by another process before it's used by geckodriver.

     
    def self.free_port
  server = TCPServer.new('127.0.0.1', 0)
  port = server.addr[1]
  server.close
  port
end

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 19, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    ✅ Prevent redundant port allocation
    Suggestion Impact:The commit addresses the same issue as the suggestion by changing how the websocket port is determined, but uses a different approach. Instead of storing the port as an instance variable, it uses WebDriver::PortProber.above(4444) to determine the port.

    code diff:

    -            args << Support::Sockets.free_port.to_s
+            args << WebDriver::PortProber.above(4444).to_s

    The current implementation assigns a new websocket port for every service
    instance, even when reusing the same service object. This could lead to port
    allocation issues and resource leaks. Store the allocated port as an instance
    variable to ensure consistent port usage throughout the service lifecycle.

    rb/lib/selenium/webdriver/firefox/service.rb [29-36]

     def initialize(path: nil, port: nil, log: nil, args: nil)
   args ||= []
   unless args.any? { |arg| arg.include?('--connect-existing') }
+    @websocket_port = Support::Sockets.free_port
     args << '--websocket-port'
-    args << Support::Sockets.free_port.to_s
+    args << @websocket_port.to_s
   end
   super
 end

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    __

    Why: Storing the allocated port as an instance variable (@websocket_port) is a good practice that prevents potential resource leaks and ensures consistent port usage throughout the service lifecycle. This improves resource management and makes the port accessible for other methods.

    		Medium
    Add error handling

    The current implementation doesn't handle potential socket errors when
    allocating ports. In high-concurrency environments or when system resources are
    constrained, this could lead to unhandled exceptions. Add error handling to
    gracefully manage socket allocation failures.

    rb/lib/selenium/webdriver/support/sockets.rb [26-31]

     def self.free_port
   server = TCPServer.new('127.0.0.1', 0)
   port = server.addr[1]
   server.close
   port
+rescue StandardError => e
+  WebDriver.logger.warn("Failed to allocate free port: #{e.message}")
+  raise Error::WebDriverError, "Unable to find free port: #{e.message}"
 end
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    __

    Why: Adding error handling for socket operations is important for robustness, especially in high-concurrency environments. The suggestion properly catches exceptions, logs warnings, and raises a more specific error, which improves error reporting and system stability.

    		Low
    Learned
    best practice
    Provide default values directly in method parameters instead of using nil checks within the method body

    Instead of using the nil check with ||= for the args parameter, initialize it
    with a default empty array in the parameter definition. This makes the default
    value explicit at the method signature level and follows the best practice of
    providing meaningful defaults directly in the parameter list.

    rb/lib/selenium/webdriver/firefox/service.rb [29-36]

    -def initialize(path: nil, port: nil, log: nil, args: nil)
-  args ||= []
+def initialize(path: nil, port: nil, log: nil, args: [])
   unless args.any? { |arg| arg.include?('--connect-existing') }
     args << '--websocket-port'
     args << Support::Sockets.free_port.to_s
   end
   super
 end
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    		Low
    • Update

    @aguspe aguspe added the C-rb Ruby Bindings label Mar 19, 2025
    @aguspe aguspe changed the title Add --websocket-port parameter to firefox service [rb] Add --websocket-port parameter to firefox service Mar 19, 2025
    @aguspe aguspe requested a review from titusfortner March 19, 2025 21:41
    @aguspe aguspe changed the title [rb] Add --websocket-port parameter to firefox service [rb] Add websocket-port parameter to firefox service Mar 19, 2025
    @pandafy pandafy mentioned this pull request Mar 20, 2025
    Merged
    4 tasks
    p0deje
    p0deje reviewed Apr 12, 2025
    View reviewed changes
    @aguspe aguspe merged commit 3ff6f9a into SeleniumHQ:trunk Apr 29, 2025
    24 of 26 checks passed
    @tomhughes
    Copy link
    Copy Markdown
    Contributor

    tomhughes commented May 6, 2025

    This doesn't actually work if your testing framework launches multiple threads as they all try and use the same port and most of them fail - using --websocket-port=0 to get a dynamically allocated port would probably be better.

    @titusfortner
    Copy link
    Copy Markdown
    Member

    titusfortner commented May 6, 2025

    PortProber should create a new port dynamically per instance, is that not working for you? Can you crate a new issue with logs?

    @tomhughes tomhughes mentioned this pull request May 6, 2025
    Closed
    @tomhughes
    Copy link
    Copy Markdown
    Contributor

    tomhughes commented May 6, 2025

    I've created #15707.

    @tomhughes tomhughes mentioned this pull request May 26, 2025
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @p0deje p0deje p0deje left review comments

    @titusfortner titusfortner titusfortner approved these changes

    Assignees

    No one assigned

    Labels

    C-rb Ruby Bindings Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    4 participants

    @aguspe @tomhughes @titusfortner @p0deje