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

[VietND96]

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

[qodo-merge-pro]

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-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	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]

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]

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

[diemol]

This makes sense, thank you, @VietND96!