[grid][java]: session-timeout set connection timeout in RemoteNode

[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

Motivation and Context

Try something for #13340
session-timeout is an attribute of all node types and can get in part of NodeStatus
Use session-timeout to set connection timeout and read timeout for HttpClient config in RemoteNode

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.

---

Type

enhancement

---

Description

• Added sessionTimeout to NodeStatus and updated JSON handling.
• Enhanced various node types and distributor classes to include and manage sessionTimeout.
• Updated tests across multiple classes to incorporate sessionTimeout in node configurations and assertions.

---

Changes walkthrough

	Relevant files
Enhancement	9 files NodeStatus.java Add session timeout to NodeStatus                                                java/src/org/openqa/selenium/grid/data/NodeStatus.java Added sessionTimeout field to NodeStatus class. Updated constructors and JSON serialization/deserialization to handle sessionTimeout. +22/-1    AddNode.java Include session timeout in node creation                                  java/src/org/openqa/selenium/grid/distributor/AddNode.java Modified execute method to include sessionTimeout when creating a new node. +1/-0      GridModel.java Handle session timeout in GridModel                                            java/src/org/openqa/selenium/grid/distributor/GridModel.java Updated rewrite and amend methods to handle sessionTimeout. +2/-0      LocalDistributor.java Adjust node registration to include session timeout            java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java Adjusted register method to include sessionTimeout in node creation. +1/-0      Node.java Add session timeout management in Node base class                java/src/org/openqa/selenium/grid/node/Node.java Added sessionTimeout to the Node class and updated constructors. Provided a getter for sessionTimeout. +12/-0    OneShotNode.java Manage session timeout in OneShotNode                                        java/src/org/openqa/selenium/grid/node/k8s/OneShotNode.java Included sessionTimeout in the constructor and during node creation. +4/-1      LocalNode.java Update LocalNode to handle session timeout                              java/src/org/openqa/selenium/grid/node/local/LocalNode.java Updated constructors to handle sessionTimeout. +7/-1      RemoteNode.java Enhance RemoteNode with session timeout configuration        java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java Updated constructors to handle sessionTimeout. Configured HTTP client to use sessionTimeout for read timeout. +8/-3      GridRedisClient.java Include session timeout in Redis client node retrieval      java/src/org/openqa/selenium/redis/GridRedisClient.java Updated getNode method to include sessionTimeout in the node status. +1/-0
Tests	4 files NodeStatusTest.java Update NodeStatus tests to include session timeout              java/test/org/openqa/selenium/grid/data/NodeStatusTest.java Updated test to include sessionTimeout in NodeStatus. +1/-0      AddingNodesTest.java Update distributor tests for session timeout handling        java/test/org/openqa/selenium/grid/distributor/AddingNodesTest.java Updated tests to handle sessionTimeout in node status events and status retrieval. +2/-0      DefaultSlotSelectorTest.java Update slot selector tests to include session timeout        java/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java Adjusted node creation in tests to include sessionTimeout. +1/-0      NodeTest.java Update Node tests to handle session timeout                            java/test/org/openqa/selenium/grid/node/NodeTest.java Updated node creation in tests to handle sessionTimeout. +4/-0

---

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

[VietND96]

@diemol, can you also review this one?

[qodo-merge-pro]

PR Description updated to latest commit (63872ee)

• Copy walkthrough table to "Files Changed" Tab

[qodo-merge-pro]

PR Review

(Review updated until commit 2eac37a)

⏱️ Estimated effort to review [1-5]	3, because the PR involves multiple changes across various files, including core classes like Node, RemoteNode, and NodeStatus. The changes are not overly complex but require a thorough understanding of the existing architecture and the implications of adding session timeout settings.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: Ensure that all constructors and methods that now include sessionTimeout properly handle this new parameter. There is a risk of null values or incorrect timeout settings being propagated if not handled correctly.
	Performance Concern: Adding session timeout to the HTTP client configuration could potentially impact performance. It's important to test how these changes affect the system's responsiveness and timeout behavior under different network conditions.
🔒 Security concerns	No

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[qodo-merge-pro]

PR Code Suggestions

Category	Suggestions
Enhancement	Add validation to ensure the session timeout is not negative or zero. The constructor Node should validate the sessionTimeout parameter to ensure it is not negative or zero. This will prevent potential runtime issues related to invalid timeout values. java/src/org/openqa/selenium/grid/node/Node.java [133] -this.sessionTimeout = Require.nonNull("Session timeout", sessionTimeout); +this.sessionTimeout = Require.nonNegative("Session timeout", sessionTimeout);
Performance	Modify the ClientConfig to have different values for connection and read timeouts. When setting the ClientConfig for the RemoteNode, it is important to ensure that the read and connection timeouts are set appropriately to handle network variability. Consider adding a buffer or using different values for connection and read timeouts. java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java [95-99] ClientConfig clientConfig = defaultConfig() .connectionTimeout(this.getSessionTimeout()) - .readTimeout(this.getSessionTimeout()) + .readTimeout(this.getSessionTimeout().plusSeconds(30)) .baseUrl(fromUri(externalUri));
Maintainability	Add logging for the session timeout value in the Node class. In the Node class, when setting the sessionTimeout, it's beneficial to log this value for debugging purposes, especially when issues related to session management occur. java/src/org/openqa/selenium/grid/node/Node.java [133] this.sessionTimeout = Require.nonNull("Session timeout", sessionTimeout); +System.out.println("Session timeout set to: " + sessionTimeout);
Bug	Ensure the session timeout is non-null before passing to the superclass constructor in OneShotNode. Ensure that the sessionTimeout parameter is passed correctly to the superclass constructor in the OneShotNode class to maintain consistency and avoid potential bugs. java/src/org/openqa/selenium/grid/node/k8s/OneShotNode.java [118] -super(tracer, id, uri, registrationSecret, sessionTimeout); +super(tracer, id, uri, registrationSecret, Require.nonNull("Session timeout", sessionTimeout));
Best practice	Validate the sessionTimeout parameter in the LocalNode constructor. In the LocalNode constructor, ensure that the sessionTimeout is passed correctly and validated similarly to other parameters to prevent inconsistencies. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [157] -super(tracer, new NodeId(UUID.randomUUID()), uri, registrationSecret, sessionTimeout); +super(tracer, new NodeId(UUID.randomUUID()), uri, registrationSecret, Require.nonNull("Session timeout", sessionTimeout));

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[qodo-merge-pro]

PR Description updated to latest commit (2eac37a)

• Copy walkthrough table to "Files Changed" Tab

[qodo-merge-pro]

Persistent review updated to latest commit 2eac37a

[qodo-merge-pro]

PR Code Suggestions

Category	Suggestions
Bug	Add null check for sessionTimeout to ensure it is not null and positive. Ensure that the sessionTimeout parameter is validated for null values to prevent potential NullPointerExceptions when the timeout is used. java/src/org/openqa/selenium/grid/node/Node.java [133] -this.sessionTimeout = Require.positive("Session timeout", sessionTimeout); +this.sessionTimeout = Require.nonNullAndPositive("Session timeout", sessionTimeout);
Enhancement	Add a fallback for sessionTimeout in ClientConfig to handle potential null values. Modify the ClientConfig to include a fallback timeout value in case sessionTimeout is not set, ensuring robustness. java/src/org/openqa/selenium/grid/node/remote/RemoteNode.java [95-96] ClientConfig clientConfig = - defaultConfig().readTimeout(this.getSessionTimeout()).baseUrl(fromUri(externalUri)); + defaultConfig().readTimeout(Optional.ofNullable(this.getSessionTimeout()).orElse(Duration.ofSeconds(30))).baseUrl(fromUri(externalUri));
Best practice	Ensure sessionTimeout is within a reasonable range to avoid resource hogging. Add a check to ensure that sessionTimeout is not excessively long, which could cause resources to be held unnecessarily. java/src/org/openqa/selenium/grid/node/k8s/OneShotNode.java [118] -super(tracer, id, uri, registrationSecret, Require.positive(sessionTimeout)); +super(tracer, id, uri, registrationSecret, Require.positiveAndNotExcessive("Session timeout", sessionTimeout, Duration.ofHours(1)));
Maintainability	Log a warning if sessionTimeout is set to a value that might negatively impact performance. Since sessionTimeout is crucial for node status, consider logging a warning if it is set to a very low value which might affect node performance. java/src/org/openqa/selenium/grid/data/NodeStatus.java [67] this.sessionTimeout = sessionTimeout; +if (sessionTimeout.compareTo(Duration.ofMinutes(1)) < 0) { + Logger.warn("Session timeout is set very low. It might affect node performance."); +}
	Refactor sessionTimeout handling in the constructor for clarity and maintainability. Refactor the constructor to handle sessionTimeout more explicitly by separating its validation from the super constructor call for clarity. java/src/org/openqa/selenium/grid/node/local/LocalNode.java [157-162] -super( - tracer, - new NodeId(UUID.randomUUID()), - uri, - registrationSecret, - Require.positive(sessionTimeout)); +Duration validatedTimeout = Require.positive("Session timeout", sessionTimeout); +super(tracer, new NodeId(UUID.randomUUID()), uri, registrationSecret, validatedTimeout);

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[diemol]

@VietND96 can you please update your fork?

[diemol]

Need to update the fork.

[VietND96]

@diemol, I updated the fork. Also, I found that the test is used to define a CustomNode, and it can be added. So, I updated the CustomNode with attr sessionTimeout, which can be set when creating. In the test, it asserts that getSessionTimeout() is working. Check if the update for the test makes sense.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.70%. Comparing base (fe2edbd) to head (3548225).
Report is 2 commits behind head on trunk.

❗ Current head 3548225 differs from pull request most recent head e199835. Consider uploading reports for the commit e199835 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #13854   +/-   ##
=======================================
  Coverage   58.70%   58.70%           
=======================================
  Files          86       86           
  Lines        5296     5296           
  Branches      226      226           
=======================================
  Hits         3109     3109           
  Misses       1961     1961           
  Partials      226      226           

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