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.

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

gh-93821: Handle connection resets on Windows #124779

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
4 changes: 4 additions & 0 deletions Lib/asyncio/proactor_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,10 @@ def loop(f=None):
if self.is_closed():
return
f = self._proactor.accept(sock)
except exceptions.CancelledError:
# Effectively ignore connections that throw a cancelled error
# during setup, loop back around and continue serving.
sock.close()
Copy link
Member

Choose a reason for hiding this comment

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

Suppressing CancelledError is a huge red flag and should only be done in desperate times. Can you write an elaborate comment what's going on here? I don't understand why CancelledError needs suppressing neither from this comment nor from the PR description.

except OSError as exc:
if sock.fileno() != -1:
self.call_exception_handler({
Expand Down
31 changes: 23 additions & 8 deletions Lib/asyncio/windows_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,24 @@ async def _make_subprocess_transport(self, protocol, args, shell,
return transp


def overlapped_connection_reset_error_handler(func):
"""
Rethrow common connection errors that come from clients
disconnecting unexpectedly. This is a common error that
can be safely ignored in most cases.
"""
def wrapper(*args, **kwargs):
try:
return func(*args, **kwargs)
except OSError as exc:
if exc.winerror in (_overlapped.ERROR_NETNAME_DELETED,
_overlapped.ERROR_OPERATION_ABORTED):
raise ConnectionResetError(*exc.args)
else:
raise
return wrapper


class IocpProactor:
"""Proactor implementation using IOCP."""

Expand Down Expand Up @@ -458,15 +476,9 @@ def _result(self, value):
return fut

@staticmethod
@overlapped_connection_reset_error_handler
def finish_socket_func(trans, key, ov):
try:
return ov.getresult()
except OSError as exc:
if exc.winerror in (_overlapped.ERROR_NETNAME_DELETED,
_overlapped.ERROR_OPERATION_ABORTED):
raise ConnectionResetError(*exc.args)
else:
raise
return ov.getresult()

@classmethod
def _finish_recvfrom(cls, trans, key, ov, *, empty_result):
Expand Down Expand Up @@ -552,6 +564,7 @@ def accept(self, listener):
ov = _overlapped.Overlapped(NULL)
ov.AcceptEx(listener.fileno(), conn.fileno())

@overlapped_connection_reset_error_handler
def finish_accept(trans, key, ov):
ov.getresult()
# Use SO_UPDATE_ACCEPT_CONTEXT so getsockname() etc work.
Expand Down Expand Up @@ -596,6 +609,7 @@ def connect(self, conn, address):
ov = _overlapped.Overlapped(NULL)
ov.ConnectEx(conn.fileno(), address)

@overlapped_connection_reset_error_handler
def finish_connect(trans, key, ov):
ov.getresult()
# Use SO_UPDATE_CONNECT_CONTEXT so getsockname() etc work.
Expand Down Expand Up @@ -628,6 +642,7 @@ def accept_pipe(self, pipe):
# completion of the connection.
return self._result(pipe)

@overlapped_connection_reset_error_handler
def finish_accept_pipe(trans, key, ov):
ov.getresult()
return pipe
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix error handling in windows events where clients terminating connections could result in an asyncio server using Proactor event loops to hang indefinitely.
Copy link
Contributor

Choose a reason for hiding this comment

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

About NEWS format please to reading this: https://devguide.python.org/documentation/markup/
for example:

:mod:`asyncio`

Copy link
Author

Choose a reason for hiding this comment

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

Is there something I need to change, or is the comment informational?

Copy link
Contributor

@rruuaanng rruuaanng Sep 30, 2024

Choose a reason for hiding this comment

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

Oh, sorry. I meant that your NEWS format seems incorrect. Maybe you should check out the NEWS written by others in the Misc section, that might be better.

Edit: For the module, you should use the example I provided.

Copy link
Member

Choose a reason for hiding this comment

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

think this is what they meant:

Suggested change
Fix error handling in windows events where clients terminating connections could result in an asyncio server using Proactor event loops to hang indefinitely.
Fix error handling in windows events where clients terminating connections could result in an :mod:`asyncio` server using Proactor event loops to hang indefinitely.

Copy link
Author

Choose a reason for hiding this comment

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

Got it! Thanks for clarification.

Loading