python
gh-123720: When closing an asyncio server, stop the handlers
Since 3.12, when server.wait_closed() was fixed, cancelling a serve_forever() call would wait for the last handler to exit, which may be never if it is waiting to read from a connection that is never closed by the client. To fix this, we insert a close_clients() call in the exit sequence (between close() and wait_closed()).
The repro shows that in 3.11 you need only a single ^C to stop the server; in 3.12 you need two. The fix indeed undoes that regression.
Some philosophical question remain:
- Should we call close_clients() in close(), so everyone who closes a server gets the same benefit?
- Should we use abort_clients() instead? (I think it's better to allow handlers to catch the cancellation and e.g. send a "goodbye" message to the client.)
- Is this a bugfix (backportable) or a new feature? I'm inclined to call it a bugfix, given the regressions in 3.12 and beyond.
Note that there's still a difference with 3.11: in 3.11, the handler receives a CancelledError; in main with this PR added, the handler receives an empty string, indicating that the connection was closed (by close_clients()). Maybe I should do something that actually cancels the handlers? I will look into the feasibility of this.
- Issue: gh-123720
The only failing test is the check for a news blurb. Once I've got time for that I'll move forward.
I think this fix is incomplete. I believe the __aexit__ method of AbstractServer at https://github.com/python/cpython/blob/ef06508f8ef1d2943b2fb1e310ab115b65e489a8/Lib/asyncio/events.py#L253-L255 also needs to be modified to include a call to self.close_clients().
The only failing test is the check for a news blurb. Once I've got time for that I'll move forward.
@gvanrossum Do you plan to complete this PR and merge it?
Sorry I have lost context. Is anyone waiting for it?
No, was just checking if this could be completed before https://github.com/python/cpython/pull/131009
Sorry I have lost context. Is anyone waiting for it?
I reported the original issue (#123720) and I am still waiting for the fix :) Others have reported on that issue that are affected as well. I believe your fix in this PR is correct, and addresses the original issue.
Based on your last messages three things were outstanding:
- A news entry;
- A test case -- @ordinary-jamie provided one;
- Asking RMs whether the fix should be backported to 3.12 and 3.13 (at the time).
It'd be great to at least see it being merged in tip, even if it doesn't get backported right now, as this is still affecting code out there.
Thanks again!
