Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide helpers for common cancellation patterns #40

Closed
Finomnis opened this issue Jun 6, 2022 · 10 comments
Closed

Provide helpers for common cancellation patterns #40

Finomnis opened this issue Jun 6, 2022 · 10 comments

Comments

@Finomnis
Copy link
Owner

Finomnis commented Jun 6, 2022

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)

@Finomnis
Copy link
Owner Author

Finomnis commented Jun 6, 2022

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.

@Finomnis
Copy link
Owner Author

Finomnis commented Jun 7, 2022

@viveksjain What do you think?

@Finomnis
Copy link
Owner Author

Finomnis commented Jun 7, 2022

@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 :)

@viveksjain
Copy link

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¢ :)

@Finomnis
Copy link
Owner Author

Finomnis commented Jun 8, 2022

"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¢ ;)

@viveksjain
Copy link

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.

@Finomnis
Copy link
Owner Author

Finomnis commented Jun 9, 2022

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())

@Finomnis
Copy link
Owner Author

Finomnis commented Jun 10, 2022

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

@Finomnis
Copy link
Owner Author

Finomnis commented Jun 10, 2022

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(())
}

@Finomnis
Copy link
Owner Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants