-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[tokio-util] AbortOnDropHandle::spawn shortcut #7555
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
Conversation
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.
This new interface will surprise downstream, and people will be confused by it because this isn't what AbortOnDropHandle
is supposed to do.
Would you mind elaborating on your motivation? it's difficult for me to grasp the current motivation in the PR description.
- What are the challenges without this feature?
- How does this feature solve these challanges?
I don't understand. What it is supposed to do?
99% of tasks I create needs abort on drop (and I guess, in many cases, other people should be doing that too). So shortcut this this common operation is desirable. Shortcut can be member function of
This PR makes this common pattern more ergonomic. |
For existing interface, downstream have to do this. let hdl = AbortOnDropHandle::new(tokio::spawn(...)); And you want a shortcut like this, is my understanding correct? let hdl = AbortOnDropHandle::spawn(...); If so, would you mind adding this kind of example to the PR description? I believe this can help reviewers to get the motivation in 10 seconds. |
83a4f88
to
b2514fd
Compare
Added an example. |
I believe this is 99% of `AbortOnDropHandle` use cases.
b2514fd
to
bec2347
Compare
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.
I searched the history of AbortOnDropHandle
, there was a short discussion about AbortOnDropHandle::spawn
, see #6786 (comment).
Initially, there was a AbortOnDropHandle::spawn
, but it was removed before merging.
Shortcut can be member function of
AbortOnDropHandle
, or could be standalone function likespawn_abort_on_drop
.
A standalone function spawn_abort_on_drop
was also considered, but the AbortOnDropHandle::new
was win.
I think having a shortcut is beneficial, but we need a approach which looks better than spawn_abort_on_drop
.
Thanks for your PR! I closed this PR as currently we don't have a better approach. Feel free to re-open this PR or open a new once once you have a better approach. |
Motivation
Some code is written incorrectly: it spawns a task, but does not track whether task is still needed, e.g. request was not dropped.
AbortOnDropHandle
helps with this problem, but it is a bit unergonomic.Solution
Add
AbortOnDropHandle::spawn
shortcut.