GeezerVison is showing github.com — Counter-intuitive behavior of Server.close() / wait_closed() · Issue #79033 · python/cpython
Counter-intuitive behavior of Server.close() / wait_closed() #79033Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2018-09-30.14:16:34.429>
labels = ['type-bug', '3.7', 'expert-asyncio']
title = 'Counter-intuitive behavior of Server.close() / wait_closed()'
updated_at = <Date 2021-05-30.16:58:41.511>
user = 'https://bugs.python.org/aymericaugustin' bugs.python.org fields:
activity = <Date 2021-05-30.16:58:41.511>
actor = 'aymeric.augustin'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2018-09-30.14:16:34.429>
creator = 'aymeric.augustin'
dependencies = []
files = []
hgrepos = []
issue_num = 34852
keywords = []
message_count = 3.0
messages = ['326725', '326732', '394772']
nosy_count = 4.0
nosy_names = ['asvetlov', 'chris.jerdonek', 'aymeric.augustin', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue34852'
versions = ['Python 3.7'] Linked PRs
Activityaymericaugustin commented on Sep 30, 2018 MannequinAuthor More actions **Summary**
- Is it correct for
Server.wait_closed() (as implemented in asyncio) to be a no-op after Server.close()?
- How can I tell that all incoming connections have been received by
connection_made() after Server.close()?
**Details**
After calling Server.close(), _sockets is None, which makes Server.wait_closed() a no-op: it returns immediately without doing anything (as mentioned in https://bugs.python.org/issue33727).
I'm not sure why the docs suggest to call wait_closed() after close() if it's a no-op. My best guess is: "this design supports third-party event loops that requires an asynchronous API for closing servers, but the built-in event loops don't need that". Does someone know?
I wrote a very simple server that merely accepts connections. I ran experiments where I saturate the server with incoming client connections and close it. I checked what happens around close() (and wait_closed() -- but as it doesn't do anything after close() I'll just say close() from now on.)
The current implementation appears to work as documented, assuming an rather low level interpretation of the docs of Server.close().
Stop serving: close listening sockets and set the sockets attribute to None.
Correct -- I'm not seeing any accept calls in BaseSelectorEventLoop._accept_connection after close().
The sockets that represent existing incoming client connections are left open.
Correct -- if "existing incoming client connections" is interpreted as "client connections that have gone through accept".
The server is closed asynchronously, use the wait_closed() coroutine to wait until the server is closed.
I'm seeing calls to connection_made() after close() because BaseSelectorEventLoop._accept_connection2 triggers connection_made() asynchronously with call_soon().
This is surprising for someone approaching asyncio from the public API rather than the internal implementation. connection_made() is the first contact with new connections. The concept of "an existing incoming client connection for which connection_made() wasn't called yet" is unexpected.
This has practical consequences.
Consider a server that keeps track of established connections via connection_made and connection_lost. If this server calls Server.close(), awaits Server.wait_closed(), makes a list of established connections and terminates them, there's no guarantee that all connections will be closed. Indeed, new connections may appear and call connection_made() after close() and wait_closed() returned!
wait_closed() seems ineffective for this use case.
aymericaugustin commented on Sep 30, 2018 MannequinAuthor More actions For now I will use the following hack:
server.close()
await server.wait_closed()# Workaround for wait_closed() not quite doing
# what I want.
await asyncio.sleep(0)
# I believe that all accepted connections have reached
# connection_made() at this point.
aymericaugustin commented on May 30, 2021 MannequinAuthor More actions Would it make sense to add await asyncio.sleep(0) in Server.wait_closed() to ensure that all connections reach connection_made() before wait_closed() returns?
This would be fragile but it would be an improvement over the current behavior, wouldn't it? kumaraditya303 commented on Oct 12, 2022 IMO it should wait for all connections to be closed and the current behavior seems like an oversight. gvanrossum commented on Oct 23, 2022 Yeah, there seem to be an unfinished thought here. Some of the logic I've discovered:
- The only waiter ever added to
self._waiters is wait_closed() itself
_attach() / _detach() serve as a kind of reference count, counting transports- The only calls are in
_SelectorTransport and _ProactorBasePipeTransport
_detach is called when the connection is lost, and wakes up the wait_closed() call when the count reaches zero
It does seem likely that wait_closed() just shouldn't check for self._sockets and then all will be well?
Somebody should give this a try.
CC: @aaugustin @1st1 gvanrossum commented on Oct 24, 2022 I think I got it (GH-79033). I don't think we should backport this, even though it's fixing a bug -- code that got the close sequence wrong but managed to get away with it might suddenly hang.
I need to think about a test for the correct behavior. gvanrossum commented on Oct 25, 2023 I think I have a fix. The check at the top of wait_closed() should actually be
if self._waiters is None:
returnThis makes it block until both conditions are true, as was the intended semantics (that we never quite got right): the server is closed and there are no active connections left. This new, simpler, check works because _wakeup() is the only place that sets _waiters to None, and it is called exactly when both conditions have become true (either in _detach() or in close()).
PR forthcoming. Dreamsorcerer commented on Oct 26, 2023 Sounds good. Another option may have been to have an asyncio.Event to signal when the server is closed, and then it could simply await on that. gvanrossum commented on Oct 26, 2023 Yeah, but internally an Event is implemented similarly, and this code was already there… Can you approve the PR? encukou commented on Feb 6, 2024 The latest PR is merged. Is there more to do in this issue? gvanrossum commented on Feb 6, 2024 I think it's done. Closing.
MetadataMetadataStatusShow more project fields
DevelopmentNo branches or pull requestsIssue actions
|