Skip to content
This repository has been archived by the owner on Sep 13, 2018. It is now read-only.

Allow shutting down a TcpServer #115

Closed
wants to merge 2 commits into from

Conversation

casey
Copy link
Contributor

@casey casey commented Jan 10, 2017

serve() and with_handle() now take a future which tell a TcpServer when
to shut down. The functions now also return a TcpServerHandle, a future
which resolves once the server has shut down.

Also added the serve_forever() convenience method with the old behavior
of serving forever.

I implemented the simplest possible codec and proto so that I could actually
instantiate a tcp server to test shutdown. Is there a better way?

Feel free to bikeshed away on the interface here, a lot could probably be
improved.

Fixes #101

casey added 2 commits January 9, 2017 21:34
serve() and with_handle() now take a future which tell a TcpServer when
to shut down. The functions now also return a TcpServerHandle, a future
which resolves once the server has shut down.

Also added the serve_forever() convenience method with the old behavior
of serving forever.
@casey
Copy link
Contributor Author

casey commented Jan 24, 2017

Let me know if there is interest in this PR, and I'll fix the merge conflicts.

@carllerche
Copy link
Member

Ping @alexcrichton. You did some work w/ shutting down the server for Hyper. Could you evaluate this PR?

@alexcrichton
Copy link
Contributor

Thanks for the PR @casey! Some thoughts of mine:

  • It's a bit unfortunate that there's a whole dedicated thread to just waiting on the original future to shut down other threads. I think we can probably get away without this extra thread.
  • This is a breaking change currently so we'll likely want to figure out how to slot this in elsewhere.
  • I think we can enhance the return value quite a bit. Pioneered in hyper I think it may make sense to basically do very little and return an opaque struct with accessors for the local TCP addr, shutdown, timeout config, etc. That struct can be extended over time.
  • I think we'll want to handle shutdown a little differently than here at an architectural level. This implements once a shutdown signal is received everything is torn down. Ideally there'd be a grace period to allow active connections to finish (while halting accepting new connections) before everything is torn down. That grace period would be bounded, however.

You can see some examples of this in hyper's server module, and I think we may want to lift some ideas from there here.

What do you think?

@alexcrichton
Copy link
Contributor

I've posted my thinking here: #135

@casey
Copy link
Contributor Author

casey commented Jan 31, 2017

Closing, #135 is better.

@casey casey closed this Jan 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean shutdown of TcpServer
3 participants