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
Immediately drop new task when runtime is shut down #3752
Conversation
Looks related to: #3708 |
} | ||
} | ||
} | ||
drop(remote_queue); |
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.
If the drop here is significant (which it probably is because this is a lock guard), it probably is worth adding a comment saying so to avoid removing it in the future by accident.
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.
Does the drop here in interact with the assert below?
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.
No, I only put it in to make it more visually clear where the lock is dropped.
@@ -429,7 +436,9 @@ impl Schedule for Arc<Shared> { | |||
// safety: the task is inserted in the list in `bind`. | |||
unsafe { cx.tasks.borrow_mut().owned.remove(ptr) } | |||
} else { | |||
self.queue.lock().push_back(Entry::Release(ptr)); | |||
if let Some(queue) = self.queue.lock().as_mut() { |
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.
So what happens if this is None
? Will the task leak?
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.
After digging in, this looks fine, a comment explaining why would be nice.
let jh1 = handle.spawn(futures::future::pending::<()>()); | ||
|
||
drop(rt); | ||
|
||
let jh2 = handle.spawn(futures::future::pending::<()>()); | ||
|
||
let err1 = jh1.now_or_never().unwrap().unwrap_err(); | ||
let err2 = jh2.now_or_never().unwrap().unwrap_err(); |
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.
@carllerche The unwrap()
on err1
here sometimes fails with the threaded scheduler. Isn't the drop call supposed to wait until the runtime is fully shut down?
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 is fixed by the "Fix loom test" commit
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.
Thanks 👍 this looks good to me. There are a couple of comments inline that have requests for comments, but the change looks correct.
Attempt to resolve #3548 by having each runtime's spawn method drop the new task.
I was only able to get it to work for the basic scheduler.