-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
rt: add support for non-send closures for thread (un)parking #7420
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
base: master
Are you sure you want to change the base?
Conversation
be90205
to
eeb45d3
Compare
// This is specifically used for executing callbacks when using a `LocalRuntime`, since the | ||
// callbacks will never be sent across threads and thus do not need to be Send or Sync. | ||
struct UnsafeSendSync<T>(T); | ||
unsafe impl<T> Send for UnsafeSendSync<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.
shouldn't this be unsafe impl<T: Send> Send for UnsafeSendSync<T> {}
and unsafe impl<T: Send + Sync> Sync for UnsafeSendSync<T> {}
Any unsafe usage should be documented with // SAFETY:
at least
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 whole point is that T
is not Send
or Sync
.
sorry, my bad, will update the comments to begin with // SAFETY:
eeb45d3
to
c124e08
Compare
Add support for non `Send`+`Sync` closures for thread parking and unparking callbacks when using a `LocalRuntime`. Since a `LocalRuntime` will always run its tasks on the same thread, its safe to accept a non `Send`+`Sync` closure. Signed-off-by: Sanskar Jaiswal <[email protected]>
c124e08
to
4f8b562
Compare
// use super::LocalOptions; | ||
|
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.
// use super::LocalOptions; |
Outdated lines?
/// This can be used to start work only when the executor is idle, or for bookkeeping | ||
/// and monitoring purposes. | ||
/// | ||
/// This differs from the `Builder::on_thread_park` method in that it accepts a non Send + Sync |
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 differs from the `Builder::on_thread_park` method in that it accepts a non Send + Sync | |
/// This differs from the [`Builder::on_thread_park`] method in that it accepts a non Send + Sync |
#[non_exhaustive] | ||
#[allow(missing_debug_implementations)] |
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 guess this is because we cannot #[derive(Debug)]
for Fn
. We could try to impl Debug
manually, so that we don't need this clippy exception.
/// Callback for a worker parking itself | ||
pub(crate) before_park: Option<Callback>, |
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.
/// Callback for a worker parking itself | |
pub(crate) before_park: Option<Callback>, | |
/// To run before the local runtime is parked. | |
pub(crate) before_park: Option<Callback>, |
This is to align with
tokio/tokio/src/runtime/builder.rs
Lines 82 to 83 in b7a75b5
/// To run before each worker thread is parked. | |
pub(super) before_park: Option<Callback>, |
/// Callback for a worker unparking itself | ||
pub(crate) after_unpark: Option<Callback>, |
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.
/// Callback for a worker unparking itself | |
pub(crate) after_unpark: Option<Callback>, | |
/// To run before the local runtime is spawned. | |
pub(crate) after_unpark: Option<Callback>, |
/// ``` | ||
/// # use tokio::runtime::{Builder, LocalOptions}; | ||
/// # pub fn main() { | ||
/// let (tx, rx) = std::sync::mpsc::channel(); | ||
/// let mut opts = LocalOptions::default(); | ||
/// opts.on_thread_park(move || match rx.recv() { | ||
/// Ok(x) => println!("Received from channel: {}", x), | ||
/// Err(e) => println!("Error receiving from channel: {}", e), | ||
/// }); | ||
/// | ||
/// let runtime = Builder::new_current_thread() | ||
/// .enable_time() | ||
/// .build_local(opts) | ||
/// .unwrap(); | ||
/// | ||
/// runtime.block_on(async { | ||
/// tokio::task::spawn_local(async move { | ||
/// tx.send(42).unwrap(); | ||
/// }); | ||
/// tokio::time::sleep(std::time::Duration::from_millis(1)).await; | ||
/// }) | ||
/// } | ||
/// ``` |
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 might not be rendered correctly.
/// ``` | ||
/// # use tokio::runtime::{Builder, LocalOptions}; | ||
/// # pub fn main() { | ||
/// let (tx, rx) = std::sync::mpsc::channel(); | ||
/// let mut opts = LocalOptions::default(); | ||
/// opts.on_thread_unpark(move || match rx.recv() { | ||
/// Ok(x) => println!("Received from channel: {}", x), | ||
/// Err(e) => println!("Error receiving from channel: {}", e), | ||
/// }); | ||
/// | ||
/// let runtime = Builder::new_current_thread() | ||
/// .enable_time() | ||
/// .build_local(opts) | ||
/// .unwrap(); | ||
/// | ||
/// runtime.block_on(async { | ||
/// tokio::task::spawn_local(async move { | ||
/// tx.send(42).unwrap(); | ||
/// }); | ||
/// tokio::time::sleep(std::time::Duration::from_millis(1)).await; | ||
/// }) | ||
/// } | ||
/// ``` |
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 might not be rendered correctly.
self | ||
} | ||
|
||
/// Executes function `f` just after a thread unparks (starts executing tasks). |
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.
/// Executes function `f` just after a thread unparks (starts executing tasks). | |
/// Executes function `f` just after the local runtime unparks (starts executing tasks). |
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.
We need a test to make sure callbacks will not be executed when using the Handle::block_on
.
#[test] | ||
fn test_on_thread_park_in_runtime() { | ||
let mut opts = LocalOptions::default(); | ||
let called = std::rc::Rc::new(std::cell::RefCell::new(false)); |
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.
let called = std::rc::Rc::new(std::cell::RefCell::new(false)); | |
// this makes the `on_thread_park` callback `!Send + !Sync` | |
let called = std::rc::Rc::new(std::cell::RefCell::new(false)); |
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.
We also want to cover the on_thread_unpark
callback.
Add support for non
Send
+Sync
closures for thread parking and unparking callbacks when using aLocalRuntime
. Since aLocalRuntime
will always run its tasks on the same thread, its safe to accept a nonSend
+Sync
closure.Motivation
Since
LocalRuntime
can run nonSend
+Sync
functions, we should accept thread parking/unparking callbacks that do not implementSend
andSync
.Solution
Add two methods on
LocalOptions
,on_thread_park
andon_thread_unpark
that accept aFn() + 'static
. These callbacks are then converted intoFn() + Send + Sync + 'static
. This requires unsafe code, but unsafe is acceptable here because of the behaviour ofLocalRuntime
fixes #7370