From a555aa04c06b051bc937b8dfa25113bd719f6171 Mon Sep 17 00:00:00 2001 From: ordinary-jamie <101677823+ordinary-jamie@users.noreply.github.com> Date: Mon, 11 Nov 2024 11:14:54 +0800 Subject: [PATCH 1/7] fix: Add None check for --- Lib/asyncio/base_events.py | 2 ++ .../next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst | 1 + 2 files changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 5dbe4b28d236d3..2db8a7698b0932 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -303,6 +303,8 @@ def _detach(self, transport): self._wakeup() def _wakeup(self): + if self._waiters is None: + return waiters = self._waiters self._waiters = None for waiter in waiters: diff --git a/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst b/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst new file mode 100644 index 00000000000000..7aaef55d0add17 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst @@ -0,0 +1 @@ +Add ``None`` check for :meth:`asyncio.Server._wakeup`. Patch by Jamie Phan. From 6dcee6bdd7b58f37524b7738fa7e7dabfd921657 Mon Sep 17 00:00:00 2001 From: ordinary-jamie <101677823+ordinary-jamie@users.noreply.github.com> Date: Mon, 11 Nov 2024 11:14:54 +0800 Subject: [PATCH 2/7] fix: Add None check for --- Lib/asyncio/base_events.py | 2 ++ .../next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst | 1 + 2 files changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 5dbe4b28d236d3..2db8a7698b0932 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -303,6 +303,8 @@ def _detach(self, transport): self._wakeup() def _wakeup(self): + if self._waiters is None: + return waiters = self._waiters self._waiters = None for waiter in waiters: diff --git a/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst b/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst new file mode 100644 index 00000000000000..7aaef55d0add17 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst @@ -0,0 +1 @@ +Add ``None`` check for :meth:`asyncio.Server._wakeup`. Patch by Jamie Phan. From a5de93e81a89b4183589805889c9f296d86157f0 Mon Sep 17 00:00:00 2001 From: ordinary-jamie <101677823+ordinary-jamie@users.noreply.github.com> Date: Tue, 12 Nov 2024 10:26:13 +1100 Subject: [PATCH 3/7] docs: Fix up referencing --- .../Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst b/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst index 7aaef55d0add17..13b8776fb670a4 100644 --- a/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst +++ b/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst @@ -1 +1,2 @@ -Add ``None`` check for :meth:`asyncio.Server._wakeup`. Patch by Jamie Phan. +Add ``None`` check for :class:`asyncio.base_events.Server` internal ``_wakeup`` +method. Patch by Jamie Phan. From a8d77f2c3eca6e95f7a3993e66a50adac2a93685 Mon Sep 17 00:00:00 2001 From: Jamie Phan Date: Wed, 13 Nov 2024 06:56:58 +0800 Subject: [PATCH 4/7] Update Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst Co-authored-by: Peter Bierma --- .../Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst b/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst index 13b8776fb670a4..70e981a8a5dd5f 100644 --- a/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst +++ b/Misc/NEWS.d/next/Library/2024-11-11-11-13-30.gh-issue-109564._f7W0v.rst @@ -1,2 +1 @@ -Add ``None`` check for :class:`asyncio.base_events.Server` internal ``_wakeup`` -method. Patch by Jamie Phan. +Fix race condition in :meth:`asyncio.Server.close`. Patch by Jamie Phan. From d96323752a277d48d6a173576ed62ce36bd0517b Mon Sep 17 00:00:00 2001 From: ordinary-jamie <101677823+ordinary-jamie@users.noreply.github.com> Date: Sat, 16 Nov 2024 14:55:45 +0800 Subject: [PATCH 5/7] fix: Add comments and use existing waiter ref --- Lib/asyncio/base_events.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 2db8a7698b0932..25edd6632b2965 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -303,9 +303,16 @@ def _detach(self, transport): self._wakeup() def _wakeup(self): - if self._waiters is None: - return waiters = self._waiters + if waiters is None: + # gh109564: the wakeup method has two possible call-sites, through an + # explicit call to the server's close method, or indirectly after the last + # client disconnects and the corresponding transport detaches from the + # server. These two can be in a race-condition if the server closes between + # `BaseSelectorEventLoop._accept_connection` and + # `BaseSelectorEventLoop._accept_connection2`; in this scenario we must + # check the wakeup call hasn't already set the server waiters to None. + return self._waiters = None for waiter in waiters: if not waiter.done(): @@ -949,7 +956,7 @@ async def sock_sendfile(self, sock, file, offset=0, count=None, try: return await self._sock_sendfile_native(sock, file, offset, count) - except exceptions.SendfileNotAvailableError as exc: + except exceptions.SendfileNotAvailableError: if not fallback: raise return await self._sock_sendfile_fallback(sock, file, @@ -1262,7 +1269,7 @@ async def sendfile(self, transport, file, offset=0, count=None, try: return await self._sendfile_native(transport, file, offset, count) - except exceptions.SendfileNotAvailableError as exc: + except exceptions.SendfileNotAvailableError: if not fallback: raise From 29c3232ae350772dec1b34b988fc85568089a242 Mon Sep 17 00:00:00 2001 From: ordinary-jamie <101677823+ordinary-jamie@users.noreply.github.com> Date: Sat, 16 Nov 2024 14:57:11 +0800 Subject: [PATCH 6/7] fix: revert unintended changes --- Lib/asyncio/base_events.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py index 25edd6632b2965..3312c6d734efb5 100644 --- a/Lib/asyncio/base_events.py +++ b/Lib/asyncio/base_events.py @@ -956,7 +956,7 @@ async def sock_sendfile(self, sock, file, offset=0, count=None, try: return await self._sock_sendfile_native(sock, file, offset, count) - except exceptions.SendfileNotAvailableError: + except exceptions.SendfileNotAvailableError as exc: if not fallback: raise return await self._sock_sendfile_fallback(sock, file, @@ -1269,7 +1269,7 @@ async def sendfile(self, transport, file, offset=0, count=None, try: return await self._sendfile_native(transport, file, offset, count) - except exceptions.SendfileNotAvailableError: + except exceptions.SendfileNotAvailableError as exc: if not fallback: raise From 001c28668c5e11e6c637bd4271c2a0125cc30409 Mon Sep 17 00:00:00 2001 From: ordinary-jamie <101677823+ordinary-jamie@users.noreply.github.com> Date: Mon, 18 Nov 2024 09:58:31 +0800 Subject: [PATCH 7/7] feat: Add unit-test to capture unraised exception --- Lib/asyncio/selector_events.py | 2 ++ Lib/test/test_asyncio/test_server.py | 46 ++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/Lib/asyncio/selector_events.py b/Lib/asyncio/selector_events.py index f94bf10b4225e7..4ec3311fbeb6dd 100644 --- a/Lib/asyncio/selector_events.py +++ b/Lib/asyncio/selector_events.py @@ -243,8 +243,10 @@ async def _accept_connection2( # It's now up to the protocol to handle the connection. except (SystemExit, KeyboardInterrupt): + conn.close() raise except BaseException as exc: + conn.close() if self._debug: context = { 'message': diff --git a/Lib/test/test_asyncio/test_server.py b/Lib/test/test_asyncio/test_server.py index 60a40cc8349fed..35962bf9eb7d73 100644 --- a/Lib/test/test_asyncio/test_server.py +++ b/Lib/test/test_asyncio/test_server.py @@ -1,11 +1,14 @@ import asyncio +import gc import os import socket import time import threading import unittest +from test import support from test.support import socket_helper +from test.support import warnings_helper from test.test_asyncio import utils as test_utils from test.test_asyncio import functional as func_tests @@ -266,6 +269,49 @@ async def serve(rd, wr): await asyncio.sleep(0) self.assertTrue(task.done()) + async def test_close_race(self): + + srv = await asyncio.start_server(lambda *_: None, socket_helper.HOSTv4, 0) + srv_sock = srv.sockets[0] + addr = srv_sock.getsockname() + + # When the server is closed before a connection is handled but after the + # connection is accepted, then a race-condition exists between the handler + # transport and the server, both which will attempt to wakeup the server to set + # any server waiters. We can recreate race-condition by opening a connection and + # waiting for the server reader callback before closing the server + loop = asyncio.get_running_loop() + srv_reader, _ = loop._selector.get_key(srv_sock.fileno()).data + conn_task = asyncio.create_task(asyncio.open_connection(addr[0], addr[1])) + for _ in range(10): + await asyncio.sleep(0) + if srv_reader in loop._ready: + break + + # Ensure accepted connection task is scheduled by the server reader, but not + # completed, before closing the server. + await asyncio.sleep(0) + srv.close() + + # Complete the client connection to close the socket. Suppress errors in the + # handler transport due to failing to attach to closed server. + with support.captured_stderr(): + try: + _, wr = await conn_task + self.addCleanup(wr.close) + except OSError: + pass + + await srv.wait_closed() + + # Verify the handler transport does not raise an error due to multiple calls to + # the server wakeup. Suppress expected ResourceWarnings from the handler + # transport failing to attach to the closed server. + with warnings_helper.check_warnings(("unclosed transport", ResourceWarning)), \ + support.catch_unraisable_exception() as cm: + support.gc_collect() + self.assertIsNone(cm.unraisable) + # Test the various corner cases of Unix server socket removal class UnixServerCleanupTests(unittest.IsolatedAsyncioTestCase):