Make server shutdown wait on any Detached handler futures#702
Conversation
| .await | ||
| .is_ok() | ||
| { | ||
| panic!("server shutdown returned while handler running"); |
There was a problem hiding this comment.
Without the addition of
handler_waitgroup.wait().await;when constructing join_handle, the test fails here.
| release_endpoint_tx.send(()).await.unwrap(); | ||
|
|
||
| // Now we can finish waiting for server shutdown. | ||
| teardown_fut.await; |
There was a problem hiding this comment.
Without the explict
mem::drop(self.app_state);in close(), this test hangs forever here, because the wait group is still waiting for the last worker (the one held in DropshotState) to be dropped.
smklein
left a comment
There was a problem hiding this comment.
Thanks for the solid test - always quirky to test these ordering things, but I appreciate that you put in the effort to make this more robust.
| use super::ProbeRegistration; | ||
|
|
||
| use async_stream::stream; | ||
| use debug_ignore::DebugIgnore; |
There was a problem hiding this comment.
TIL, this crate seems useful!
davepacheco
left a comment
There was a problem hiding this comment.
Ugh, now the websocket case makes me wonder whether it's a problem that we don't cancel these things now? Do you happen to know what used to happen before #701 if you shut down a server with request handlers running? What if there was a websocket stream attached?
| slog-term = "2.9.0" | ||
| tokio-rustls = "0.24.0" | ||
| toml = "0.7.4" | ||
| waitgroup = "0.1.2" |
There was a problem hiding this comment.
This is fine. I see this pulls in two deps. It's a shame if there's nothing in tokio or futures that can already do this. 🤷
| api, | ||
| private, | ||
| log, | ||
| handler_waitgroup.worker(), |
There was a problem hiding this comment.
This is probably not something to worry about right now, but: I wonder how debuggable this is, either in situ or post mortem. Like if you walk up to a server that's shutting down and stuck waiting on one of these, would you have any way to know which request it was waiting on? I imagine eventually we'll want to elevate this to an API but that's probably way down the road.
Just to show what I mean, in the past I built something like this:
https://github.com/TritonDataCenter/node-vasync#barrier-coordinate-multiple-concurrent-operations
In that thing, each outstanding operation has a distinct name. You can take the whole object and expose that over a debug HTTP API to ask the server "what are the operations you're waiting on". This proved incredibly useful. But that was more for stuff like our own RSS, where you've got complicated long-running things that could get stuck somewhere. This particular case seems less likely to be a problem. It would just be neat to have a thing like waitgroup but where it was easy to ask it what it was waiting on.
There was a problem hiding this comment.
This is probably not something to worry about right now, but: I wonder how debuggable this is, either in situ or post mortem. Like if you walk up to a server that's shutting down and stuck waiting on one of these, would you have any way to know which request it was waiting on?
Probably not! Internally the waitgroup is just a wrapper around an AtomicWaker, which itself is just a glorified AtomicUsize; I think you could pretty quickly find the count, but assuming it's something like 1, I don't know how you'd find the guilty party.
I think we did the right thing, at least as far as we can - the web socket upgrade handler happens within the normal handler's future, right? And graceful shutdown waits for all handler futures to complete already. If in practice most websocket handlers do their own |
The thing I'm (low-level) worried about is that someone starts up a WebSocket for something like a serial console (just as an example of something that's a best-effort background thing) and now the server cannot be shut down. It seems like the consumer would probably want to cancel that rather than wait for the client to determine when they can shut down. The same applies to ordinary HTTP requests but those are usually short-lived and should usually have aggressive timers on them so they seem less likely to cause a problem. If we previously blocked on WebSockets closing, there's no behavior change here, and this is definitely fine. |
d0d8b37 to
9816cb2
Compare
I ended up using a
WaitGroupto track outstanding handler futures instead of trying to accumulate them into something like aFuturesUnorderedorJoinSet, because we don't actually care about their results: we just want to know when they're all done.Testing this was a little weird: we have to force a situation where we have a detached handler future still running after server shutdown has been requested. It uses a similar pattern to the tests added #701, where the handler will wait for a message on a channel from the test driver before returning, allowing us to construct a client request and then cancel it, leaving the detached handler still running.