Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for unexpected socket closures and data leakage under heavy load #646

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix for unexpected socket closures and data leakage under heavy load #646

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions uvloop/loop.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1877,6 +1877,7 @@ cdef class Loop:
AddrInfo ai_local = None
AddrInfo ai_remote
TCPTransport tr
int sockfd

system.addrinfo *rai = NULL
system.addrinfo *lai = NULL
Expand Down Expand Up @@ -2060,8 +2061,10 @@ cdef class Loop:
waiter = self._new_future()
tr = TCPTransport.new(self, protocol, None, waiter, context)
try:
# Take ownership of the file descriptor
sockfd = sock.detach()
# libuv will make socket non-blocking
tr._open(sock.fileno())
tr._open(sockfd)
Copy link
Member

Choose a reason for hiding this comment

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

The approach looks correct -- but I'm wondering how vanilla asyncio handles the same thing?

Copy link
Author

Choose a reason for hiding this comment

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

I think vanilla asyncio has an easier problem in that it can just have python sockets "all the way down", so just let reference counting take care of cleanup, while here we need to manage the disconnect with libuv dealing in file descriptors. I am suspecting there is some error handling path where a file descriptor is closed while the python socket object remains alive and not detached, so when it is finally closed, it messes up any new socket that happens to have the same file descriptor.
e.g. create socket s, call a loop method passing in an explicit socket, <bad error path which will end with sock.close()> overlapping with an .accept. I think the .accept never results in a python socket object being created.

So with the methods accepting sockets and other methods that internally work directly in file descriptors can there be a discrepancy?

tr._init_protocol()
await waiter
except (KeyboardInterrupt, SystemExit):
Expand All @@ -2075,8 +2078,6 @@ cdef class Loop:
tr._close()
raise

tr._attach_fileobj(sock)

if ssl:
app_transport = protocol._get_app_transport(context)
try:
Expand Down
Loading