-
Notifications
You must be signed in to change notification settings - Fork 82
Add graceful shutdown to TcpServer #135
base: master
Are you sure you want to change the base?
Conversation
This is very similar to the support I added to Hyper and is an alternative to #115. |
One thing I'd like to add to what exists in hyper is the ability to also update the keep-alive status of the currently running services. This would allow hyper to stop accepting new requests on existing connections while trying to shutdown. hyper already has the concept of a shared keep-alive status, which is used by the client, and I could make similar use for the server, but if I were to shift to using the |
bdc91d6
to
71a3053
Compare
@seanmonstar yeah that's a good point. In general it may be useful to propagate the shutdown signal to services being run so they can run custom logic themselves. How one might represent that, though, is... tricky |
This is definitely better than #115. TcpServer should probably now be called something else, like TcpServerBuilder, but that's obviously a breaking change. |
src/tcp_server.rs
Outdated
let service = try!(new_service.new_service()); | ||
let service = NotifyService { | ||
inner: try!(new_service.new_service()), | ||
info: Rc::downgrade(&info2), |
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.
It should increase info.active
when creating new NotifyService
.
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.
Oops, thanks!
This commit prototypes support for graceful shutdown in `TcpServer` through some new methods. A `TcpServer::bind` method was acced which is used to construct a server and unlike `serve` returns the value immediately. The returned value can then be used to inspect the state of the server, finally calling `run` to execute indefinitely or `run_until` to run until a shutdown signal is received.
71a3053
to
ea26470
Compare
Could you expand on the semantics of graceful shutdown a bit? I assume it doesn't allow new connections, but does it allow existing connections to continue to make requests? I've also been working on graceful shutdown for tarpc. It'd be nice to just reuse this capability, but I don't want to be required to use the tcp server itself, because I don't want to require users to hand over management of the reactor. |
@tikue it currently will stop accepting new connections, and let the current ones run (and hopefully shutdown) up until a time limit. An improvement I'd like is for a way to notify the underlying service that shutdown is desired, and hyper can turn off keep-alive for all the existing connections, so that after the next response, they will close. I've been thinking about how to do this, with some possibilities:
|
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.
I don't love TcpServerInstance
as a name, but I also don't have a great alternative and it doesn't seem worth bike shedding at this point.
Maybe once tokio-proto evolves a bit more, the names of all the types can be rethought.
Is there any way this code could be made more modular so that folks not using |
Just wondering about the status of this bug/issue... still doesn't seem to be a clean way to shutdown a |
- Can cleanly shutdown the HTTP server and RPC clients - Cannot shutdown RPC Server as TcpServer doesn't allow for this - tokio-rs/tokio-proto#135
This commit prototypes support for graceful shutdown in
TcpServer
through some new methods. A
TcpServer::bind
method was acced which isused to construct a server and unlike
serve
returns the valueimmediately.
The returned value can then be used to inspect the state of the server,
finally calling
run
to execute indefinitely orrun_until
to rununtil a shutdown signal is received.