Skip to content

[js] Close BiDi websocket connection#14507

Merged
pujagani merged 7 commits intoSeleniumHQ:trunkfrom
pujagani:close-bidi-socket-connection
Oct 4, 2024
Merged

[js] Close BiDi websocket connection#14507
pujagani merged 7 commits intoSeleniumHQ:trunkfrom
pujagani:close-bidi-socket-connection

Conversation

@pujagani
Copy link
Copy Markdown
Contributor

@pujagani pujagani commented Sep 17, 2024 •
edited by qodo-code-review Bot
Loading

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

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

  • Renamed the websocket connection variable from this._wsConnection to this._cdpWsConnection to improve clarity and consistency.
  • Added functionality to close the BiDi websocket connection to prevent node processes from hanging.
  • Modified the getBidi method to ensure the BiDi connection is initialized only once.
  • Updated all websocket event handlers to use the newly renamed this._cdpWsConnection variable.

Changes walkthrough 📝

Relevant files
Enhancement
webdriver.js
Enhance websocket connection management and naming             

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

  • Renamed this._wsConnection to this._cdpWsConnection for clarity.
  • Added logic to close the BiDi websocket connection.
  • Ensured BiDi connection is initialized only once.
  • Updated websocket event handlers to use the new variable name.
    		•  +23/-15 

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

    @pujagani pujagani marked this pull request as ready for review September 17, 2024 06:42
    @pujagani pujagani marked this pull request as draft September 17, 2024 06:42
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Sep 17, 2024 •
    edited
    Loading

    PR Reviewer Guide 🔍

    (Review updated until commit 243c068)

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

    Potential Memory Leak
    The getBidi() method creates a new BIDI connection if it doesn't exist, but there's no mechanism to close this connection when it's no longer needed.

    Error Handling
    The error handling in the WebSocket connection setup doesn't provide specific error information, which might make debugging issues harder.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Sep 17, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    Latest suggestions up to 243c068

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

    Consider using a more specific check for the existence of this._cdpWsConnection,
    such as this._cdpWsConnection && this._cdpWsConnection.readyState ===
    WebSocket.OPEN, to ensure the connection is actually open before attempting to close
    it.

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

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

    Why: This suggestion improves the robustness of the code by ensuring that the WebSocket connection is actually open before attempting to close it, which prevents potential errors and aligns with best practices.

    		9
    Possible issue
    Add a check for the existence of a required value before using it

    Consider adding a check to ensure that WebSocketUrl is not undefined before using
    it, to prevent potential runtime errors.

    javascript/node/selenium-webdriver/lib/webdriver.js [1298-1299]

     let WebSocketUrl = caps['map_'].get('webSocketUrl')
-this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1'))
+if (WebSocketUrl) {
+  this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1'))
+} else {
+  throw new Error('WebSocket URL is not available in capabilities')
+}
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion is crucial as it prevents runtime errors by ensuring that a necessary value is present before use, which is a fundamental practice in defensive programming.

    		9
    Error handling
    Add error handling when closing connections

    Consider adding error handling when closing the BiDi connection to prevent potential
    exceptions from interrupting the quit process.

    javascript/node/selenium-webdriver/lib/webdriver.js [797-799]

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

    Why: Adding error handling when closing connections is a good practice to prevent unexpected exceptions from disrupting the application flow, especially during critical operations like quitting.

    		8
    Maintainability
    Use consistent naming conventions for variables

    Consider using a more descriptive variable name for WebSocketUrl to follow
    JavaScript naming conventions and improve code readability.

    javascript/node/selenium-webdriver/lib/webdriver.js [1298-1299]

    -let WebSocketUrl = caps['map_'].get('webSocketUrl')
-this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1'))
+let webSocketUrl = caps['map_'].get('webSocketUrl')
+this._bidiConnection = new BIDI(webSocketUrl.replace('localhost', '127.0.0.1'))
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using camelCase for variable names in JavaScript is a standard convention that enhances code readability and maintainability, making this a useful suggestion for consistency.

    		7

    💡 Need additional feedback ? start a PR chat

    Previous suggestions

    Suggestions up to commit 878dbab
    CategorySuggestion                                                                                                                                    Score
    Best practice
    Check if the WebSocket connection is open before closing it

    Consider using a more specific check for the existence of this._cdpWsConnection,
    such as this._cdpWsConnection && this._cdpWsConnection.readyState ===
    WebSocket.OPEN, to ensure the connection is actually open before attempting to close
    it.

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

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

    Why: This suggestion improves the robustness of the code by ensuring that the WebSocket connection is actually open before attempting to close it, which can prevent potential runtime errors.

    		9
    Error handling
    Add error handling when closing connections

    Consider adding error handling when closing the BiDi connection to prevent potential
    exceptions from interrupting the quit process.

    javascript/node/selenium-webdriver/lib/webdriver.js [797-799]

     if (this._bidiConnection !== undefined) {
-  this._bidiConnection.close()
+  try {
+    this._bidiConnection.close()
+  } catch (error) {
+    console.warn('Error closing BiDi connection:', error)
+  }
 }
    Suggestion importance[1-10]: 8

    Why: Adding error handling when closing the BiDi connection is a good practice to prevent exceptions from interrupting the quit process, enhancing the stability of the application.

    		8
    Add a check for the existence of required capabilities

    Consider adding a check for the existence of the 'webSocketUrl' capability before
    accessing it to prevent potential errors if the capability is not present.

    javascript/node/selenium-webdriver/lib/webdriver.js [1297-1299]

     const caps = await this.getCapabilities()
 let WebSocketUrl = caps['map_'].get('webSocketUrl')
+if (!WebSocketUrl) {
+  throw new Error('WebSocket URL not found in capabilities')
+}
 this._bidiConnection = new BIDI(WebSocketUrl.replace('localhost', '127.0.0.1'))
    Suggestion importance[1-10]: 8

    Why: Adding a check for the existence of the 'webSocketUrl' capability prevents potential errors if the capability is not present, which is crucial for error handling and application stability.

    		8
    Enhancement
    Use a more robust method for URL manipulation

    Consider using a more robust method to replace 'localhost' with '127.0.0.1', such as
    using a URL object, to handle potential edge cases in the WebSocket URL.

    javascript/node/selenium-webdriver/lib/webdriver.js [1249]

    -this._cdpWsConnection = new WebSocket(this._wsUrl.replace('localhost', '127.0.0.1'))
+const url = new URL(this._wsUrl);
+url.hostname = url.hostname === 'localhost' ? '127.0.0.1' : url.hostname;
+this._cdpWsConnection = new WebSocket(url.toString());
    Suggestion importance[1-10]: 7

    Why: Using a URL object for hostname replacement is a more robust method that can handle potential edge cases, improving the reliability of the code.

    		7
    Suggestions up to commit a37a727
    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Use optional chaining for safer BiDi connection closure

    Consider using optional chaining (?.) when closing the BiDi connection to handle
    cases where this._bidi might be undefined, preventing potential errors.

    javascript/node/selenium-webdriver/lib/webdriver.js [796-798]

    -if (this._bidi !== undefined) {
-  this._bidi.close()
-}
+this._bidi?.close()
    Suggestion importance[1-10]: 9

    Why: The use of optional chaining (?.) is a safer and more concise way to handle potential undefined values, preventing runtime errors. This suggestion correctly addresses a potential issue in the code.

    		9
    Best practice
    Improve variable naming convention for better code readability

    Consider using a more descriptive variable name instead of WebSocketUrl to improve
    code readability and follow JavaScript naming conventions for local variables.

    javascript/node/selenium-webdriver/lib/webdriver.js [1297]

    -let WebSocketUrl = caps['map_'].get('webSocketUrl')
+let webSocketUrl = caps['map_'].get('webSocketUrl')
    Suggestion importance[1-10]: 7

    Why: Changing WebSocketUrl to webSocketUrl aligns with JavaScript naming conventions for local variables, improving code readability. While beneficial, this is a minor improvement.

    		7

    @pujagani pujagani marked this pull request as ready for review September 17, 2024 11:08
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Sep 17, 2024

    Persistent review updated to latest commit 878dbab

    @pujagani pujagani marked this pull request as draft September 17, 2024 11:12
    @pujagani pujagani marked this pull request as ready for review September 23, 2024 08:04
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Sep 23, 2024

    Persistent review updated to latest commit 243c068

    @pujagani pujagani merged commit 27ad7f4 into SeleniumHQ:trunk Oct 4, 2024
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @pujagani @M1troll
    [js] Close BiDi websocket connection (SeleniumHQ#14507)
    1997083
    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 3

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @pujagani