Make server shutdown wait on any Detached handler futures

[jgallagher]

I ended up using a WaitGroup to track outstanding handler futures instead of trying to accumulate them into something like a FuturesUnordered or JoinSet, 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.

[jgallagher]

Without the addition of

handler_waitgroup.wait().await;

when constructing join_handle, the test fails here.

[jgallagher]

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]

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.

[smklein]

TIL, this crate seems useful!

[davepacheco]

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?

[davepacheco]

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. 🤷

[davepacheco]

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.

[jgallagher]

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.

[jgallagher]

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?

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 tokio::spawn() and move the upgraded websocket off to a background task, we wouldn't (and still can't) do anything to wait on that.

[davepacheco]

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?

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 tokio::spawn() and move the upgraded websocket off to a background task, we wouldn't (and still can't) do anything to wait on that.

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.