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

Remove implicit clear in ReadyToRunQueue::drop #2493

Merged
merged 2 commits into from Sep 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 3 additions & 18 deletions futures-util/src/stream/futures_unordered/mod.rs
Expand Up @@ -558,7 +558,7 @@ impl<Fut> FuturesUnordered<Fut> {
pub fn clear(&mut self) {
self.clear_head_all();

// we just cleared all the tasks, and we have &mut self, so this is safe.
// SAFETY: we just cleared all the tasks and we have &mut self
unsafe { self.ready_to_run_queue.clear() };

self.is_terminated.store(false, Relaxed);
Expand All @@ -575,24 +575,9 @@ impl<Fut> FuturesUnordered<Fut> {

impl<Fut> Drop for FuturesUnordered<Fut> {
fn drop(&mut self) {
// When a `FuturesUnordered` is dropped we want to drop all futures
// associated with it. At the same time though there may be tons of
// wakers flying around which contain `Task<Fut>` references
// inside them. We'll let those naturally get deallocated.
self.clear_head_all();

// Note that at this point we could still have a bunch of tasks in the
// ready to run queue. None of those tasks, however, have futures
// associated with them so they're safe to destroy on any thread. At
// this point the `FuturesUnordered` struct, the owner of the one strong
// reference to the ready to run queue will drop the strong reference.
// At that point whichever thread releases the strong refcount last (be
// it this thread or some other thread as part of an `upgrade`) will
// clear out the ready to run queue and free all remaining tasks.
//
// While that freeing operation isn't guaranteed to happen here, it's
// guaranteed to happen "promptly" as no more "blocking work" will
// happen while there's a strong refcount held.
// SAFETY: we just cleared all the tasks and we have &mut self
unsafe { self.ready_to_run_queue.clear() };
}
}

Expand Down
15 changes: 1 addition & 14 deletions futures-util/src/stream/futures_unordered/ready_to_run_queue.rs
Expand Up @@ -94,7 +94,7 @@ impl<Fut> ReadyToRunQueue<Fut> {
//
// # Safety
//
// - All tasks **must** have had their futures dropped already (by FuturesUnordered::clear)
// - All tasks **must** have had their futures dropped already (by FuturesUnordered::clear_head_all)
// - The caller **must** guarantee unique access to `self`
pub(crate) unsafe fn clear(&self) {
loop {
Expand All @@ -107,16 +107,3 @@ impl<Fut> ReadyToRunQueue<Fut> {
}
}
}

impl<Fut> Drop for ReadyToRunQueue<Fut> {
fn drop(&mut self) {
// Once we're in the destructor for `Inner<Fut>` we need to clear out
// the ready to run queue of tasks if there's anything left in there.

// All tasks have had their futures dropped already by the `FuturesUnordered`
// destructor above, and we have &mut self, so this is safe.
unsafe {
self.clear();
}
}
}