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

Allow setting JDKHttpClient connectionTimeout, readTimeout, version via system property #14306

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jul 25, 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

Allow setting JDKHttpClient connectionTimeout, readTimeout, version via system property

Motivation and Context

Fix: #14258
Proactive to set different config for readTimeout, connectionTimeout, and version via system properties
webdriver.httpclient.connectionTimeout, webdriver.httpclient.readTimeout, webdriver.httpclient.version

In Java bindings, we are able to create ClientConfig and insert via RemoteWebDriver.builder()
However, all other bindings will be implemented by #12368. Hence, fetching from system env would also be a potential (workaround) solution for all bindings.

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 support for setting JDKHttpClient connectionTimeout, readTimeout, and version via system properties.
  • Modified the ClientConfig class to read these properties and apply them to the default configuration.
  • Added a test case to verify that the ClientConfig correctly reads and applies these system properties.
  • Ensured that system properties are cleared after the test to avoid side effects.

Changes walkthrough 📝

Relevant files
Enhancement
ClientConfig.java
Add system property support for HTTP client configuration

java/src/org/openqa/selenium/remote/http/ClientConfig.java

  • Added system property support for connectionTimeout, readTimeout, and
    version.
  • Modified default configuration to use system properties.
  • +5/-3     
    Tests
    HttpClientTestBase.java
    Add test for HTTP client configuration via system properties

    java/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java

  • Added a test to verify configuration from system properties.
  • Ensured system properties are cleared after the test.
  • +30/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    …ia system property
    
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    There is no error handling for parsing the system properties to long values. If the properties are not valid longs, this will cause a NumberFormatException.

    Default Value Clarification
    The default value for 'webdriver.httpclient.version' is set to null if not specified. It might be more appropriate to set a default HTTP version explicitly for clarity and to avoid potential null issues.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling for number parsing from system properties to prevent crashes

    Replace the direct use of System.getProperty with a method that handles potential
    NumberFormatException when parsing long values. This will prevent the application
    from crashing due to improperly formatted system properties.

    java/src/org/openqa/selenium/remote/http/ClientConfig.java [66-69]

     Duration.ofSeconds(
    -    Long.parseLong(System.getProperty("webdriver.httpclient.connectionTimeout", "10"))),
    +    safeParseLong(System.getProperty("webdriver.httpclient.connectionTimeout", "10"))),
     Duration.ofSeconds(
    -    Long.parseLong(System.getProperty("webdriver.httpclient.readTimeout", "180"))),
    +    safeParseLong(System.getProperty("webdriver.httpclient.readTimeout", "180"))),
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug where improperly formatted system properties could cause a NumberFormatException, leading to application crashes. Adding error handling improves robustness.

    9
    Enhancement
    Validate HTTP version from system properties to ensure compatibility

    Ensure that the HTTP version property is validated against supported versions before
    setting it, to avoid potential issues with unsupported HTTP versions.

    java/src/org/openqa/selenium/remote/http/ClientConfig.java [74]

    -System.getProperty("webdriver.httpclient.version", null)
    +validateHttpVersion(System.getProperty("webdriver.httpclient.version", null))
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Validating the HTTP version ensures that only supported versions are used, which can prevent potential issues related to unsupported HTTP versions. This is a valuable enhancement.

    8
    Best practice
    Replace Thread.sleep with CountDownLatch for more reliable delay handling in tests

    Use a more robust approach for simulating delays and handling interruptions in
    tests, such as using CountDownLatch or Semaphore instead of Thread.sleep, which can
    lead to flaky tests.

    java/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java [211]

    -Thread.sleep(1100);
    +CountDownLatch latch = new CountDownLatch(1);
    +latch.await(1100, TimeUnit.MILLISECONDS);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using CountDownLatch or Semaphore instead of Thread.sleep can make tests more reliable and less flaky, which is a best practice for writing robust tests.

    7
    Maintainability
    Encapsulate setting and clearing system properties in test methods

    Encapsulate the system property setting and clearing in a separate method to reduce
    duplication and improve test maintainability.

    java/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java [218-232]

    -System.setProperty("webdriver.httpclient.connectionTimeout", "1");
    -System.setProperty("webdriver.httpclient.readTimeout", "300");
    -System.setProperty("webdriver.httpclient.version", "HTTP_1_1");
    -System.clearProperty("webdriver.httpclient.connectionTimeout");
    -System.clearProperty("webdriver.httpclient.readTimeout");
    -System.clearProperty("webdriver.httpclient.version");
    +setTestProperties("1", "300", "HTTP_1_1");
    +clearTestProperties();
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Encapsulating the setting and clearing of system properties reduces code duplication and improves maintainability, making the test code cleaner and easier to manage.

    6

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    Thank you, @VietND96!

    @diemol diemol merged commit 2280161 into SeleniumHQ:trunk Jul 29, 2024
    9 of 11 checks passed
    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]: Selenium Grid problem with processing requests from JDKHttpClient
    2 participants