Skip to content
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.

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

[py] Remote connection use timeout from ClientConfig #14692

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Oct 31, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Should be merged after #14690

Fixes #14691

The idea in #14578 is still retained. Just moved default 120s to ClientConfig in corresponding driver init

Motivation and Context

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

Bug fix, Enhancement


Description

  • Introduced a ClientConfig object with a default timeout of 120 seconds for various WebDriver remote connections (Chromium, Firefox, Safari, IE).
  • Removed hardcoded timeout values and utilized the ClientConfig timeout in HTTP requests.
  • Enhanced the initialization of remote connections by setting default configurations, improving maintainability and flexibility.

Changes walkthrough 📝

Relevant files
Enhancement
remote_connection.py
Set default ClientConfig for Chromium remote connection   

py/selenium/webdriver/chromium/remote_connection.py

  • Added default ClientConfig with a 120s timeout.
  • Removed explicit remote_server_addr and keep_alive parameters from
    super() call.
  • +3/-2     
    remote_connection.py
    Set default ClientConfig for Firefox remote connection     

    py/selenium/webdriver/firefox/remote_connection.py

  • Added default ClientConfig with a 120s timeout.
  • Removed explicit remote_server_addr and keep_alive parameters from
    super() call.
  • +3/-2     
    webdriver.py
    Implement ClientConfig in IE WebDriver initialization       

    py/selenium/webdriver/ie/webdriver.py

  • Introduced ClientConfig for IE WebDriver.
  • Modified RemoteConnection to use client_config.
  • +3/-2     
    remote_connection.py
    Set default ClientConfig for Safari remote connection       

    py/selenium/webdriver/safari/remote_connection.py

  • Added default ClientConfig with a 120s timeout.
  • Removed explicit remote_server_addr and keep_alive parameters from
    super() call.
  • +3/-2     
    webdriver.py
    Implement ClientConfig in Safari WebDriver initialization

    py/selenium/webdriver/safari/webdriver.py

  • Introduced ClientConfig for Safari WebDriver.
  • Modified SafariRemoteConnection to use client_config.
  • +3/-2     
    Bug fix
    remote_connection.py
    Use ClientConfig timeout in remote connection requests     

    py/selenium/webdriver/remote/remote_connection.py

  • Removed hardcoded timeout parameter from _request.
  • Utilized timeout from ClientConfig.
  • +3/-3     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @VietND96 VietND96 changed the title Python read timeout [py] Remote connection use timeout from ClientConfig Oct 31, 2024
    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

    Potential Bug
    The removal of the hardcoded timeout parameter in the _request method might affect existing code that relies on this default value. Ensure that all callers of this method are updated to use the new ClientConfig timeout.

    Code Consistency
    The implementation of ClientConfig for IE WebDriver differs from other drivers. Consider standardizing the approach across all drivers for better maintainability.

    @VietND96 VietND96 requested a review from diemol October 31, 2024 02:04
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add error handling for timeout exceptions to improve robustness and provide informative error messages

    Consider adding error handling for potential timeout exceptions when making HTTP
    requests. This can improve the robustness of the code and provide more informative
    error messages.

    py/selenium/webdriver/remote/remote_connection.py [400]

    -response = self._conn.request(method, url, body=body, headers=headers, timeout=self._client_config.timeout)
    +try:
    +    response = self._conn.request(method, url, body=body, headers=headers, timeout=self._client_config.timeout)
    +except TimeoutError:
    +    raise WebDriverException(f"Request timed out after {self._client_config.timeout} seconds")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for timeout exceptions significantly enhances the robustness of the code by providing informative error messages and preventing unhandled exceptions. This is a valuable improvement for maintaining code reliability.

    8
    Allow custom timeout configuration to provide more flexibility in different usage scenarios

    Consider allowing the user to pass a custom timeout value when creating the
    ClientConfig object. This provides more flexibility in configuring the timeout for
    different environments or use cases.

    py/selenium/webdriver/ie/webdriver.py [53]

    -client_config = ClientConfig(remote_server_addr=self.service.service_url, keep_alive=keep_alive)
    +client_config = ClientConfig(
    +    remote_server_addr=self.service.service_url,
    +    keep_alive=keep_alive,
    +    timeout=options.timeout if hasattr(options, 'timeout') else None
    +)
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Allowing for a custom timeout configuration enhances flexibility and adaptability to different environments. This suggestion is beneficial as it provides users with the ability to tailor the timeout settings to their specific needs, improving the code's usability.

    7
    Make the timeout value configurable to allow for more flexibility in different usage scenarios

    Consider making the timeout value configurable instead of hardcoding it to 120
    seconds. This allows users to set custom timeout values based on their specific
    needs.

    py/selenium/webdriver/chromium/remote_connection.py [33-35]

    +DEFAULT_TIMEOUT = 120
     client_config = client_config or ClientConfig(
    -    remote_server_addr=remote_server_addr, keep_alive=keep_alive, timeout=120
    +    remote_server_addr=remote_server_addr, keep_alive=keep_alive, timeout=DEFAULT_TIMEOUT
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to make the timeout value configurable by introducing a constant improves flexibility, but it doesn't fully address the need for user-defined configurations. It offers a minor improvement by centralizing the timeout value, but doesn't significantly enhance the code's functionality.

    5

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 added the C-py label Oct 31, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: [Python] 4.26.0 request Read timed out. (read timeout=120) when init remote to Grid
    1 participant