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

Service.close #32

Closed
nayato opened this issue Dec 6, 2017 · 7 comments
Closed

Service.close #32

nayato opened this issue Dec 6, 2017 · 7 comments

Comments

@nayato
Copy link

nayato commented Dec 6, 2017

Are there plans to support graceful termination of services? E.g. in Finagle Close() method returning Future is a core part of the Service class.

@carllerche
Copy link
Member

I've been thinking a bunch about the issue of graceful shutdown. I know that finagle includes it as part of its abstraction, but I'm not sure it makes sense to include as part of Tower.

Specifically, I would imagine that graceful shutdown would happen directly with the server. I would think that the server implementation would take some sort of future that completes when the server should start to gracefully shut down. When the future resolves, the server would then drop the service handle. Service values can implement a drop handle if they need to hook into close and perform some cleanup.

Do you think there is something else that I am missing?

@carllerche carllerche mentioned this issue Dec 11, 2017
8 tasks
@nayato
Copy link
Author

nayato commented Dec 19, 2017

sorry, it's hard to link everything together here. I guess a little example would help. E.g. how can one construct a chain of services with NewService, yet provide the linking between server and some inner service in the chain to let server know when shutdown is complete.

@carllerche
Copy link
Member

Well, let me turn it back on you and ask for a concrete example illustrating what you are trying to achieve :)

@olix0r
Copy link
Collaborator

olix0r commented Jan 8, 2018

A specific case I have in mind is the following:

  • my application has an H2 server
  • it handles SIGTERM to initiate shutdown
  • during shutdown, i want to stop accepting new connections and new H2 streams.
  • i want all pending streams to have 10s to complete (after the SIGTERM is handled) before the process stops abruptly. This is done to prevent premature cancelation of in-flight requests.

Bridging the gap between the signal handler and the server may just be a simple Oneshot -- but I'd expect tower servers (and clients, even) to be able to instrument the rest of this shutdown logic...

Ideally, specifically for tower-h2, this would do the right thing by issuing GOAWAY frames to clients.

@carllerche
Copy link
Member

Right, so given this example, the SIGTERM signal is a separate concept to handling the request / response exchange, it doesn't seem like it makes sense for Service to notify the server that the termination was sent.

I'm even not sure how much it makes sense to handle at the tower-h2 level because that lib doesn't manage accepting connections. Off the top of my head, server::Connection might have a start_shutdown fn, which sends the GOAWAY, but that is it.

At the hyper level where the library handles accepting sockets, I would expect that you pass in a shutdown future when starting the server that completes when the SIGTERM happens, then the shutdown logic starts.

@carllerche
Copy link
Member

@nayato

sorry, it's hard to link everything together here. I guess a little example would help. E.g. how can one construct a chain of services with NewService, yet provide the linking between server and some inner service in the chain to let server know when shutdown is complete.

NewService is only responsible for returning a new service :) When shutting down, it is the server's responsibility to stop asking for new services.

The shutdown process is roughly something like:

  • The server stops accepting connections, thus NewService is no longer called. The server impl is responsible for defining how it gets notified of this.
  • In-flight response futures are given some grace period to complete.
  • The server terminates any lingering responses by dropping the future.

@carllerche
Copy link
Member

I believe that this is out of scope. I do think there is a need to figure out graceful shutdown, but I don't think the answer will involve changes to the Service trait.

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

No branches or pull requests

3 participants