-
Notifications
You must be signed in to change notification settings - Fork 401
feat: add cancellation_token method to RunningService #218
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
feat: add cancellation_token method to RunningService #218
Conversation
currently both RunningService::waiting and RunningService::cancel needs ownership of the RunningService instance, which makes it impossible to poll waiting() while other async task can cancel it A split() function is a common pattern to solve this
|
Perhaps we can simply add a |
|
but DropGuard can't be cloned, unless you want to change how cancellation is handled (if you clone a cancellation token and keep a copy in the RunningService the drop will never be triggered - and currently it also needs an owned value) |
|
I believe it still can be triggered, you can see the source code. https://docs.rs/tokio-util/0.7.15/src/tokio_util/sync/cancellation_token/guard.rs.html#22-26 |
|
@4t145 pushed |
|
I explicitly added the cancellation token as part of the service struct because i don't want to call .disarm() on the drop guard - i want to preserve the "cancel on drop" functionality alternatively we could also implement drop on our own |
That's exactly what I meant |
4t145
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation and Context
currently both RunningService::waiting and RunningService::cancel needs ownership of the RunningService instance
this makes it impossible to poll waiting() while other async task can cancel it
A cancellation_token() function is solves this, cloning the tokio_util::sync::CancellationToken
so we can dispatch a cancel from a separate task
How Has This Been Tested?
Breaking Changes
No breaking change
Types of changes
Checklist
Additional context