Skip to content

[grid] Set session-request-timeout as client readTimeout in RemoteNewSessionQueue#14272

Merged
diemol merged 2 commits intoSeleniumHQ:trunkfrom
NDViet:trunk
Jul 18, 2024
Merged

[grid] Set session-request-timeout as client readTimeout in RemoteNewSessionQueue#14272
diemol merged 2 commits intoSeleniumHQ:trunkfrom
NDViet:trunk

Conversation

@VietND96
Copy link
Copy Markdown
Member

@VietND96 VietND96 commented Jul 17, 2024 •
edited by qodo-code-review bot
Loading

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

[grid] Set session-request-timeout as client readTimeout in RemoteNewSessionQueue

Motivation and Context

In CI tests for Selenium Grid autoscaling on K8s in repo https://github.com/SeleniumHQ/docker-selenium. Sometime could see below error even set --session-request-timeout over the default 300 seconds

selenium.common.exceptions.SessionNotCreatedException: Message: Could not start a new session. Could not start a new session. Unable to create new session 
Host info: host: 'selenium-distributor-7496d64db6-k4kpk', ip: '10.244.202.139'
Build info: version: '4.23.0-SNAPSHOT', revision: '10b3305'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.5.0-1023-azure', java.version: '17.0.11'
Driver info: driver.version: unknown
Stacktrace:
    at org.openqa.selenium.grid.sessionqueue.SessionNotCreated.execute (SessionNotCreated.java:56)
    at org.openqa.selenium.remote.http.Route$TemplatizedRoute.handle (Route.java:192)
    at org.openqa.selenium.remote.http.Route.execute (Route.java:69)
    at org.openqa.selenium.grid.security.RequiresSecretFilter.lambda$apply$0 (RequiresSecretFilter.java:62)
    at org.openqa.selenium.remote.http.Filter$1.execute (Filter.java:63)
    at org.openqa.selenium.remote.http.Route$CombinedRoute.handle (Route.java:360)
    at org.openqa.selenium.remote.http.Route.execute (Route.java:69)
    at org.openqa.selenium.grid.sessionqueue.NewSessionQueue.execute (NewSessionQueue.java:128)
    at org.openqa.selenium.remote.http.Route$CombinedRoute.handle (Route.java:360)
    at org.openqa.selenium.remote.http.Route.execute (Route.java:69)
    at org.openqa.selenium.remote.AddWebDriverSpecHeaders.lambda$apply$0 (AddWebDriverSpecHeaders.java:35)
    at org.openqa.selenium.remote.ErrorFilter.lambda$apply$0 (ErrorFilter.java:44)
    at org.openqa.selenium.remote.http.Filter$1.execute (Filter.java:63)
    at org.openqa.selenium.remote.ErrorFilter.lambda$apply$0 (ErrorFilter.java:44)
    at org.openqa.selenium.remote.http.Filter$1.execute (Filter.java:63)
    at org.openqa.selenium.netty.server.SeleniumHandler.lambda$channelRead0$0 (SeleniumHandler.java:44)
    at java.util.concurrent.Executors$RunnableAdapter.call (None:-1)
    at java.util.concurrent.FutureTask.run (None:-1)
    at java.util.concurrent.ThreadPoolExecutor.runWorker (None:-1)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run (None:-1)
    at java.lang.Thread.run (None:-1)

I suspect that is due to timeout in HTTP client created by default config. So in this change, get session-request-timeout and set it as read timeout for HTTP client in RemoteNewSessionQueue

Can you also review the change is able to resolve the problem mentioned in #13718?

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

Description

  • Enhanced the RemoteNewSessionQueue to set the session-request-timeout as the client readTimeout.
  • Imported necessary classes (ClientConfig and Duration) to support the new functionality.
  • Modified the create method to include sessionRequestTimeout and configure the HttpClient accordingly.

Changes walkthrough 📝

Relevant files
Enhancement
RemoteNewSessionQueue.java
Set session-request-timeout as client readTimeout in
RemoteNewSessionQueue

java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java

  • Imported ClientConfig and Duration classes.
  • Added sessionRequestTimeout to RemoteNewSessionQueue.create method.
  • Configured HttpClient with sessionRequestTimeout using ClientConfig.
    		•  +7/-1     

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

    @VietND96
    [grid] Set session-request-timeout as client readTimeout in RemoteNew…
    1540761 
    …SessionQueue

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @qodo-code-review qodo-code-review bot added the P-enhancement PR with a new feature label Jul 17, 2024
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Jul 17, 2024

    PR Reviewer Guide 🔍

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

    Missing Exception Handling
    The new ClientConfig setup with readTimeout does not handle potential exceptions from uri.toURL() separately, which could obscure the source of MalformedURLException.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Jul 17, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Handle MalformedURLException from uri.toURL() within the same try block to improve error handling

    It's recommended to handle potential exceptions that might be thrown by uri.toURL()
    directly within the try block where it's used, rather than allowing it to propagate.
    This is because uri.toURL() can throw a MalformedURLException, which should be
    caught and handled appropriately to avoid application crashes.

    java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java [91]

    +URL baseUrl;
+try {
+  baseUrl = uri.toURL();
+} catch (MalformedURLException e) {
+  throw new UncheckedIOException("Invalid URL for session queue", e);
+}
 ClientConfig clientConfig =
-    defaultConfig().readTimeout(sessionRequestTimeout).baseUrl(uri.toURL());
+    defaultConfig().readTimeout(sessionRequestTimeout).baseUrl(baseUrl);
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion improves error handling by ensuring that the potential MalformedURLException is caught and handled immediately, preventing the application from crashing due to an unhandled exception.

    		9
    Add a null check for uri before using it to prevent NullPointerException

    To ensure that the ClientConfig is correctly configured with a non-null base URL,
    add a null check for uri before converting it to a URL. This prevents potential
    NullPointerException in scenarios where uri might be unexpectedly null.

    java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java [91]

    +if (uri == null) {
+  throw new IllegalArgumentException("Session queue URI must not be null");
+}
 ClientConfig clientConfig =
     defaultConfig().readTimeout(sessionRequestTimeout).baseUrl(uri.toURL());
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a null check for uri ensures that the code does not attempt to convert a null URI to a URL, which would result in a NullPointerException. This improves the robustness of the code.

    		8
    Enhancement
    Add validation for sessionRequestTimeout to prevent negative or excessively long durations

    Consider validating the sessionRequestTimeout to ensure it's not negative or
    excessively long, which could lead to unintended behavior or resource exhaustion.
    This validation should be done before using it to set the read timeout.

    java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java [83]

     Duration sessionRequestTimeout = new NewSessionQueueOptions(config).getSessionRequestTimeout();
+if (sessionRequestTimeout.isNegative() || sessionRequestTimeout.toMinutes() > 30) {
+  throw new IllegalArgumentException("Session request timeout is invalid");
+}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Validating sessionRequestTimeout ensures that the value is within a reasonable range, preventing potential issues related to negative or excessively long timeouts, which could lead to unintended behavior or resource exhaustion.

    		8
    Maintainability
    Refactor configuration and client creation into separate methods to improve code maintainability

    To improve the maintainability and readability of the code, consider extracting the
    configuration and client creation logic into separate methods. This refactoring
    helps in isolating changes and makes unit testing easier.

    java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java [81-87]

    -Tracer tracer = new LoggingOptions(config).getTracer();
-URI uri = new NewSessionQueueOptions(config).getSessionQueueUri();
-Duration sessionRequestTimeout = new NewSessionQueueOptions(config).getSessionRequestTimeout();
-HttpClient.Factory clientFactory = new NetworkOptions(config).getHttpClientFactory(tracer);
-SecretOptions secretOptions = new SecretOptions(config);
-Secret registrationSecret = secretOptions.getRegistrationSecret();
+Tracer tracer = getTracerFromConfig(config);
+URI uri = getSessionQueueUriFromConfig(config);
+Duration sessionRequestTimeout = getSessionRequestTimeoutFromConfig(config);
+HttpClient.Factory clientFactory = getHttpClientFactoryFromConfig(config, tracer);
+Secret registrationSecret = getRegistrationSecretFromConfig(config);
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Extracting configuration and client creation logic into separate methods enhances code readability and maintainability, making it easier to manage and test individual components.

    		7

    @codecov
    Copy link
    Copy Markdown

    codecov bot commented Jul 17, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 57.46%. Comparing base (5e8d6a1) to head (1540761).
    Report is 15 commits behind head on trunk.

    Additional details and impacted files 
    @@            Coverage Diff             @@
##            trunk   #14272      +/-   ##
==========================================
+ Coverage   57.18%   57.46%   +0.28%     
==========================================
  Files          89       89              
  Lines        5514     5549      +35     
  Branches      232      228       -4     
==========================================
+ Hits         3153     3189      +36     
- Misses       2129     2132       +3     
+ Partials      232      228       -4

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @VietND96
    Copy link
    Copy Markdown
    Member Author

    VietND96 commented Jul 18, 2024

    @diemol, @pujagani, can you have a review on this?

    diemol
    diemol approved these changes Jul 18, 2024
    View reviewed changes
    Copy link
    Copy Markdown
    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.

    This makes sense, thank you, @VietND96!

    @diemol diemol merged commit 8cf9a59 into SeleniumHQ:trunk Jul 18, 2024
    This was referenced Jul 18, 2024
    Closed
    Closed
    Closed
    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @VietND96 @sandeepsuryaprasad
    [grid] Set session-request-timeout as client readTimeout in RemoteNew…
    57e212e 
    …SessionQueue (SeleniumHQ#14272)

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Nov 8, 2024
    Open
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @VietND96 @M1troll
    [grid] Set session-request-timeout as client readTimeout in RemoteNew…
    547f02c 
    …SessionQueue (SeleniumHQ#14272)

Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @diemol diemol diemol approved these changes

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @VietND96 @diemol