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

[java]: allow setting custom timeout for DevTools #14931

Merged
merged 5 commits into from
Jan 10, 2025

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Dec 23, 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

Fixes #14912

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, Bug fix


Description

  • Added an overloaded constructor to set custom timeouts in DevTools.

  • Introduced a setTimeout method to validate and update timeout duration.

  • Ensured default timeout remains at 10 seconds for backward compatibility.

  • Improved error handling for invalid timeout values.


Changes walkthrough 📝

Relevant files
Enhancement
DevTools.java
Support custom timeout and validation in `DevTools`           

java/src/org/openqa/selenium/devtools/DevTools.java

  • Added an overloaded constructor to accept custom timeout.
  • Introduced a setTimeout method for timeout validation and assignment.
  • Updated default timeout initialization for flexibility.
  • Enhanced error handling for timeout validation.
  • +15/-1   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @navin772 navin772 marked this pull request as ready for review January 9, 2025 06:19
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 9, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Thread Safety

    The timeout field is not marked as volatile or synchronized, which could cause visibility issues in multi-threaded scenarios when timeout is updated via setTimeout

    private Duration timeout = Duration.ofSeconds(10);

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Add an upper bound validation to prevent excessive timeout values that could lead to resource issues

    Add an upper bound validation for the timeout value to prevent potential resource
    exhaustion or performance issues. A reasonable maximum timeout (e.g., 5 minutes)
    should be enforced.

    java/src/org/openqa/selenium/devtools/DevTools.java [222-228]

     public void setTimeout(Duration timeout) {
       Require.nonNull("Timeout duration", timeout);
       if (timeout.isNegative() || timeout.isZero()) {
         throw new IllegalArgumentException("Timeout must be positive");
       }
    +  if (timeout.toMinutes() > 5) {
    +    throw new IllegalArgumentException("Timeout cannot exceed 5 minutes");
    +  }
       this.timeout = timeout;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding an upper bound for timeout values is a good practice to prevent resource exhaustion and system hangs. The suggestion provides a reasonable limit of 5 minutes and includes proper validation logic.

    7

    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.

    How would an end user set the timeout locally or in the Grid?

    @navin772
    Copy link
    Contributor Author

    navin772 commented Jan 9, 2025

    @diemol I am not much clear about it. Initially, I thought the user can create a DevTools object with custom timeout, but I think that is not convenient.
    Any suggestions/feedback are appreciated.

    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.

    Thinking more about this... The DevTools code will go away when we move to BiDi completely.

    So, adding a whole new interface seems a bit to much in this case. Maybe we can just increase the timeout to 20 or 30 seconds and call it a day.

    I don't think we should invest in this complexity considering this will go away.

    What do you think?

    @navin772
    Copy link
    Contributor Author

    navin772 commented Jan 9, 2025

    @diemol I will ask the issue creator for their approx timeout value and we can update it here if its upto 30 seconds, or else if it is 30+, we can update to 30 and tell them to wait for BiDi.

    @diemol
    Copy link
    Member

    diemol commented Jan 9, 2025

    Sounds good.

    @navin772
    Copy link
    Contributor Author

    @diemol @pujagani I have increased the timeout to 30s for CDP as discussed above.

    We can merge this and work on configurable timeouts for BiDi. I can give it a try.

    @navin772 navin772 requested a review from diemol January 10, 2025 05:19
    @diemol diemol merged commit 8740f13 into SeleniumHQ:trunk Jan 10, 2025
    1 check passed
    @navin772 navin772 deleted the java-devtools-timeout branch January 10, 2025 13:52
    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]: DevTools timeout is hardcoded and not sufficient for some commands
    2 participants