NotificationsYou must be signed in to change notification settings
Fork32k
Star67.2k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

ordinary-jamie wants to merge9 commits intopython:main

base:main

Choose a base branch

fromordinary-jamie:asyncio-server-state-machine

Open

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

ordinary-jamie wants to merge9 commits intopython:mainfromordinary-jamie:asyncio-server-state-machine

+73 −27

Conversation

Copy link

Contributor

ordinary-jamie commentedMar 9, 2025

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

Analysis

Addressed issues

This addresses two issues raised inGH-109564:

A transport will first check if a server is closed before attempting to attach. This will prevent raising anAssertionError (changed to aRuntimeError here)
When a connection is accepted byBaseSelectorEventLoop._accept_connection but is not handled inBaseSelectorEventLoop._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 callServer._detach ->Server._wakeup after close and attempt to access theServer._waiters=None attribute.

These errors can be seen in the new testtest_close_before_transport_attach againstmain:

0:00:00 load avg: 2.05 [1/1] test_asyncio.test_serverError on transport creation for incoming connectionhandle_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           ^^^^^^^^^^^^^^^^^^^^^^^^^AssertionErrorWarning -- Unraisable exceptionException 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 iterabletest 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)

feat: add asyncio server state machine

0fec860

ordinary-jamie requested review from1st1,asvetlov,kumaraditya303 andwillingc ascode owners

March 9, 2025 12:24

bedevere-appbot added the awaiting review label

Mar 9, 2025

bedevere-appbot mentioned this pull request

Mar 9, 2025

Race condition in asyncio.Server.close#109564

Open

Copy link

ContributorAuthor

ordinary-jamie commentedMar 9, 2025

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 commented

Mar 9, 2025

View reviewed changes

Lib/asyncio/base_events.py

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

Copy link

ContributorAuthor

ordinary-jamieMar 9, 2025

Choose a reason for hiding this comment

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

Not sure about the nameSHUTDOWN here, but needed something more "definitive" thanclosed.

If we do useSHUTDOWN should I also renameServer._wakeup ->Server._shutdown?

Copy link

Member

ZeroIntensityMar 25, 2025

Choose a reason for hiding this comment

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

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

I think that makes sense.

I'm more worried aboutINITIALIZED here; nothing is really initialized, other than the Python object, which isn't particularly useful knowledge. Let's call it something likeNOT_SERVING orNOT_YET_STARTED.

Copy link

ContributorAuthor

ordinary-jamieMar 30, 2025

Choose a reason for hiding this comment

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

Addressed in07129e5

Lib/asyncio/base_events.py Outdated

		self._wakeup()

		def _wakeup(self):
		match self._state:

Copy link

ContributorAuthor

ordinary-jamieMar 9, 2025

Choose a reason for hiding this comment

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

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

Copy link

Member

ZeroIntensityMar 10, 2025

Choose a reason for hiding this comment

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

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

Copy link

Contributor

kumaraditya303Mar 10, 2025

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

ordinary-jamieMar 30, 2025

Choose a reason for hiding this comment

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

Addressed in8e409b7

Lib/asyncio/selector_events.py Outdated

Comment on lines 797 to 802

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

Copy link

ContributorAuthor

ordinary-jamieMar 9, 2025

Choose a reason for hiding this comment

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

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

Usedtransport.abort overtransport.close to force clear the buffer, since no server exists to service that.

A return statement was added to theelse block to prevent the transport getting tracked inloop._transports

fix: fix proactor transport for windows

f3b96bf

ZeroIntensity added the topic-asyncio label

Mar 10, 2025

ZeroIntensity reviewed

Mar 10, 2025

View reviewed changes

Copy link

Member

ZeroIntensity left a comment

Choose a reason for hiding this comment

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

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

Lib/asyncio/base_events.py Outdated

		self._wakeup()

		def _wakeup(self):
		match self._state:

Copy link

Member

ZeroIntensityMar 10, 2025

Choose a reason for hiding this comment

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

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

ZeroIntensity self-requested a review

March 10, 2025 11:06

Copy link

Contributor

kumaraditya303 commentedMar 10, 2025

I'll try to review by this week.

fix: change match to if statements

8e409b7

ZeroIntensity reviewed

Mar 25, 2025

View reviewed changes

Copy link

Member

ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!

Lib/asyncio/base_events.py OutdatedShow resolvedHide resolved

Lib/asyncio/base_events.py

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

Copy link

Member

ZeroIntensityMar 25, 2025

Choose a reason for hiding this comment

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

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 atry/except to undo this.

Copy link

ContributorAuthor

ordinary-jamieMar 30, 2025

Choose a reason for hiding this comment

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

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 aforce kwarg/flag?

I pushed an initial fix with a simpletry/except to recover the previous state on fail in48a3c0d

Lib/asyncio/base_events.py

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

Copy link

Member

ZeroIntensityMar 25, 2025

Choose a reason for hiding this comment

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

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

I think that makes sense.

Lib/asyncio/base_events.py Outdated

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

Copy link

Member

ZeroIntensityMar 25, 2025

Choose a reason for hiding this comment

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

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 isSERVING orINITIALIZED.

Copy link

ContributorAuthor

ordinary-jamieMar 30, 2025

Choose a reason for hiding this comment

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

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 theSERVING state, nor theINITIALIZED/NOT_STARTED state)

Made a fix in:7f3481b

Lib/asyncio/base_events.py OutdatedShow resolvedHide resolved

Lib/asyncio/proactor_events.py Outdated

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

Copy link

Member

ZeroIntensityMar 25, 2025

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

ordinary-jamieMar 30, 2025

Choose a reason for hiding this comment

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

Ah yup sorry, my misunderstanding! Addressed in5832655

I commentedhere 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 forServer.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.

ordinary-jamieand others added6 commits

March 30, 2025 23:51

Update Lib/asyncio/base_events.py

a92158a

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

Update Lib/asyncio/base_events.py

44d24fb

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

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

Copy link

ContributorAuthor

ordinary-jamie commentedMar 30, 2025

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.