-
Notifications
You must be signed in to change notification settings - Fork 121
refactor(scheduler): use Own and RawRef for TaskQueue
#514
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,17 @@ | ||
| use std::{cell::RefCell, future::Future, marker::PhantomData, rc::Rc, sync::Arc, task::Waker}; | ||
| use std::{cell::RefCell, future::Future, marker::PhantomData, rc::Rc, task::Waker}; | ||
|
|
||
| use async_task::{Runnable, Task}; | ||
| use compio_driver::NotifyHandle; | ||
| use crossbeam_queue::SegQueue; | ||
| use slab::Slab; | ||
|
|
||
| use crate::runtime::scheduler::{ | ||
| drop_hook::DropHook, local_queue::LocalQueue, send_wrapper::SendWrapper, | ||
| drop_hook::DropHook, local_queue::LocalQueue, raw_ref::Own, send_wrapper::SendWrapper, | ||
| }; | ||
|
|
||
| mod drop_hook; | ||
| mod local_queue; | ||
| mod raw_ref; | ||
| mod send_wrapper; | ||
|
|
||
| /// A task queue consisting of a local queue and a synchronized queue. | ||
|
|
@@ -99,7 +100,7 @@ impl TaskQueue { | |
| /// A scheduler for managing and executing tasks. | ||
| pub(crate) struct Scheduler { | ||
| /// Queue for scheduled tasks. | ||
| task_queue: Arc<TaskQueue>, | ||
| task_queue: Own<TaskQueue>, | ||
|
|
||
| /// `Waker` of active tasks. | ||
| active_tasks: Rc<RefCell<Slab<Waker>>>, | ||
|
|
@@ -115,7 +116,7 @@ impl Scheduler { | |
| /// Creates a new `Scheduler`. | ||
| pub(crate) fn new(event_interval: usize) -> Self { | ||
| Self { | ||
| task_queue: Arc::new(TaskQueue::new()), | ||
| task_queue: Own::new(TaskQueue::new()), | ||
| active_tasks: Rc::new(RefCell::new(Slab::new())), | ||
| event_interval, | ||
| _local_marker: PhantomData, | ||
|
|
@@ -149,16 +150,15 @@ impl Scheduler { | |
| }; | ||
|
|
||
| let schedule = { | ||
| // The schedule closure is managed by the `Waker` and may be dropped on another | ||
| // thread, so use `Weak` to ensure the `TaskQueue` is always dropped | ||
| // on the creator thread. | ||
| let task_queue = Arc::downgrade(&self.task_queue); | ||
| let task_queue = self.task_queue.raw_ref(); | ||
|
|
||
| move |runnable| { | ||
| // The `upgrade()` never fails because all tasks are dropped when the | ||
| // `Scheduler` is dropped, if a `Waker` is used after that, the | ||
| // schedule closure will never be called. | ||
| task_queue.upgrade().unwrap().push(runnable, ¬ify); | ||
| // SAFETY: | ||
| // `Scheduler` drains and drops all `Runnable`s before it is dropped. | ||
| // If this closure is still invoked and can push a `Runnable`, the | ||
| // `Scheduler` is necessarily still alive, so the `task_queue` reference | ||
| // obtained from `raw_ref()` is valid. | ||
| unsafe { task_queue.as_ref() }.push(runnable, ¬ify); | ||
|
Comment on lines
+157
to
+161
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How about the scheduler is being dropped while the waker is kept on another thread?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no such guarantee. Here's a possible case:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There’s an important detail that was overlooked. In
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but the given case says that the waker is waked before
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain about the memory leaks? Especially in #513 . At least memory leaks are safe and sound. Or we might need to rethink about writing our own
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I mean by “memory leak” refers to the case where the closure captures an
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weak is atomic so the upgrade is always safe. But yes, if it fails, the only safe solution is to panic, I think...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that’s the choice right now: a memory leak or a panic. As a temporary workaround, the small chance of a memory leak is still the better option. Later we can look for a cleaner solution (maybe rewrite the task).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, let's focus on #513, then. I'll push a commit to forget instead of panic. |
||
| } | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| use std::{ops::Deref, ptr::NonNull}; | ||
|
|
||
| /// A value with ownership. | ||
| pub(crate) struct Own<T: ?Sized>(Box<T>); | ||
|
|
||
| impl<T> Own<T> { | ||
| /// Creates a new [`Own`]. | ||
| pub(crate) fn new(value: T) -> Self { | ||
| Own(Box::new(value)) | ||
| } | ||
| } | ||
|
|
||
| impl<T: ?Sized> Own<T> { | ||
| /// Returns a [`RawRef`] to the owned value. | ||
| pub(crate) fn raw_ref(&self) -> RawRef<T> { | ||
| RawRef(NonNull::from(&*self.0)) | ||
| } | ||
| } | ||
|
|
||
| impl<T: ?Sized> Deref for Own<T> { | ||
| type Target = T; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| } | ||
| } | ||
|
|
||
| /// A reference to an owned value without lifetime tracking. | ||
| pub(crate) struct RawRef<T: ?Sized>(NonNull<T>); | ||
|
|
||
| impl<T: ?Sized> RawRef<T> { | ||
| /// Returns a shared reference to the value. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller must ensure the associated [`Own<T>`] outlives the returned | ||
| /// reference. | ||
| pub(crate) const unsafe fn as_ref(&self) -> &T { | ||
| // SAFETY: | ||
| // - The `NonNull` is created from a valid reference in `Own::raw_ref()`. | ||
| // - Only shared reference is returned, so aliasing rules are not violated. | ||
| // - The validity of the returned reference is ensured by the caller. | ||
| unsafe { self.0.as_ref() } | ||
| } | ||
| } | ||
|
|
||
| /// `Sync` and `Send` implementations follow `&T`. | ||
| unsafe impl<T: ?Sized + Sync> Sync for RawRef<T> {} | ||
| unsafe impl<T: ?Sized + Sync> Send for RawRef<T> {} | ||
|
|
||
| impl<T: ?Sized> Clone for RawRef<T> { | ||
| fn clone(&self) -> Self { | ||
| *self | ||
| } | ||
| } | ||
|
|
||
| impl<T: ?Sized> Copy for RawRef<T> {} |
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.
The safety reasoning here has a potential soundness issue. While the comment states that "Scheduler drains and drops all Runnables before it is dropped", this doesn't account for Wakers that may have been cloned and stored outside of the scheduler's control (e.g., in user code or external futures).
If a Waker is cloned and stored elsewhere, then used after the Runtime/Scheduler is dropped, the schedule closure would dereference the dangling RawRef, causing undefined behavior. The previous implementation using
Arc::downgrade()andupgrade()was safer because it would fail gracefully (panic on unwrap) rather than cause UB.Consider either: