Counter-intuitive behavior of Server.close() / wait_closed()

[aymericaugustin]

BPO	34852
Nosy	@asvetlov, @cjerdonek, @1st1

^{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

• GH-79033: Fix asyncio.Server.wait_closed() #98582
• GH-79033: docs: Minor improvement to asyncio.Server.wait_closed() #111260
• gh-79033: Try to fix asyncio.Server.wait_closed() again #111336
• [3.12] gh-79033: Try to fix asyncio.Server.wait_closed() again (GH-111336) #111424

[aymericaugustin]

**Summary**

1. Is it correct for Server.wait_closed() (as implemented in asyncio) to be a no-op after Server.close()?
2. 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]

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]

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]

IMO it should wait for all connections to be closed and the current behavior seems like an oversight.

[gvanrossum]

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]

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.