Graceful shutdown

[ivan770]

Tide could store JoinHandle of every request it handles, and await them when server is shutting down.

tide/src/server.rs

Lines 300 to 311 in 7d0b848

	task::spawn(async move {
	let res = async_h1::accept(&addr, stream, |req| async {
	let res = this.respond(req).await;
	let res = res.map_err(|_| io::Error::from(io::ErrorKind::Other))?;
	Ok(res)
	})
	.await;
	if let Err(err) = res {
	log::error!("async-h1 error", { error: err.to_string() });
	}
	});

async-std book

[Fishrock123]

I think the server only shuts down in a panic! or due to an external signal, such SIGINT, or whatever the Windows equivalents are.

I suppose this could possibly be desirable for SIGTERM which allows for such a thing. If you panic I'm not sure it is ever really safe to continue.

[Fishrock123]

Oh I think I see what you are saying - essentially we should have a way to await all tasks which are still alive, I think?

[ivan770]

Oh I think I see what you are saying - essentially we should have a way to await all tasks which are still alive, I think?

Yes. I mean, new requests are not handled, but at least those that are still awaiting response should be finished

[mehcode]

Was just coming to make an issue for this. It's critical from a production standpoint that a load balancer or swarm manager can initiate a controlled or graceful shutdown.

Any requests that came in before the "shut down please" request should still finish. Icing would be a way to shutdown with a timeout but I think we can compose that if there is an API for shutdown.

The way I envision usage would be a shutdown method on Server. As Server is clone-able we could clone, wait for SIGTERM, and call shutdown on it.

[repnop]

Definitely agree with @mehcode. Having the ability to gracefully shutdown the server is pretty important for me as well. 👍

[ririsoft]

FWIW here is how I do it using the async-ctrlc crate:

use async_std::prelude::FutureExt;

async fn run() -> Result<()> {
    let ctrlc = async {
        CtrlC::new().expect("Cannot use CTRL-C handler").await;
        println!("termination signal received, stopping server...");
        Ok(())
    };

    let app = async {
        let app = tide::new();
        app.listen("localhost:8080").await
    };

    app.race(ctrlc).await?;
    println!("server stopped.");
    Ok(())
}

Please note that this will not wait for all running requests to complete, this is a "brutal graceful shutdown", if I dare. But at least it handles SIGTERM and CTRL-C shutdown (cross platform) which is better than nothing.

[repnop]

I did try messing around with adding graceful shutdown to tide yesterday evening and even got tests working, but ran into a bit of a problem that I'll outline in hopes someone better at async code can figure out a better solution.

The actual waiting for requests to finish is pretty simple with the design I used: spawn a task that uses a Receiver (I used the futures-channel channel) and use that as a stream to await all job handles and when the channel is closed from the sender side, it'll finish all of the jobs and break out of looping, which can be used to signal all finished requests have been finished.

The other part seems just as simple as well: if someone has initiated a shutdown (I used an Arc<AtomicBool> here and set it to true in a shutdown method), stop listening for new connections, close the sender portion of the channel, and wait for the existing ones to finish (I also had it serve up 503s during this time), then return. This worked, except that using stream combinators to figure out when to stop polling would result in it waiting for a request to be served after shutdown was initiated because, I think, the TcpListener was only notifying the waker it was ready when there was new data on the socket, at which point it would then see shutdown is true on the next poll cycle, and go as expected.

To get around this I was able to use a select! and check the value of shutdown if the incoming.next() didn't resolve, but to make it stop infinitely looping there needed to be a task::yield_now().await on the false case, which seemed less than ideal, and where I ended my attempt.

If anyone has thoughts on how to avoid the issue, do let me know and I can give them a try and report back. Or if anyone else wants to take a stab at implementing it themselves, by all means 👍

[yoshuawuyts]

I've thought about this a little in the past, and gathered some notes on this. What I think we need to do is use stop-token here:

tide/src/server.rs

Lines 334 to 335 in 293610b

	let mut incoming = listener.incoming();
	while let Some(stream) = incoming.next().await {

and here

tide/src/server.rs

Lines 292 to 293 in 293610b

	let mut incoming = listener.incoming();
	while let Some(stream) = incoming.next().await {

This allows us to stop receiving incoming requests and allow the server to gracefully exit once a token has been received.

---

However stop-token is not quite the end of the road for us. What I think we need is a tighter integration with Stream and Future. I've also gathered some notes on a design for async-std we could use to make cancellation easier. If we had that we could probably use it to enable cancellation of Tide as well:

let src = StopSource::new();
let token = src.token();

// Send a token to each app we initialize
task::spawn(async move {
    let mut app = tide::new();
    app.stop_on(token.clone());
    app.listen("localhost:8080").await?;
});

// app will stop receiving requests once we drop the `StopSource`.
// this can be be done in response to e.g. a signal handler.

---

It's a bit of a question how we'd combine this today. We could probably define our own StopToken for the time being, and implement the API outlined above. But once we have this in async-std we could probably rely on that. Hope this is helpful!

[GWBasic]

I'm learning Rust, so I tried to see if I could fix this: https://github.com/http-rs/tide/compare/main...GWBasic:GracefulShutdown2?w=1

Specifically, I solved @repnop 's problem by using future::select. It allows awaiting on two futures and returns when the first completes: https://github.com/http-rs/tide/compare/main...GWBasic:GracefulShutdown2?w=1#diff-1f14b8b6953e2388b19efaf814862a89c0b57c45a6814f79ed373fde05d864d0R82

I then created a "CancelationToken" future that returns once it is canceled.

So far, I only know how to test tcp_listener, and I adjusted its tests to work by canceling gracefully: https://github.com/http-rs/tide/compare/main...GWBasic:GracefulShutdown2?w=1#diff-0f530b03216a301ca1004179731df8510170be421ff7abf545db673eca3bc4beR12

I haven't done anything with waiting for ongoing requests to complete, or testing other listeners. I must admit that I've tried to return futures from the listen function; but that leads to lots of fighting with the borrow checker. That's a bit more than I can handle as a Rust novice. ;)

Anyway, if you believe my branch is in line with tide's overall design philosophy, I'm happy to make a pull request and change whatever you want. (Especially for testing the rest of the listeners.) Otherwise, this bug was a great learning exercise for me.

[Fishrock123]

@GWBasic Can you make at least a draft PR?

[GWBasic]

@Fishrock123 : See #746

(Note that some merge conflicts creeped in.)

Looking forward to your feedback!