Finomnis/tokio-graceful-shutdown

Provide helpers for common cancellation patterns

Finomnis opened this issue · 10 comments

I did get the chance to try out the beta version, and happy to say that it works great. Helped me clean up my code quite a bit. Feel free to close this issue!

One observation I will mention is that in most cases, I would like a newly started subsystem to select! on the parent's on_shutdown_requested so that it shuts down immediately on error, and this was causing quite a bit of boilerplate. I was able to address it with the following macro though

/// Start a nested subsystem that `select!`s on `subsys.on_shutdown_requested()` to stop automatically.
/// `subsystem_to_start` must have type `Future<Output=anyhow::Result<()>>`.
#[macro_export]
macro_rules! start {
    ($subsys:expr, $name:expr, $subsystem_to_start:expr) => {
        let subsys_clone = $subsys.clone();
        $subsys.start($name, move |_h: SubsystemHandle| async move {
            tokio::select! {
                r = $subsystem_to_start => r,
                _ = subsys_clone.on_shutdown_requested() => Ok::<(), anyhow::Error>(())
            }
        });
    };
}

Will leave it up to you on whether such a feature makes sense to be in the lib or not (in which case it can probably be another function on SubsystemHandle, rather than a macro).

Originally posted by @viveksjain in #37 (comment)

I thought about things like that.

I think I'd prefer to write:

let async_action_result = some_async_action.cancel_on_shutdown(subsys).await?;

Opposed to the current

let async_action_result = select!{
    res = some_async_action => res,
    _ = subsys.on_shutdown_requested() => return Ok(())
};

I've thought about adding a trait for all Future objects for a while now, just didn't get to implement it.
I think that's cleaner than providing a macro.

@viveksjain What do you think?

@viveksjain
Might have gotten hooked a little :D
What do you think about this?
https://github.com/Finomnis/tokio-graceful-shutdown/blob/ffeb8f3f2653f1731a00e7f236614d2cd86e297f/examples/09_task_cancellation.rs

I'm not sure if that's exactly your usecase, I'd value your feedback nonetheless :)

Hmm I guess if you can use ? then it can help reduce boilerplate, but in that specific example it doesn't seem that much less code than select! - with the latter arguably being more "idiomatic"/well-known for async code. At least what I was thinking was something more like

Toplevel::new()
  .start_auto_shutdown("countdown", myhandler)

which of course provides less flexibility (the only SubsystemHandle that it will wait on is the one for myhandler), but reduces boilerplate. Perhaps it could take a third argument on what to return in case shutdown is requested, so that at least the handler return type can be generic. My 2¢ :)

"less idiomatic/well-known" - I was hoping it was a known pattern, it's similar to tokio::timeout.

OK I get your use case.

Three things I struggle with:

  • I don't want to encourage people to cancel on a higher level, if rather haben them cancel inside of the subsystem.
  • I'd like to be a library to be as generic as possible, I don't want to become a library that overwhelms its user by providing a special function for every corner case.
  • I'm not sure how I would name such a function.

I think the most idiomatic I could think of would be neither a new function nor a macro, but instead a wrapper struct:

.start(AutoCancel(my_subsys))

where AutoCancel's constructor can take a FnOnce() -> Result, and itself is a subsystem.

Although I think it would be easy enough to implement, so I would almost leave that to the user if he desires such a functionality.

I value your 2¢ ;)

Ah I see, I just wasn't familiar with tokio::timeout. I think select is a more common use case so it's mentioned in the guide but not sure timeout is. I think your points make a lot of sense, and I really like the AutoCancel idea. Will look into switching from my macro to something like AutoCancel.

Now that I think about it, it would probably have to be a function instead of a struct. Or you would have to write .start(AutoCancel::new(my_subsys).into_subsystem())

Turns out this is not as easy as I thought. Maybe your macro solution has something to it after all :D

you could always write |subsys_handle| my_subsystem().cancel_on_shutdown(&subsys_handle), as soon as the new PR is merged. but that would raise an error on cancellation.

Or you could wrap your entire subsystem in:

async fn subsys(subsys_handle) -> Result<()> {
    async {
        ...
    }.cancel_on_shutdown(&subsys_handle).await.err();
    Ok(())
}

Then let's close this for now, I think I'd leave such a macro out. Maybe I'll ad it later if more people demand it, but right now it feels like too much of a corner case.

If you want further discussion about things, feel free to contact me on https://discord.com/invite/tokio.