asyncio possible Improvement: Using shutdown() Before close() in _close_self_pipe

[allrob23]

Description

While reviewing the _close_self_pipe method in Lib/asyncio/selector_events.py, I noticed that the sockets _ssock and _csock are directly closed using close(), without first calling shutdown().

Code Reference

    def _close_self_pipe(self):
        self._remove_reader(self._ssock.fileno())
        self._ssock.close()
        self._ssock = None
        self._csock.close()
        self._csock = None
        self._internal_fds -= 1

Question

Would there be any potential downsides or benefits to adding a shutdown(socket.SHUT_RDWR) call before closing these sockets?

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.

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()." link

Looking forward to your thoughts!

Thanks!

Linked PRs

• gh-130850 Refactor _close_self_pipe for conditional socket shutdown in asyncio/selector_events.py #130862

[sharktide]

I am not a member of python in any way, but here is my input:

Your Proposed Change: Adding a shutdown(socket.SHUT_RDWR) call before close() for both _ssock and _csock.

Benefits:

• Data Discard: Ensures that all pending data is properly discarded before closing as you said

• Resource Cleanup: Prevents potential issues with lingering resources, especially in edge cases as you also said

• Best Practices: Aligns with the best practices for socket cleanup, as mentioned in the Python socket documentation as you also
  said

Downsides:

Extra Function Call: Introduces an additional function call (shutdown()) which might be unnecessary in some scenarios where close() is sufficient.

Error Handling: Requires handling potential exceptions from the shutdown() call.

[sharktide]

I could make a pr

[sharktide]

yeah I'll make a pr on this branch

[sharktide]

done writing unittests

[sharktide]

I'll post a pr when I fix all the issues

[allrob23]

Hello @sharktide,

Thank you for reviewing and submitting a PR for issue this Issue! I really appreciate your time and effort.

I came across a similar case in the http library and have opened a new issue: #130902.
When you have a moment, could you also take a look at this one? Your insights would be greatly appreciated!

Thanks again!

[sharktide]

I'll check it out!

[kumaraditya303]

The self pipe is only ever written by the wakeup fd as such there is no need to shutdown it, it can be closed safely.