Skip to content

Conversation

@gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Sep 27, 2024

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.

@gvanrossum
Copy link
Member Author

The only failing test is the check for a news blurb. Once I've got time for that I'll move forward.

@danpascu
Copy link

I think this fix is incomplete. I believe the __aexit__ method of AbstractServer at

async def __aexit__(self, *exc):
self.close()
await self.wait_closed()
also needs to be modified to include a call to self.close_clients().

@kumaraditya303
Copy link
Contributor

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?

@gvanrossum
Copy link
Member Author

Sorry I have lost context. Is anyone waiting for it?

@kumaraditya303
Copy link
Contributor

Sorry I have lost context. Is anyone waiting for it?

No, was just checking if this could be completed before #131009

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants