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] Avoid waiting indefinitely on a frozen chromedriver process #14578

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

dbernhard-0x7CD
Copy link
Contributor

@dbernhard-0x7CD dbernhard-0x7CD commented Oct 8, 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

By default requests.request waits indefinitely until it gets a response. This is unfortunate if the chromedriver process froze for some reason.
This PR adds a default timeout of two minutes to those requests.

Motivation and Context

Fixes waiting indefinitely because of a frozen chromedriver process.

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


Description

  • Added a default timeout of 2 minutes to HTTP requests in the remote_connection.py to prevent indefinite waiting due to a frozen chromedriver process.
  • Updated the _request method to accept a timeout parameter and applied it to HTTP requests.

Changes walkthrough 📝

Relevant files
Bug fix
remote_connection.py
Add timeout parameter to HTTP requests in remote connection

py/selenium/webdriver/remote/remote_connection.py

  • Added a timeout parameter with a default value of 120 seconds to the
    _request method.
  • Updated HTTP request calls to include the timeout parameter.
  • +3/-3     

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

    @CLAassistant
    Copy link

    CLAassistant commented Oct 8, 2024

    CLA assistant check
    All committers have signed the CLA.

    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 Side Effect
    The new timeout parameter might affect existing implementations that rely on longer execution times.

    Error Handling
    The PR doesn't include error handling for timeout exceptions, which could lead to unexpected behavior.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Implement specific error handling for request timeouts to provide clearer feedback

    Implement error handling for timeout exceptions. This will provide more informative
    feedback to users when a request times out.

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

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

    Why: Adding error handling for timeout exceptions improves the robustness of the code by providing clear feedback when a request times out, which is crucial for debugging and user experience.

    8
    Enhancement
    Allow customizable timeout values for HTTP requests to accommodate various network conditions and use cases

    Consider adding a parameter to allow users to customize the timeout value. This
    would provide more flexibility for different use cases where longer or shorter
    timeouts might be necessary.

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

    -def _request(self, method, url, body=None, timeout=120):
    +def _request(self, method, url, body=None, timeout=None):
    +    timeout = timeout or self.default_timeout  # Use a class attribute for default
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances flexibility by allowing users to specify custom timeout values, which can be beneficial for different network conditions. However, it requires additional implementation to define and manage a class attribute for the default timeout.

    7
    Best practice
    Use a context manager for connection requests to ensure proper resource handling

    Consider using a context manager for the self._conn.request() call to ensure proper
    resource management, especially in case of exceptions.

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

    -response = self._conn.request(method, url, body=body, headers=headers, timeout=timeout)
    +with self._conn as conn:
    +    response = conn.request(method, url, body=body, headers=headers, timeout=timeout)
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a context manager for the connection request ensures that resources are properly managed, especially in the event of an exception. This is a good practice for resource management, although the current implementation may already handle resources adequately.

    6

    💡 Need additional feedback ? start a PR chat

    @pujagani pujagani added the python Pull requests that update Python code label Oct 10, 2024
    @dbernhard-0x7CD dbernhard-0x7CD changed the title Avoid waiting indefinitely on a frozen chromedriver process [py]: Avoid waiting indefinitely on a frozen chromedriver process Oct 11, 2024
    @dbernhard-0x7CD dbernhard-0x7CD changed the title [py]: Avoid waiting indefinitely on a frozen chromedriver process [py] Avoid waiting indefinitely on a frozen chromedriver process Oct 11, 2024
    @AutomatedTester
    Copy link
    Member

    Thanks for the PR!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix python Pull requests that update Python code Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants