Add server::Handle::shutdown #117
Conversation
src/server.rs
Outdated
| } | ||
| }); | ||
| reactor.handle().spawn(server.select(shutdown).then(|_| { | ||
| debug!("Entering lame duck mode."); |
There was a problem hiding this comment.
What does this mean?
Also, am I reading this right that the server won't actually shut down? That is, will the thread that started the server now return from the call to .run()?
There was a problem hiding this comment.
It means existing connections will be honored but no new connections will be accepted.
Yeah, so I was a bit hesitant to implement that as part of this, because it'd require one of the following, all of which I wanted to spend a bit more time thinking about and getting feedback on:
- Have the
Handlehold the server future so that it can run it directly. Thenrunreturns once the server stops accepting connections. This requires exposing some type parameters onHandlethat are, I think, implementation details. - Same as 1 but box the server to avoid type parameters. Probably not a huge performance hit since it's just boxing the top-level future, but would be better to benchmark first.
- Signal shutdown some other way, maybe via an
AtomicBool. Not sure if there'd be any overhead in checking an atomic once per iteration ofloop { reactor.turn(None); }.
There was a problem hiding this comment.
I'm going to go with 2. It also means shutdown will be instantaneous rather than honoring existing connections. I think that's fine for its purpose.
Also, shutdown handle blocks until shutdown completes.
The future handle has addr() and shutdown(), but not run(). Move handles into server::future and server::sync submodules.
Cargo.toml
Outdated
| @@ -15,7 +15,7 @@ description = "An RPC framework for Rust with a focus on ease of use." | |||
| travis-ci = { repository = "google/tarpc" } | |||
|
|
|||
| [dependencies] | |||
| bincode = "1.0.0-alpha2" | |||
| bincode = { git = "https://github.com/tikue/bincode", branch = "faster-byte-buf-deserialization" } | |||
There was a problem hiding this comment.
Was this an intentional inclusion?
|
The |
|
Tokio seems to have plans to support shutdown. See tokio-rs/tokio-core#33. I don't think we need to wait for that to play out before making progress on this. We can always revisit later. |
| shutdown: shutdown2, | ||
| connections: connections2, | ||
| }) | ||
| .map_err(Warn("UnboundedReceiver resolved to an Err; can it do that?")) |
There was a problem hiding this comment.
Is there any guidance from the docs about whether this can happen? If it's never supposed to resolve to an error, maybe this should be unreachable!?
There was a problem hiding this comment.
Yeah I just tried replacing the () with ! in futures-rs and it still compiles, so it must never be used.
There was a problem hiding this comment.
I'll do it in the next PR (this file doesn't actually exist anymore)
Fixes #62
Edit: as @jonhoo pointed out, this doesn't actually allow
runto return right now. It just sends servers into lame duck mode, where they honor existing connections but don't accept new ones.Edit 2: the server now shuts down after all pre-lameduck connections are closed.
Edit 3: changed to two handles:
server::sync::Handleandserver::future::Handle. The future one hasaddr()andshutdown()and the sync one has those plusrun().