selenium icon indicating copy to clipboard operation
selenium copied to clipboard
verified publisher icon SeleniumHQ

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

Open VietND96 opened this issue 1 year ago • 8 comments

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 https://github.com/SeleniumHQ/selenium/issues/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)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [x] 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 avatar Apr 22 '24 16:04 VietND96

    @diemol, can you also review this one?

    VietND96 avatar Apr 22 '24 16:04 VietND96

    PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/63872ee43451ad557dcf21033971aec25fd1f5af)

    qodo-code-review[bot] avatar Apr 22 '24 16:04 qodo-code-review[bot]

    PR Review

    (Review updated until commit https://github.com/SeleniumHQ/selenium/commit/2eac37a01de25b9762b7e0fde4d9780bc06ca69c)

    ⏱️ 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=...

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

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

    qodo-code-review[bot] avatar Apr 22 '24 16:04 qodo-code-review[bot]

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    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=...

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

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

    qodo-code-review[bot] avatar Apr 22 '24 16:04 qodo-code-review[bot]

    PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/2eac37a01de25b9762b7e0fde4d9780bc06ca69c)

    qodo-code-review[bot] avatar Apr 26 '24 05:04 qodo-code-review[bot]

    Persistent review updated to latest commit https://github.com/SeleniumHQ/selenium/commit/2eac37a01de25b9762b7e0fde4d9780bc06ca69c

    qodo-code-review[bot] avatar Apr 26 '24 05:04 qodo-code-review[bot]

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    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=...

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

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

    qodo-code-review[bot] avatar Apr 26 '24 05:04 qodo-code-review[bot]

    @VietND96 can you please update your fork?

    diemol avatar May 06 '24 08:05 diemol

    @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.

    VietND96 avatar May 06 '24 12:05 VietND96

    Codecov Report

    All modified and coverable lines are covered by tests :white_check_mark:

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

    :exclamation: 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

    :umbrella: View full report in Codecov by Sentry.
    :loudspeaker: Have feedback on the report? Share it here.

    codecov[bot] avatar May 06 '24 12:05 codecov[bot]