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

Add a way to cooperatively shut down the server #1237

Closed
wants to merge 1 commit into from

Conversation

notriddle
Copy link
Contributor

Closes #180

@jebrosen
Copy link
Collaborator

This specifically implements this approach from #180 (comment):

provide a mechanism to cooperatively stop responses, etc.

The implementation looks good, but it would be good to have more test coverage. I am also not yet sure it's the approach we want to go with; I would like to see more discussion before we commit to an API.

@notriddle
Copy link
Contributor Author

I'm almost certain that I'd want cooperative connection teardown, for a couple of reasons:

  • Everything else in async-land is based on cooperative resource management, so it seems arbitrary to use anything else here.

  • The whole point of having "clean" shutdown is to avoid closing a connection until after you're done sending your response. If clean shutdown doesn't provide that, then it doesn't have much reason to exist. This means not stopping a request before it's done. But eternal requests are never done (unless, as implemented in this PR, it decides it's done because Rocket asks).

  • You are probably running underneath a service manager like systemd, kubernetes, heroku, runit... Those systems all work by sending you service a signal to cooperatively shut down, which would trigger the code written in this PR, and then if you don't shut yourself down after ten seconds or so they would hard-kill you. No point in duplicating their functionality.

@jebrosen
Copy link
Collaborator

But eternal requests are never done (unless, as implemented in this PR, it decides it's done because Rocket asks).

This is pretty much what my uncertainty hinges on: this mechanism would require that all potentially infinite Responder implementations "know" about shutdown. In general, I am uncomfortable with APIs that must be "opted into" in order to be correct.

How to integrate this can be difficult to determine; for example a modification of my rocket-rooms example would require a call to self.shutdown_notification_handle.poll() somewhere, but it's not entirely obvious where that should be and is use-case dependent. It would be easier to add it to the user-side code by select!ing between events and the shutdown notification, but I think it should be the responsibility of the underlying Responder.

@notriddle
Copy link
Contributor Author

Then what's the point of having a clean-shutdown API other than guaranteeing that connections aren't closed until they're done?

@jebrosen
Copy link
Collaborator

Then what's the point of having a clean-shutdown API other than guaranteeing that connections aren't closed until they're done?

Clean shutdown isn't only about the connections; it is also about the rest of the application state and for some users that aspect can be more important. For example an application using the async branch can already be assured that Drop handlers will run on shutdown.

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.

2 participants