-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Comments
**Summary**
**Details** After calling I'm not sure why the docs suggest to call 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 The current implementation appears to work as documented, assuming an rather low level interpretation of the docs of
Correct -- I'm not seeing any
Correct -- if "existing incoming client connections" is interpreted as "client connections that have gone through
I'm seeing calls to This is surprising for someone approaching asyncio from the public API rather than the internal implementation. This has practical consequences. Consider a server that keeps track of established connections via
|
For now I will use the following hack: server.close()
await server.wait_closed()
|
Would it make sense to add 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. |
Yeah, there seem to be an unfinished thought here. Some of the logic I've discovered:
It does seem likely that 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. |
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
The committed #98582 broke class Server:
# ...
async def wait_closed(self):
if self._waiters is None or self._active_count == 0:
return
waiter = self._loop.create_future()
self._waiters.append(waiter)
await waiter It now can incorrectly assume that the
Before #98582, the condition was Now the |
This appears to be a 3.12 regression and IMO the commit should be just reverted. I don't understand the motivation for the change from the commit message of 5d09d11 (gh-98582). The added test is synthetic, calling directly the underlying private API, something that we shouldn't rely too heavily on in general, aiming for higher-level functional tests instead. |
The test that works in 3.11 but would fail in 3.12 does the following:
We do have to fix that in 3.12.1. |
Based on our aiohttp code, I was originally under the impression that However, I think the referenced change looks correct, because the very first thing that Having looked at all this, I think aiohttp shouldn't be calling |
In 3.11, wouldn't that still be an issue, depending on timing? From my reading of the code, wait_closed() would create a waiter, then as soon as the server wakes up, it will resolve that waiter. I think it's irrelevant whether the close() method is called later or not, right? |
I don't see the timing issue you refer to. The waiter is immediately (without intervening |
Right, I think some of those things could be named better. But, doesn't that still happen when connections drop to 0 normally then? i.e. If we just don't call |
Maybe what you're after is the same race condition that @1st1 reported a few comments back: if I wonder if the fix is to change the condition at the top of if self._sockets is None and self._active_count == 0:
return EDIT: Not quite. But now you've piqued my interest and I'll figure out the true condition. |
I think I have a fix. The check at the top of 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 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? |
* Try to fix asyncio.Server.wait_closed() again I identified the condition that `wait_closed()` is intended to wait for: the server is closed *and* there are no more active connections. When this condition first becomes true, `_wakeup()` is called (either from `close()` or from `_detach()`) and it sets `_waiters` to `None`. So we just check for `self._waiters is None`; if it's not `None`, we know we have to wait, and do so. A problem was that the new test introduced in 3.12 explicitly tested that `wait_closed()` returns immediately when the server is *not* closed but there are currently no active connections. This was a mistake (probably a misunderstanding of the intended semantics). I've fixed the test, and added a separate test that checks exactly for this scenario. I also fixed an oddity where in `_wakeup()` the result of the waiter was set to the waiter itself. This result is not used anywhere and I changed this to `None`, to avoid a GC cycle. * Update Lib/asyncio/base_events.py --------- Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
…GH-111336) * Try to fix asyncio.Server.wait_closed() again I identified the condition that `wait_closed()` is intended to wait for: the server is closed *and* there are no more active connections. When this condition first becomes true, `_wakeup()` is called (either from `close()` or from `_detach()`) and it sets `_waiters` to `None`. So we just check for `self._waiters is None`; if it's not `None`, we know we have to wait, and do so. A problem was that the new test introduced in 3.12 explicitly tested that `wait_closed()` returns immediately when the server is *not* closed but there are currently no active connections. This was a mistake (probably a misunderstanding of the intended semantics). I've fixed the test, and added a separate test that checks exactly for this scenario. I also fixed an oddity where in `_wakeup()` the result of the waiter was set to the waiter itself. This result is not used anywhere and I changed this to `None`, to avoid a GC cycle. * Update Lib/asyncio/base_events.py --------- (cherry picked from commit 2655369) Co-authored-by: Guido van Rossum <guido@python.org> Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
…1336) (#111424) gh-79033: Try to fix asyncio.Server.wait_closed() again (GH-111336) * Try to fix asyncio.Server.wait_closed() again I identified the condition that `wait_closed()` is intended to wait for: the server is closed *and* there are no more active connections. When this condition first becomes true, `_wakeup()` is called (either from `close()` or from `_detach()`) and it sets `_waiters` to `None`. So we just check for `self._waiters is None`; if it's not `None`, we know we have to wait, and do so. A problem was that the new test introduced in 3.12 explicitly tested that `wait_closed()` returns immediately when the server is *not* closed but there are currently no active connections. This was a mistake (probably a misunderstanding of the intended semantics). I've fixed the test, and added a separate test that checks exactly for this scenario. I also fixed an oddity where in `_wakeup()` the result of the waiter was set to the waiter itself. This result is not used anywhere and I changed this to `None`, to avoid a GC cycle. * Update Lib/asyncio/base_events.py --------- (cherry picked from commit 2655369) Co-authored-by: Guido van Rossum <guido@python.org> Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
…GH-111336) * Try to fix asyncio.Server.wait_closed() again I identified the condition that `wait_closed()` is intended to wait for: the server is closed *and* there are no more active connections. When this condition first becomes true, `_wakeup()` is called (either from `close()` or from `_detach()`) and it sets `_waiters` to `None`. So we just check for `self._waiters is None`; if it's not `None`, we know we have to wait, and do so. A problem was that the new test introduced in 3.12 explicitly tested that `wait_closed()` returns immediately when the server is *not* closed but there are currently no active connections. This was a mistake (probably a misunderstanding of the intended semantics). I've fixed the test, and added a separate test that checks exactly for this scenario. I also fixed an oddity where in `_wakeup()` the result of the waiter was set to the waiter itself. This result is not used anywhere and I changed this to `None`, to avoid a GC cycle. * Update Lib/asyncio/base_events.py --------- Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
* Send responses to all ongoing commands. * Then close the writer so that the server can be shutdown. This is required so `wait_closed` can finished which has been fixed in Python 3.12 to actually block, see python/cpython#79033
This is required so that the server can shutdown, after `wait_closed` has been fixed in Python 3.12 to actually block. See python/cpython#79033
The latest PR is merged. Is there more to do in this issue? |
I think it's done. Closing. |
…GH-111336) * Try to fix asyncio.Server.wait_closed() again I identified the condition that `wait_closed()` is intended to wait for: the server is closed *and* there are no more active connections. When this condition first becomes true, `_wakeup()` is called (either from `close()` or from `_detach()`) and it sets `_waiters` to `None`. So we just check for `self._waiters is None`; if it's not `None`, we know we have to wait, and do so. A problem was that the new test introduced in 3.12 explicitly tested that `wait_closed()` returns immediately when the server is *not* closed but there are currently no active connections. This was a mistake (probably a misunderstanding of the intended semantics). I've fixed the test, and added a separate test that checks exactly for this scenario. I also fixed an oddity where in `_wakeup()` the result of the waiter was set to the waiter itself. This result is not used anywhere and I changed this to `None`, to avoid a GC cycle. * Update Lib/asyncio/base_events.py --------- Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
…GH-111336) * Try to fix asyncio.Server.wait_closed() again I identified the condition that `wait_closed()` is intended to wait for: the server is closed *and* there are no more active connections. When this condition first becomes true, `_wakeup()` is called (either from `close()` or from `_detach()`) and it sets `_waiters` to `None`. So we just check for `self._waiters is None`; if it's not `None`, we know we have to wait, and do so. A problem was that the new test introduced in 3.12 explicitly tested that `wait_closed()` returns immediately when the server is *not* closed but there are currently no active connections. This was a mistake (probably a misunderstanding of the intended semantics). I've fixed the test, and added a separate test that checks exactly for this scenario. I also fixed an oddity where in `_wakeup()` the result of the waiter was set to the waiter itself. This result is not used anywhere and I changed this to `None`, to avoid a GC cycle. * Update Lib/asyncio/base_events.py --------- Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
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:
bugs.python.org fields:
Linked PRs
The text was updated successfully, but these errors were encountered: