rt: add thread (un)parking hooks for LocalRuntime
#7420
Conversation
aryan9600
force-pushed
the
non-send-thread-park
branch
from
be90205 to
eeb45d3
Compare
aryan9600
force-pushed
the
non-send-thread-park
branch
2 times, most recently
from
c124e08 to
4f8b562
Compare
aryan9600
force-pushed
the
non-send-thread-park
branch
from
4f8b562 to
fde0d15
Compare
github-actions
bot
removed
the
R-loom-current-thread
aryan9600
force-pushed
the
non-send-thread-park
branch
from
fde0d15 to
453dbd2
Compare
ADD-SP
added
the
S-waiting-on-author
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]>
aryan9600
force-pushed
the
non-send-thread-park
branch
from
453dbd2 to
980ac53
Compare
ADD-SP
changed the title
LocalRuntime
| /// Marker used to make this !Send and !Sync. | ||
| _phantom: PhantomData<*mut u8>, |
There was a problem hiding this comment.
It’s unusual to place PhantomData as the first field — please move it to the end.
| self.before_park = opts.before_park; | ||
| self.after_unpark = opts.after_unpark; |
There was a problem hiding this comment.
Is adding the support for the following hooks also safe? If so, we should also add them.
on_after_task_pollon_before_task_pollon_task_spawnon_task_terminate
| /// [`Builder`]: crate::runtime::Builder | ||
| #[derive(Default)] | ||
| #[non_exhaustive] | ||
| #[allow(missing_debug_implementations)] |
There was a problem hiding this comment.
This is no more needed. The Debug impl has been added (https://github.com/tokio-rs/tokio/pull/7420/files#diff-f672a058c630d5f7e012e7ee8a2c99d65890433661aed45e324e277121045ea1R30-R37)
| /// To run before the local runtime is parked. | ||
| pub(crate) before_park: Option<Callback>, | ||
|
|
||
| /// To run before the local runtime is spawned. |
There was a problem hiding this comment.
| /// To run before the local runtime is spawned. | |
| /// To run after the local runtime is unparked. |
| }); | ||
|
|
||
| // assert that the callbacks were not called - `Handle::block_on` can not drive IO or timer | ||
| // drivers on a current-thread runtime, so the park/unpark callbacks should not be invoked. |
There was a problem hiding this comment.
The comment above is not clear to me.
Where is the difference with the code for test_on_thread_park_unpark_in_runtime() ?
Until line https://github.com/tokio-rs/tokio/pull/7420/files#diff-3c0fef031dc5f261b671919a47345cbbc60841e2ffcd3287df279290b7297933R79 they are exactly the same. They differ in the bodies of block_on().
|
|
||
| let tid = std::thread::current().id(); | ||
|
|
||
| self.before_park = opts.before_park; |
There was a problem hiding this comment.
IMO this should be done only if opts.on_xyz_abc is Some.
Use case:
let opts = LocalOptions::default();
// no on_before_park/on_after_unpark for opts !
Builder::new_current_thread()
.enable_time()
.on_before_park(...) // 1
.build_local(opts) // 2
.unwrap();Currently the user application sets on_before_park callback at 1) and then 2) will silently wipe them
5 participants
Add support for non
Send+Syncclosures for thread parking and unparking callbacks when using aLocalRuntime. Since aLocalRuntimewill always run its tasks on the same thread, its safe to accept a nonSend+Syncclosure.Motivation
Since
LocalRuntimecan run nonSend+Syncfunctions, we should accept thread parking/unparking callbacks that do not implementSendandSync.Solution
Add two methods on
LocalOptions,on_thread_parkandon_thread_unparkthat 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 ofLocalRuntimefixes #7370