`http.client` Possible Improvement: Using shutdown() Before close() in `HTTPConnection.close()`

[allrob23]

Description

While reviewing the close method in (

cpython/Lib/http/client.py

Lines 1015 to 1028 in 5e73ece

	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()

I noticed that the socket is directly closed using close() (line 1022), without first calling shutdown().

This is similar to what I previously reported in issue #130850, where _close_self_pipe in asyncio.selector_events also closes sockets without calling shutdown() first.

Code Reference

Currently, the code is as follows:

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()

Question

Would there be any potential downsides or benefits to adding a shutdown(socket.SHUT_RDWR) call before close() in this case?

Possible Benefits

• 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.

Possible Change

def close(self):
    """Close the connection to the HTTP server."""
    self.__state = _CS_IDLE
    try:
        sock = self.sock
        if sock:
            self.sock = None
            try:
                sock.shutdown(socket.SHUT_RDWR)  # Gracefully terminate the connection
            except OSError:
                pass  # Ignore errors if the socket is already closed
            sock.close()   # Close it manually... there may be other refs
    finally:
        response = self.__response
        if response:
            self.__response = None
            response.close()

Reference

The Python socket documentation states:

"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

Since a similar discussion is happening in #130850, I wanted to bring attention to this case in http.client as well.

Looking forward to your thoughts!

Linked PRs

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

[sharktide]

Potential Downsides:

• Increased Complexity: Adding shutdown() introduces a small amount of additional complexity to the code.
• Error Handling: If shutdown() raises an OSError, it needs to be handled properly to avoid masking other issues.

Benefits:

• Proper Resource Cleanup: Ensures that all pending data is properly discarded before closing, particularly in scenarios where data might still be buffered.

• Best Practices: Aligns with best practices for socket cleanup, as recommended by the Python socket documentation.

• Preventing Issues: Helps avoid potential issues with lingering resources and RST (reset by peer) errors in some network conditions.

It seems sound. I'll make a pr when I can similar to #130862

[sharktide]

To be honest the downsides I mentioned aren't really a problem. I'll make a pr fix either tmrw, today, or dayafter

[sharktide]

I'll write tests then open a pr

[picnixz]

Ensures that all pending data is properly discarded before closing, particularly in scenarios where data might still be buffered.

Data will be properly discarded once garbage-collected. The docs say:

releases the resource associated with a connection but does not necessarily close the connection immediately.

It's just that we're freeing up resources a bit after, not when calling it.

Prevents potential issues with lingering resources in some edge cases.

What kind of issues? please present a concrete scenario.

Can help avoid RST (reset by peer) issues in some network conditions.

What are the issues? and what are the conditions? AFAIK, the peer will be disconnected upon closing the connection so it's an expected error that the server should handle.

See also PR rejection notice: #130919 (review).

---

That being said, what could be better is to add a .shutdown() method that solely shutdowns the underlying socket. However, the rest of the library does not do this, e.g., ftplib, rpc or multiprocessing. So I prefer not adding this functionality in close() itself and only add shutdown() if there are real needs (you can just do conn.sock.shutdown(socket.SHUT_RDWR); conn.close() by the way which is exactly what you want)