GH-109564: add asyncio.Server state machine #131009

ordinary-jamie · 2025-03-09T12:24:55Z

Supersedes #126660 and implements a state machine as suggested by this comment: #126660 (review)

Analysis

Addressed issues

This addresses two issues raised in GH-109564:

A transport will first check if a server is closed before attempting to attach. This will prevent raising an AssertionError (changed to a RuntimeError here)
When a connection is accepted by BaseSelectorEventLoop._accept_connection but is not handled in BaseSelectorEventLoop._accept_connection2, then it will fail to correctly construct and raise an "unclosed transport" ResourceWarning on __del__; this will raise an error as it will call Server._detach -> Server._wakeup after close and attempt to access the Server._waiters=None attribute.

These errors can be seen in the new test test_close_before_transport_attach against main:

0:00:00 load avg: 2.05 [1/1] test_asyncio.test_server
Error on transport creation for incoming connection
handle_traceback: Handle created at (most recent call last):
  File "cpython/Lib/unittest/async_case.py", line 120, in _callMaybeAsync
    return self._asyncioRunner.run(
  File "cpython/Lib/asyncio/runners.py", line 127, in run
    return self._loop.run_until_complete(task)
  File "cpython/Lib/asyncio/base_events.py", line 706, in run_until_complete
    self.run_forever()
  File "cpython/Lib/asyncio/base_events.py", line 677, in run_forever
    self._run_once()
  File "cpython/Lib/asyncio/base_events.py", line 2029, in _run_once
    handle._run()
  File "cpython/Lib/asyncio/events.py", line 98, in _run
    self._context.run(self._callback, *self._args)
  File "cpython/Lib/asyncio/selector_events.py", line 215, in _accept_connection
    self.create_task(accept)
  File "cpython/Lib/asyncio/base_events.py", line 470, in create_task
    task = tasks.Task(coro, loop=self, **kwargs)
protocol: <Mock id='4354243424'>
Traceback (most recent call last):
  File "cpython/Lib/asyncio/selector_events.py", line 234, in _accept_connection2
    transport = self._make_socket_transport(
        conn, protocol, waiter=waiter, extra=extra,
        server=server)
  File "cpython/Lib/asyncio/selector_events.py", line 72, in _make_socket_transport
    return _SelectorSocketTransport(self, sock, protocol, waiter,
                                    extra, server)
  File "cpython/Lib/asyncio/selector_events.py", line 941, in __init__
    super().__init__(loop, sock, protocol, extra, server)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "cpython/Lib/asyncio/selector_events.py", line 799, in __init__
    self._server._attach(self)
    ~~~~~~~~~~~~~~~~~~~~^^^^^^
  File "cpython/Lib/asyncio/base_events.py", line 297, in _attach
    assert self._sockets is not None
           ^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
Warning -- Unraisable exception
Exception ignored while calling deallocator <function _SelectorTransport.__del__ at 0x1037d1900>:
Traceback (most recent call last):
  File "cpython/Lib/asyncio/selector_events.py", line 881, in __del__
    self._server._detach(self)
  File "cpython/Lib/asyncio/base_events.py", line 303, in _detach
    self._wakeup()
  File "cpython/Lib/asyncio/base_events.py", line 308, in _wakeup
    for waiter in waiters:
TypeError: 'NoneType' object is not iterable
test test_asyncio.test_server failed -- Traceback (most recent call last):
  File "cpython/Lib/asyncio/runners.py", line 127, in run
    return self._loop.run_until_complete(task)
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^
  File "cpython/Lib/asyncio/base_events.py", line 719, in run_until_complete
    return future.result()
           ~~~~~~~~~~~~~^^
  File "cpython/Lib/test/test_asyncio/test_server.py", line 293, in test_close_before_transport_attach
    proto.connection_lost.assert_called()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "cpython/Lib/unittest/mock.py", line 949, in assert_called
    raise AssertionError(msg)
AssertionError: Expected 'connection_lost' to have been called.

0:00:00 load avg: 2.05 [1/1/1] test_asyncio.test_server failed (1 failure)

ordinary-jamie · 2025-03-09T12:27:24Z

Hi @1st1 and @ZeroIntensity !

Very sorry for the major delay on this! I had to go on leave for personal reasons.

I've created a new PR for #126660 which implements a state machine as @1st1 suggested.

This change is a bit bigger than what I'm used to (and qualified for 😅 ) so please let me know if there needs to be any changes. There was some areas I wasn't quite sure about, which I will mark up with comments

ordinary-jamie · 2025-03-09T12:28:41Z

Lib/asyncio/base_events.py

+    INITIALIZED = "initialized"
+    SERVING = "serving"
+    CLOSED = "closed"
+    SHUTDOWN = "shutdown"


Not sure about the name SHUTDOWN here, but needed something more "definitive" than closed.

If we do use SHUTDOWN should I also rename Server._wakeup -> Server._shutdown?

If we do use SHUTDOWN should I also rename Server._wakeup -> Server._shutdown?

I think that makes sense.

I'm more worried about INITIALIZED here; nothing is really initialized, other than the Python object, which isn't particularly useful knowledge. Let's call it something like NOT_SERVING or NOT_YET_STARTED.

Addressed in 07129e5

ordinary-jamie · 2025-03-09T12:30:05Z

Lib/asyncio/base_events.py

            self._wakeup()

    def _wakeup(self):
+        match self._state:


Not sure if PEP 636 match-case statements are permitted; can change to if-else statement if needed.

They're permitted, but it's not particularly useful to use match here (no pattern is being matched).

let's stick to if else as i am not so used to match case.

Addressed in 8e409b7

ordinary-jamie · 2025-03-09T12:33:34Z

Lib/asyncio/selector_events.py

        if self._server is not None:
-            self._server._attach(self)
+            if self._server.is_serving():
+                self._server._attach(self)
+            else:
+                self.abort()
+                return


This is a functional change over main.. In my head, if a transport was constructed with server != None, then the transport should not be created/serviced if its corresponding server was shutdown.

Used transport.abort over transport.close to force clear the buffer, since no server exists to service that.

A return statement was added to the else block to prevent the transport getting tracked in loop._transports

ZeroIntensity

This is looking a lot better! I'll do a thorough review sometime soon :)

ZeroIntensity · 2025-03-10T11:05:40Z

Lib/asyncio/base_events.py

            self._wakeup()

    def _wakeup(self):
+        match self._state:


They're permitted, but it's not particularly useful to use match here (no pattern is being matched).

kumaraditya303 · 2025-03-10T12:35:14Z

I'll try to review by this week.

ZeroIntensity

Sorry for the delay!

Lib/asyncio/base_events.py

ZeroIntensity · 2025-03-25T12:53:10Z

Lib/asyncio/base_events.py

+        if self._state == _ServerState.CLOSED or self._state == _ServerState.SHUTDOWN:
+            return
+        else:
+            self._state = _ServerState.CLOSED


What happens if something magically goes wrong after this has been set? Is the server still alive while thinking it's closed? It might be worth adding a try/except to undo this.

Ah great point! Although I'm not sure how that would look from a user's point of view. Should we give them a warning to let them know they must try to close the server again? Alternatively, should we expose something like a force kwarg/flag?

I pushed an initial fix with a simple try/except to recover the previous state on fail in 48a3c0d

ZeroIntensity · 2025-03-25T12:56:00Z

Lib/asyncio/base_events.py

+    INITIALIZED = "initialized"
+    SERVING = "serving"
+    CLOSED = "closed"
+    SHUTDOWN = "shutdown"


If we do use SHUTDOWN should I also rename Server._wakeup -> Server._shutdown?

I think that makes sense.

I'm more worried about INITIALIZED here; nothing is really initialized, other than the Python object, which isn't particularly useful knowledge. Let's call it something like NOT_SERVING or NOT_YET_STARTED.

ZeroIntensity · 2025-03-25T12:59:57Z

Lib/asyncio/base_events.py

+            # Server._detach() by the last connected client.
+            return
+        else:
+            raise RuntimeError(f"server {self!r} can only wakeup waiters after closing")


This error message isn't making much sense to me. We'll never reach here "after closing," right? This can only be triggered when the state is SERVING or INITIALIZED.

Sorry this is bad wording, I was trying to say that the Server must be closed before it can be shutdown (i.e. we cannot shutdown from the SERVING state, nor the INITIALIZED/NOT_STARTED state)

Made a fix in: 7f3481b

Lib/asyncio/base_events.py

ZeroIntensity · 2025-03-25T13:05:58Z

Lib/asyncio/proactor_events.py

+            if self._server.is_serving():
+                self._server._attach(self)
+            else:
+                self.abort()


Why abort the whole transport? This is seemingly a change in behavior.

Ah yup sorry, my misunderstanding! Addressed in 5832655

I commented here my original thinking:

This is a functional change over main.. In my head, if a transport was constructed with server != None, then the transport should not be created/serviced if its corresponding server was shutdown.

But I re-read the docs for Server.close and see I was thinking about it incorrectly:

https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.Server.close

The sockets that represent existing incoming client connections are left open.

Co-authored-by: Peter Bierma <[email protected]>

ordinary-jamie · 2025-03-30T13:43:09Z

Sorry for the delay!

No worries; I'm really grateful for the review!

Sorry for my delay on responding too, had a busy week and just got around to this 🙏

I've tried to address all your comments in each of their threads and linked the respective commits. Please let me know if there's anything else that needs addressing/fixing.

ZeroIntensity

I'm really happy to report that I can confirm this fixes the original reproducer reported in #109564, without ResourceWarning spam.

Thanks, @ordinary-jamie!

feat: add asyncio server state machine

0fec860

ordinary-jamie requested review from 1st1, asvetlov, kumaraditya303 and willingc as code owners March 9, 2025 12:24

bedevere-app bot added the awaiting review label Mar 9, 2025

bedevere-app bot mentioned this pull request Mar 9, 2025

Race condition in asyncio.Server.close #109564

Open

ordinary-jamie commented Mar 9, 2025

View reviewed changes

fix: fix proactor transport for windows

f3b96bf

ZeroIntensity added the topic-asyncio label Mar 10, 2025

ZeroIntensity reviewed Mar 10, 2025

View reviewed changes

ZeroIntensity self-requested a review March 10, 2025 11:06

fix: change match to if statements

8e409b7

ZeroIntensity reviewed Mar 25, 2025

View reviewed changes

ordinary-jamie and others added 6 commits March 30, 2025 23:51

Update Lib/asyncio/base_events.py

a92158a

Co-authored-by: Peter Bierma <[email protected]>

Update Lib/asyncio/base_events.py

44d24fb

Co-authored-by: Peter Bierma <[email protected]>

fix: rename wakeup to shutdown and states

07129e5

fix: Add try-catch for close to recover state on fail

48a3c0d

fix: do not abort client conn transport on close

5832655

fix: rename error

7f3481b

ordinary-jamie requested a review from ZeroIntensity March 30, 2025 13:43

ZeroIntensity approved these changes Mar 30, 2025

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Mar 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-109564: add asyncio.Server state machine #131009

GH-109564: add asyncio.Server state machine #131009

ordinary-jamie commented Mar 9, 2025

ordinary-jamie commented Mar 9, 2025

ordinary-jamie Mar 9, 2025

ZeroIntensity Mar 25, 2025

ordinary-jamie Mar 30, 2025

ordinary-jamie Mar 9, 2025

ZeroIntensity Mar 10, 2025

kumaraditya303 Mar 10, 2025

ordinary-jamie Mar 30, 2025

ordinary-jamie Mar 9, 2025

ZeroIntensity left a comment

ZeroIntensity Mar 10, 2025

kumaraditya303 commented Mar 10, 2025

ZeroIntensity left a comment

ZeroIntensity Mar 25, 2025

ordinary-jamie Mar 30, 2025

ZeroIntensity Mar 25, 2025

ZeroIntensity Mar 25, 2025

ordinary-jamie Mar 30, 2025

ZeroIntensity Mar 25, 2025

ordinary-jamie Mar 30, 2025

ordinary-jamie commented Mar 30, 2025

ZeroIntensity left a comment

GH-109564: add asyncio.Server state machine #131009

Are you sure you want to change the base?

GH-109564: add asyncio.Server state machine #131009

Conversation

ordinary-jamie commented Mar 9, 2025

Analysis

Addressed issues

ordinary-jamie commented Mar 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeroIntensity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumaraditya303 commented Mar 10, 2025

ZeroIntensity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordinary-jamie commented Mar 30, 2025

ZeroIntensity left a comment

Choose a reason for hiding this comment