Skip to content

[py] improve socket resource management with proper shutdown sequence#15453

Merged
cgoldberg merged 2 commits intoSeleniumHQ:trunkfrom
allrob23:patch-socket
Mar 20, 2025
Merged

[py] improve socket resource management with proper shutdown sequence#15453
cgoldberg merged 2 commits intoSeleniumHQ:trunkfrom
allrob23:patch-socket

Conversation

@allrob23
Copy link
Copy Markdown
Contributor

@allrob23 allrob23 commented Mar 19, 2025 •
edited
Loading

User description

Description

This PR adds a resource management improvement to socket handling by implementing the recommended shutdown-then-close pattern in the error handling path of socket connections.

Benefits

  1. Complete connection termination: shutdown() properly initiates the TCP connection termination sequence at the socket level before resources are released
  2. Cleaner resource management: Prevents potential resource leaks in edge cases where connections aren't properly closed
  3. More reliable networking: Ensures all pending data is properly handled before closing
  4. Follows Python documentation recommendations: Implements best practices directly from Python's socket documentation

Technical details

The Python documentation explicitly recommends this approach:

"close() releases the resource associated with a connection but does not necessarily close the connection immediately. If you want to close the connection in a timely fashion, call shutdown() before close()." Python socket documentation

This change adds properly exception-handled shutdown() calls before close() to ensure sockets are terminated gracefully even in error conditions.

Impact

This is a low-risk improvement that brings the codebase more in line with networking best practices. The change is minimal but meaningful for overall reliability.

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.
  • All new and existing tests passed.

PR Type

Bug fix

Description

  • Added socket.shutdown() before close() for proper socket termination.

  • Ensured exception handling around shutdown() to prevent crashes.

  • Improved resource management and reliability in socket handling.

Changes walkthrough 📝

Relevant files
Bug fix
utils.py
Improve socket resource management in `is_connectable`     

py/selenium/webdriver/common/utils.py

  • Added socket.shutdown(socket.SHUT_RDWR) before close() in
    is_connectable.
  • Wrapped shutdown() in a try-except block to handle potential
    exceptions.
    		• +4/-0     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 19, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Exception Handling

    The bare except clause catches all exceptions without specifying which ones are expected during socket shutdown. Consider catching specific exceptions like socket.error or OSError for better error handling.

     
    except:

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 19, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Replace bare exception handling with specific exception types to avoid masking unexpected errors

    The code is catching all exceptions silently with a bare except: clause, which
    is considered a bad practice as it can hide unexpected errors. Instead, catch
    specific exceptions that might occur during socket shutdown, such as
    socket.error or OSError, and optionally log the error for debugging purposes.

    py/selenium/webdriver/common/utils.py [107-110]

     try:
     socket_.shutdown(socket.SHUT_RDWR)
-except:
+except (socket.error, OSError):
+    # Socket might already be closed or disconnected
     pass
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    		Low
    • Update

    @allrob23 allrob23 changed the title Improve socket resource management with proper shutdown sequence [py] improve socket resource management with proper shutdown sequence Mar 19, 2025
    @CLAassistant
    Copy link
    Copy Markdown

    CLAassistant commented Mar 19, 2025 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @cgoldberg cgoldberg self-requested a review March 20, 2025 16:46
    cgoldberg
    cgoldberg approved these changes Mar 20, 2025
    View reviewed changes
    Copy link
    Copy Markdown
    Member

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM 👍
    Thanks!

    @cgoldberg cgoldberg merged commit 52e6e1d into SeleniumHQ:trunk Mar 20, 2025
    2 of 4 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @allrob23 @cgoldberg @sandeepsuryaprasad
    [py] improve socket resource management with proper shutdown sequence (…
    e2df045 
    …SeleniumHQ#15453)

fix: add socket.shutdown() before close() to ensure timely closing

Co-authored-by: Corey Goldberg <1113081+cgoldberg@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @cgoldberg cgoldberg cgoldberg approved these changes

    Assignees

    No one assigned

    Labels

    Review effort 1/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @allrob23 @CLAassistant @cgoldberg