X Tutup
The Wayback Machine - https://web.archive.org/web/20221223094928/https://github.com/python/cpython/issues/79033
Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
aymericaugustin mannequin opened this issue Sep 30, 2018 · 6 comments
Closed

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

aymericaugustin mannequin opened this issue Sep 30, 2018 · 6 comments
Labels
expert-asyncio type-bug An unexpected behavior, bug, or error

Comments

@aymericaugustin
Copy link
Mannequin

aymericaugustin mannequin commented Sep 30, 2018

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

@aymericaugustin
Copy link
Mannequin Author

aymericaugustin mannequin commented Sep 30, 2018

**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 aymericaugustin mannequin added 3.7 expert-asyncio type-bug An unexpected behavior, bug, or error labels Sep 30, 2018
@aymericaugustin
Copy link
Mannequin Author

aymericaugustin mannequin commented Sep 30, 2018

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
Copy link
Mannequin Author

aymericaugustin mannequin commented May 30, 2021

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
Copy link
Contributor

kumaraditya303 commented Oct 12, 2022

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

@gvanrossum
Copy link
Member

gvanrossum commented 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
Copy link
Member

gvanrossum commented 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 added a commit that referenced this issue Nov 24, 2022
It was a no-op when used as recommended (after close()).

I had to debug one test (test__sock_sendfile_native_failure) --
the cleanup sequence for the test fixture was botched.

Hopefully that's not a portend of problems in user code --
this has never worked so people may well be doing this wrong. :-(

Co-authored-by: kumar aditya
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expert-asyncio type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

No branches or pull requests

2 participants
X Tutup