-
-
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
gh-79033: Try to fix asyncio.Server.wait_closed() again #111336
Conversation
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.
Maybe @graingert has some time to review? (I'll trade you -- I'm reviewing #110728 right now.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gvanrossum for the additional comments and clarification. I'm making one small docstring clarification and then will merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks @gvanrossum for the PR, and @willingc for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
…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-111424 is a backport of this pull request to the 3.12 branch. |
…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>
…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>
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 fromclose()
or from_detach()
) and it sets_waiters
toNone
. So we just check forself._waiters is None
; if it's notNone
, 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 toNone
, to avoid a GC cycle.📚 Documentation preview 📚: https://cpython-previews--111336.org.readthedocs.build/