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

@asvetlov

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

**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.

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.

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?

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

@aaugustin

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

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.

GH-79033: Fix asyncio.Server.wait_closed() (#98582)

I think I have a fix. The check at the top of wait_closed() should actually be

        if self._waiters is None:
            return

This 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.

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.

Yeah, but internally an Event is implemented similarly, and this code was already there… Can you approve the PR?

gh-79033: Try to fix asyncio.Server.wait_closed() again (GH-111336)

pythongh-79033: Try to fix asyncio.Server.wait_closed() again (python…

[3.12] gh-79033: Try to fix asyncio.Server.wait_closed() again (GH-11…

pythongh-79033: Try to fix asyncio.Server.wait_closed() again (python…

Ensure proper cleanup in MockImapServer

Close writers in test

The latest PR is merged. Is there more to do in this issue?

I think it's done. Closing.

pythongh-79033: Try to fix asyncio.Server.wait_closed() again (python…

Navigation Menu

Repository navigation

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

Linked PRs

13 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

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

Description

Linked PRs

Activity

aymericaugustin commented on Sep 30, 2018

aymericaugustin commented on Sep 30, 2018

aymericaugustin commented on May 30, 2021

kumaraditya303 commented on Oct 12, 2022

gvanrossum commented on Oct 23, 2022

gvanrossum commented on Oct 24, 2022

13 remaining items

gvanrossum commented on Oct 25, 2023

Dreamsorcerer commented on Oct 26, 2023

gvanrossum commented on Oct 26, 2023

encukou commented on Feb 6, 2024

gvanrossum commented on Feb 6, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions