gh-130902 Add optional socket shutdown to close method of HTTPConnection class in http/client.py

[sharktide]

Similar to gh-130862 but for http library

This pr will close #130902

Overview:
Based on #130902, I added an optional socket shutdown to HTTPConnection.close. The modified function, HTTPConnection.close has a new optional shutdown parameter defaulted to false.

Original function:

  def close(self):
      """Close the connection to the HTTP server."""
      self.__state = _CS_IDLE
      try:
          sock = self.sock
          if sock:
              self.sock = None
              sock.close()   # close it manually... there may be other refs
      finally:
          response = self.__response
          if response:
              self.__response = None
              response.close()

Refactored function:

def close(self, shutdown=False):
        self.__state = _CS_IDLE
        try:
            sock = self.sock
            if sock:
                self.sock = None
                if shutdown:
                    try:
                        sock.shutdown(socket.SHUT_RDWR)
                    except OSError as e:
                        print(f"Socket shutdown error: {e}", file=sys.stderr)
                    except Exception as e:
                        print(f"Unexpected error during socket shutdown: {e}", file=sys.stderr)
                try:
                    sock.close()
                except OSError as e:
                    print(f"Socket close error: {e}", file=sys.stderr)
                except Exception as e:
                    print(f"Unexpected error during socket close: {e}", file=sys.stderr)
        finally:
            response = self.__response
            if response:
                self.__response = None
                try:
                    response.close()
                except OSError as e:
                    print(f"Response close error: {e}", file=sys.stderr)
                except Exception as e:
                    print(f"Unexpected error during response close: {e}", file=sys.stderr)

This new change has some benefits such as:

• Ensures that all pending data is properly discarded before closing, particularly in scenarios where data might still be buffered.
• Prevents potential issues with lingering resources in some edge cases.
• Aligns with best practices for socket cleanup.
• Can help avoid RST (reset by peer) issues in some network conditions.

View #130902 for more information

I tested it against a custom test script:

import sys
from unittest.mock import MagicMock
from http.client import HTTPConnection  # Replace with the actual module name
from io import StringIO

def test_close_with_shutdown():
    # Create a mock socket object
    mock_socket = MagicMock()
    mock_socket.close = MagicMock()
    mock_socket.shutdown = MagicMock()

    # Instantiate the HTTPConnection object
    connection = HTTPConnection('www.example.com')
    connection.sock = mock_socket

    # Redirect stderr to capture error messages
    original_stderr = sys.stderr
    sys.stderr = StringIO()

    try:
        # Call the close method with shutdown=True
        connection.close(shutdown=True)

        # Check that shutdown and close were called
        mock_socket.shutdown.assert_called_once_with(2)  # SHUT_RDWR
        mock_socket.close.assert_called_once()

        # Check that no error messages were written to stderr
        error_output = sys.stderr.getvalue()
        assert not error_output, f"Error messages were written to stderr: {error_output}"

    finally:
        # Restore original stderr
        sys.stderr = original_stderr

def test_close_without_shutdown():
    # Create a mock socket object
    mock_socket = MagicMock()
    mock_socket.close = MagicMock()
    mock_socket.shutdown = MagicMock()

    # Instantiate the HTTPConnection object
    connection = HTTPConnection('www.example.com')
    connection.sock = mock_socket

    # Redirect stderr to capture error messages
    original_stderr = sys.stderr
    sys.stderr = StringIO()

    try:
        # Call the close method with shutdown=False
        connection.close(shutdown=False)

        # Check that shutdown was not called, but close was
        mock_socket.shutdown.assert_not_called()
        mock_socket.close.assert_called_once()

        # Check that no error messages were written to stderr
        error_output = sys.stderr.getvalue()
        assert not error_output, f"Error messages were written to stderr: {error_output}"

    finally:
        # Restore original stderr
        sys.stderr = original_stderr

def test_close_shutdown_error():
    # Create a mock socket object that raises an OSError on shutdown
    mock_socket = MagicMock()
    mock_socket.close = MagicMock()
    mock_socket.shutdown = MagicMock(side_effect=OSError("Shutdown error"))

    # Instantiate the HTTPConnection object
    connection = HTTPConnection('www.example.com')
    connection.sock = mock_socket

    # Redirect stderr to capture error messages
    original_stderr = sys.stderr
    sys.stderr = StringIO()

    try:
        # Call the close method with shutdown=True
        connection.close(shutdown=True)

        # Check that shutdown and close were called
        mock_socket.shutdown.assert_called_once_with(2)  # SHUT_RDWR
        mock_socket.close.assert_called_once()

        # Check that the error message was written to stderr
        error_output = sys.stderr.getvalue()
        assert "Socket shutdown error: Shutdown error" in error_output, f"Expected error message not found: {error_output}"

    finally:
        # Restore original stderr
        sys.stderr = original_stderr

def test_close_unexpected_error():
    # Create a mock socket object that raises an unexpected exception on shutdown
    mock_socket = MagicMock()
    mock_socket.close = MagicMock()
    mock_socket.shutdown = MagicMock(side_effect=Exception("Unexpected error"))

    # Instantiate the HTTPConnection object
    connection = HTTPConnection('www.example.com')
    connection.sock = mock_socket

    # Redirect stderr to capture error messages
    original_stderr = sys.stderr
    sys.stderr = StringIO()

    try:
        # Call the close method with shutdown=True
        connection.close(shutdown=True)

        # Check that shutdown and close were called
        mock_socket.shutdown.assert_called_once_with(2)  # SHUT_RDWR
        mock_socket.close.assert_called_once()

        # Check that the error message was written to stderr
        error_output = sys.stderr.getvalue()
        assert "Unexpected error during socket shutdown: Unexpected error" in error_output, f"Expected error message not found: {error_output}"

    finally:
        # Restore original stderr
        sys.stderr = original_stderr

if __name__ == '__main__':
    test_close_with_shutdown()
    test_close_without_shutdown()
    test_close_shutdown_error()
    test_close_unexpected_error()
    print("All tests passed.")

And it passed successfully:

PS C:\Users\---\Documents\GitHub\cpython> .\PCbuild\amd64\python_d.exe test.py
All tests passed.
PS C:\Users\---\Documents\GitHub\cpython> 

• Issue: http.client Possible Improvement: Using shutdown() Before close() in HTTPConnection.close() #130902

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[Mr-Sunglasses]

Some small triage corrections.

[Mr-Sunglasses]

Tests should be added for this.

[sharktide]

Tests should be added for this.

Adding to test_httplib.py as a TestCase

[sharktide]

@Mr-Sunglasses The change you requested to the news entry that specified the close method of the httpconnection class resulted in an error because it could not find ":func:HTTPConnection.close" and ":class:HTTPConnection", causing an error in the docs build process. I am removing it and surrounding it in regular code tags in the news entry, if it okay.

/home/runner/work/cpython/cpython/Doc/build/NEWS:57: WARNING: py:func reference target not found: HTTPConnection.close [ref.func]
/home/runner/work/cpython/cpython/Doc/build/NEWS:57: WARNING: py:class reference target not found: HTTPConnection [ref.class]
Error: Process completed with exit code 255.

[sharktide]

@Mr-Sunglasses I also finished adding the tests to test_httplib.py

[sharktide]

@Mr-Sunglasses done used your new fix ready for review (again)

[Mr-Sunglasses]

• Please update the documentation of socket.close() and mention the use of optional shutdown

TiA.

[Mr-Sunglasses]

I think we should add it in the Doc/library/socket.rst also, about the shutdown.

Ref.

cpython/Doc/library/socket.rst

Line 1488 in b1b4f96

.. method:: socket.close()

[sharktide]

ok, shouldn't take too long

[sharktide]

@Mr-Sunglasses Finally ready for review/ Added/updated docs for the close method of HTTPConnection class as well as added back a modifed docstring in http/client.py!

[Mr-Sunglasses]

Just some small changes in calling of terms.

[sharktide]

@Mr-Sunglasses applied your small changes for like the billionth time ready for review again :)

[Mr-Sunglasses]

@Mr-Sunglasses applied your small changes for like the billionth time ready for review again :)

Sorry for being nitty, Thanks a lot 🥇

[Mr-Sunglasses]

LGTM, thanks @sharktide

[sharktide]

@Mr-Sunglasses applied your small changes for like the billionth time ready for review again :)

Sorry for being nitty, Thanks a lot 🥇

nah don't worry. Just been having a rough day

[sharktide]

I had accidentally clicked the close with comment button, just ignore it anyone who sees this pr. Still needs core review

[sharktide]

@picnixz Could you please give a core review for this PR? No one else has done so yet.

[picnixz]

This is incorrect. All exceptions are now swallowed but they should be forwarded so that the caller can handle them how they see fit.

There is no practical need to shutdown the socket. The HTTP server may want to keep the connection alive, possibly to make some cleanup on their side. It's just that we don't need it anymore on the client side.

Also, there are other places where .close() is being called but then without a specific shutdown (e.g., _tunnel()). Sorry but I'm rejecting this. Also, it's not socket.close() that is being changed but http.client.close() which is entirely different.

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[picnixz]

Closing per #130902 (comment).