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

[js] Close CDP websocket connection on driver.quit #14501

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Sep 16, 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

When running the CDP tests against the Selenium Grid, the Node process does not stop because the CDP websocket connection is open. The Node process stops if the Selenium Grid is shut down and that causes the websocket connection to close. Ideally, we should be closing the websocket connection when we close the session.

Motivation and Context

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

Bug fix


Description

  • Added logic to close the CDP websocket connection when driver.quit is called.
  • Prevents the Node.js process from hanging by ensuring the websocket connection is closed, especially when running CDP sessions against Selenium Grid.

Changes walkthrough 📝

Relevant files
Bug fix
webdriver.js
Ensure websocket connection is closed on driver quit         

javascript/node/selenium-webdriver/lib/webdriver.js

  • Added logic to close the websocket connection on driver.quit.
  • Prevents node process from hanging when running CDP sessions against
    Selenium Grid.
  • +8/-0     

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Error Handling
    The new code doesn't include error handling for the websocket close operation, which could potentially throw an error and affect the quit process.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Check if the WebSocket connection is open before closing it

    Consider adding a check to ensure that this._wsConnection is not only defined but
    also in an open state before attempting to close it. This can prevent potential
    errors if the connection is already closed.

    javascript/node/selenium-webdriver/lib/webdriver.js [792-794]

    -if (this._wsConnection !== undefined) {
    +if (this._wsConnection && this._wsConnection.readyState === WebSocket.OPEN) {
       this._wsConnection.close()
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion is highly relevant as it adds a check to ensure the WebSocket connection is open before attempting to close it, preventing potential errors if the connection is already closed.

    9
    Error handling
    Add error handling when closing the WebSocket connection

    Consider adding error handling when closing the WebSocket connection to gracefully
    handle any potential errors during the closing process.

    javascript/node/selenium-webdriver/lib/webdriver.js [792-794]

     if (this._wsConnection !== undefined) {
    -  this._wsConnection.close()
    +  try {
    +    this._wsConnection.close()
    +  } catch (error) {
    +    console.error('Error closing WebSocket connection:', error)
    +  }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling is a good practice to gracefully manage any potential errors during the WebSocket closing process, enhancing the robustness of the code.

    8
    Maintainability
    Add a comment explaining potential undefined state of WebSocket connection

    Consider adding a comment explaining why this._wsConnection might be undefined, to
    improve code maintainability and understanding for future developers.

    javascript/node/selenium-webdriver/lib/webdriver.js [792-794]

    +// The WebSocket connection might be undefined if CDP was not used or if the connection failed to establish
     if (this._wsConnection !== undefined) {
       this._wsConnection.close()
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: While adding a comment can improve code maintainability and understanding, it is not as crucial as the other suggestions. It provides context for future developers, which is beneficial but not critical.

    6

    @pujagani pujagani merged commit 7c8b46d into SeleniumHQ:trunk Sep 16, 2024
    10 of 11 checks passed
    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.

    1 participant