From 8aec341f28a00012e1771d5c35d2647e11830755 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Wed, 9 Jul 2025 01:49:31 +0200 Subject: executor: return error when creating the spawntoken, not when spawning. --- embassy-executor/src/raw/mod.rs | 18 +++--- embassy-executor/src/spawner.rs | 65 +++------------------- embassy-executor/tests/test.rs | 29 +++++----- .../tests/ui/return_impl_future_nonsend.rs | 2 +- .../tests/ui/return_impl_future_nonsend.stderr | 6 +- embassy-executor/tests/ui/return_impl_send.stderr | 2 +- embassy-executor/tests/ui/spawn_nonsend.rs | 2 +- embassy-executor/tests/ui/spawn_nonsend.stderr | 10 ++-- 8 files changed, 43 insertions(+), 91 deletions(-) (limited to 'embassy-executor') diff --git a/embassy-executor/src/raw/mod.rs b/embassy-executor/src/raw/mod.rs index a7e65360d..bdaa32951 100644 --- a/embassy-executor/src/raw/mod.rs +++ b/embassy-executor/src/raw/mod.rs @@ -41,7 +41,7 @@ use self::state::State; use self::util::{SyncUnsafeCell, UninitCell}; pub use self::waker::task_from_waker; use super::SpawnToken; -use crate::Metadata; +use crate::{Metadata, SpawnError}; #[no_mangle] extern "Rust" fn __embassy_time_queue_item_from_waker(waker: &Waker) -> &'static mut TimerQueueItem { @@ -220,11 +220,11 @@ impl TaskStorage { /// /// Once the task has finished running, you may spawn it again. It is allowed to spawn it /// on a different executor. - pub fn spawn(&'static self, future: impl FnOnce() -> F) -> SpawnToken { + pub fn spawn(&'static self, future: impl FnOnce() -> F) -> Result, SpawnError> { let task = AvailableTask::claim(self); match task { - Some(task) => task.initialize(future), - None => SpawnToken::new_failed(), + Some(task) => Ok(task.initialize(future)), + None => Err(SpawnError::Busy), } } @@ -353,10 +353,10 @@ impl TaskPool { } } - fn spawn_impl(&'static self, future: impl FnOnce() -> F) -> SpawnToken { + fn spawn_impl(&'static self, future: impl FnOnce() -> F) -> Result, SpawnError> { match self.pool.iter().find_map(AvailableTask::claim) { - Some(task) => task.initialize_impl::(future), - None => SpawnToken::new_failed(), + Some(task) => Ok(task.initialize_impl::(future)), + None => Err(SpawnError::Busy), } } @@ -367,7 +367,7 @@ impl TaskPool { /// This will loop over the pool and spawn the task in the first storage that /// is currently free. If none is free, a "poisoned" SpawnToken is returned, /// which will cause [`Spawner::spawn()`](super::Spawner::spawn) to return the error. - pub fn spawn(&'static self, future: impl FnOnce() -> F) -> SpawnToken { + pub fn spawn(&'static self, future: impl FnOnce() -> F) -> Result, SpawnError> { self.spawn_impl::(future) } @@ -380,7 +380,7 @@ impl TaskPool { /// SAFETY: `future` must be a closure of the form `move || my_async_fn(args)`, where `my_async_fn` /// is an `async fn`, NOT a hand-written `Future`. #[doc(hidden)] - pub unsafe fn _spawn_async_fn(&'static self, future: FutFn) -> SpawnToken + pub unsafe fn _spawn_async_fn(&'static self, future: FutFn) -> Result, SpawnError> where FutFn: FnOnce() -> F, { diff --git a/embassy-executor/src/spawner.rs b/embassy-executor/src/spawner.rs index cd2113a28..83d896b76 100644 --- a/embassy-executor/src/spawner.rs +++ b/embassy-executor/src/spawner.rs @@ -23,39 +23,28 @@ use crate::Metadata; /// Once you've invoked a task function and obtained a SpawnToken, you *must* spawn it. #[must_use = "Calling a task function does nothing on its own. You must spawn the returned SpawnToken, typically with Spawner::spawn()"] pub struct SpawnToken { - pub(crate) raw_task: Option, + pub(crate) raw_task: raw::TaskRef, phantom: PhantomData<*mut S>, } impl SpawnToken { pub(crate) unsafe fn new(raw_task: raw::TaskRef) -> Self { Self { - raw_task: Some(raw_task), + raw_task, phantom: PhantomData, } } - /// Return a SpawnToken that represents a failed spawn. - pub fn new_failed() -> Self { - Self { - raw_task: None, - phantom: PhantomData, - } - } - - /// Returns the task ID if available, otherwise 0 + /// Returns the task ID. /// This can be used in combination with rtos-trace to match task names with IDs pub fn id(&self) -> u32 { - match self.raw_task { - None => 0, - Some(t) => t.id(), - } + self.raw_task.id() } /// Get the metadata for this task. You can use this to set metadata fields /// prior to spawning it. pub fn metadata(&self) -> &Metadata { - self.raw_task.unwrap().metadata() + self.raw_task.metadata() } } @@ -164,30 +153,10 @@ impl Spawner { /// Spawn a task into an executor. /// /// You obtain the `token` by calling a task function (i.e. one marked with `#[embassy_executor::task]`). - pub fn spawn(&self, token: SpawnToken) -> Result<(), SpawnError> { + pub fn spawn(&self, token: SpawnToken) { let task = token.raw_task; mem::forget(token); - - match task { - Some(task) => { - unsafe { self.executor.spawn(task) }; - Ok(()) - } - None => Err(SpawnError::Busy), - } - } - - // Used by the `embassy_executor_macros::main!` macro to throw an error when spawn - // fails. This is here to allow conditional use of `defmt::unwrap!` - // without introducing a `defmt` feature in the `embassy_executor_macros` package, - // which would require use of `-Z namespaced-features`. - /// Spawn a task into an executor, panicking on failure. - /// - /// # Panics - /// - /// Panics if the spawning fails. - pub fn must_spawn(&self, token: SpawnToken) { - unwrap!(self.spawn(token)); + unsafe { self.executor.spawn(task) } } /// Convert this Spawner to a SendSpawner. This allows you to send the @@ -245,25 +214,9 @@ impl SendSpawner { /// Spawn a task into an executor. /// /// You obtain the `token` by calling a task function (i.e. one marked with `#[embassy_executor::task]`). - pub fn spawn(&self, token: SpawnToken) -> Result<(), SpawnError> { + pub fn spawn(&self, token: SpawnToken) { let header = token.raw_task; mem::forget(token); - - match header { - Some(header) => { - unsafe { self.executor.spawn(header) }; - Ok(()) - } - None => Err(SpawnError::Busy), - } - } - - /// Spawn a task into an executor, panicking on failure. - /// - /// # Panics - /// - /// Panics if the spawning fails. - pub fn must_spawn(&self, token: SpawnToken) { - unwrap!(self.spawn(token)); + unsafe { self.executor.spawn(header) } } } diff --git a/embassy-executor/tests/test.rs b/embassy-executor/tests/test.rs index 530314ac3..85c5dc1d9 100644 --- a/embassy-executor/tests/test.rs +++ b/embassy-executor/tests/test.rs @@ -65,7 +65,7 @@ fn executor_task() { } let (executor, trace) = setup(); - executor.spawner().spawn(task1(trace.clone())).unwrap(); + executor.spawner().spawn(task1(trace.clone()).unwrap()); unsafe { executor.poll() }; unsafe { executor.poll() }; @@ -93,7 +93,7 @@ fn executor_task_rpit() { } let (executor, trace) = setup(); - executor.spawner().spawn(task1(trace.clone())).unwrap(); + executor.spawner().spawn(task1(trace.clone()).unwrap()); unsafe { executor.poll() }; unsafe { executor.poll() }; @@ -120,7 +120,7 @@ fn executor_task_self_wake() { } let (executor, trace) = setup(); - executor.spawner().spawn(task1(trace.clone())).unwrap(); + executor.spawner().spawn(task1(trace.clone()).unwrap()); unsafe { executor.poll() }; unsafe { executor.poll() }; @@ -152,7 +152,7 @@ fn executor_task_self_wake_twice() { } let (executor, trace) = setup(); - executor.spawner().spawn(task1(trace.clone())).unwrap(); + executor.spawner().spawn(task1(trace.clone()).unwrap()); unsafe { executor.poll() }; unsafe { executor.poll() }; @@ -188,7 +188,7 @@ fn waking_after_completion_does_not_poll() { let waker = Box::leak(Box::new(AtomicWaker::new())); let (executor, trace) = setup(); - executor.spawner().spawn(task1(trace.clone(), waker)).unwrap(); + executor.spawner().spawn(task1(trace.clone(), waker).unwrap()); unsafe { executor.poll() }; waker.wake(); @@ -200,7 +200,7 @@ fn waking_after_completion_does_not_poll() { unsafe { executor.poll() }; // Clears running status // Can respawn waken-but-dead task - executor.spawner().spawn(task1(trace.clone(), waker)).unwrap(); + executor.spawner().spawn(task1(trace.clone(), waker).unwrap()); unsafe { executor.poll() }; @@ -250,7 +250,7 @@ fn waking_with_old_waker_after_respawn() { let waker = Box::leak(Box::new(AtomicWaker::new())); let (executor, trace) = setup(); - executor.spawner().spawn(task1(trace.clone(), waker)).unwrap(); + executor.spawner().spawn(task1(trace.clone(), waker).unwrap()); unsafe { executor.poll() }; unsafe { executor.poll() }; // progress to registering the waker @@ -273,8 +273,7 @@ fn waking_with_old_waker_after_respawn() { let (other_executor, other_trace) = setup(); other_executor .spawner() - .spawn(task1(other_trace.clone(), waker)) - .unwrap(); + .spawn(task1(other_trace.clone(), waker).unwrap()); unsafe { other_executor.poll() }; // just run to the yield_now waker.wake(); // trigger old waker registration @@ -338,22 +337,22 @@ fn task_metadata() { // check no task name let (executor, _) = setup(); - executor.spawner().spawn(task1(None)).unwrap(); + executor.spawner().spawn(task1(None).unwrap()); unsafe { executor.poll() }; // check setting task name - let token = task1(Some("foo")); + let token = task1(Some("foo")).unwrap(); token.metadata().set_name("foo"); - executor.spawner().spawn(token).unwrap(); + executor.spawner().spawn(token); unsafe { executor.poll() }; - let token = task1(Some("bar")); + let token = task1(Some("bar")).unwrap(); token.metadata().set_name("bar"); - executor.spawner().spawn(token).unwrap(); + executor.spawner().spawn(token); unsafe { executor.poll() }; // check name is cleared if the task pool slot is recycled. let (executor, _) = setup(); - executor.spawner().spawn(task1(None)).unwrap(); + executor.spawner().spawn(task1(None).unwrap()); unsafe { executor.poll() }; } diff --git a/embassy-executor/tests/ui/return_impl_future_nonsend.rs b/embassy-executor/tests/ui/return_impl_future_nonsend.rs index b8c184b21..77b3119d6 100644 --- a/embassy-executor/tests/ui/return_impl_future_nonsend.rs +++ b/embassy-executor/tests/ui/return_impl_future_nonsend.rs @@ -15,7 +15,7 @@ fn task() -> impl Future { } fn send_spawn(s: SendSpawner) { - s.spawn(task()).unwrap(); + s.spawn(task().unwrap()); } fn main() {} diff --git a/embassy-executor/tests/ui/return_impl_future_nonsend.stderr b/embassy-executor/tests/ui/return_impl_future_nonsend.stderr index 8aeb9738a..51944ad65 100644 --- a/embassy-executor/tests/ui/return_impl_future_nonsend.stderr +++ b/embassy-executor/tests/ui/return_impl_future_nonsend.stderr @@ -1,8 +1,8 @@ error: future cannot be sent between threads safely --> tests/ui/return_impl_future_nonsend.rs:18:13 | -18 | s.spawn(task()).unwrap(); - | ^^^^^^ future created by async block is not `Send` +18 | s.spawn(task().unwrap()); + | ^^^^^^^^^^^^^^^ future created by async block is not `Send` | = help: within `impl Sized`, the trait `Send` is not implemented for `*mut ()` note: captured value is not `Send` @@ -13,5 +13,5 @@ note: captured value is not `Send` note: required by a bound in `SendSpawner::spawn` --> src/spawner.rs | - | pub fn spawn(&self, token: SpawnToken) -> Result<(), SpawnError> { + | pub fn spawn(&self, token: SpawnToken) { | ^^^^ required by this bound in `SendSpawner::spawn` diff --git a/embassy-executor/tests/ui/return_impl_send.stderr b/embassy-executor/tests/ui/return_impl_send.stderr index 759be1cde..5d19465ec 100644 --- a/embassy-executor/tests/ui/return_impl_send.stderr +++ b/embassy-executor/tests/ui/return_impl_send.stderr @@ -97,7 +97,7 @@ note: required by a bound in `TaskPool::::spawn` | impl TaskPool { | ^^^^^^ required by this bound in `TaskPool::::spawn` ... - | pub fn spawn(&'static self, future: impl FnOnce() -> F) -> SpawnToken { + | pub fn spawn(&'static self, future: impl FnOnce() -> F) -> Result, SpawnError> { | ----- required by a bound in this associated function = note: this error originates in the attribute macro `embassy_executor::task` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/embassy-executor/tests/ui/spawn_nonsend.rs b/embassy-executor/tests/ui/spawn_nonsend.rs index 4c4cc7697..601041941 100644 --- a/embassy-executor/tests/ui/spawn_nonsend.rs +++ b/embassy-executor/tests/ui/spawn_nonsend.rs @@ -10,7 +10,7 @@ async fn task(non_send: *mut ()) { } fn send_spawn(s: SendSpawner) { - s.spawn(task(core::ptr::null_mut())).unwrap(); + s.spawn(task(core::ptr::null_mut()).unwrap()); } fn main() {} diff --git a/embassy-executor/tests/ui/spawn_nonsend.stderr b/embassy-executor/tests/ui/spawn_nonsend.stderr index 2a06c8b94..25bd7d78d 100644 --- a/embassy-executor/tests/ui/spawn_nonsend.stderr +++ b/embassy-executor/tests/ui/spawn_nonsend.stderr @@ -12,8 +12,8 @@ error[E0277]: `*mut ()` cannot be sent between threads safely 7 | #[embassy_executor::task] | ------------------------- within this `impl Sized` ... -13 | s.spawn(task(core::ptr::null_mut())).unwrap(); - | ----- ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut ()` cannot be sent between threads safely +13 | s.spawn(task(core::ptr::null_mut()).unwrap()); + | ----- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut ()` cannot be sent between threads safely | | | required by a bound introduced by this call | @@ -26,8 +26,8 @@ note: required because it's used within this closure note: required because it appears within the type `impl Sized` --> src/raw/mod.rs | - | pub unsafe fn _spawn_async_fn(&'static self, future: FutFn) -> SpawnToken - | ^^^^^^^^^^ + | pub unsafe fn _spawn_async_fn(&'static self, future: FutFn) -> Result, SpawnError> + | ^^^^^^^^^^ note: required because it appears within the type `impl Sized` --> tests/ui/spawn_nonsend.rs:7:1 | @@ -36,6 +36,6 @@ note: required because it appears within the type `impl Sized` note: required by a bound in `SendSpawner::spawn` --> src/spawner.rs | - | pub fn spawn(&self, token: SpawnToken) -> Result<(), SpawnError> { + | pub fn spawn(&self, token: SpawnToken) { | ^^^^ required by this bound in `SendSpawner::spawn` = note: this error originates in the attribute macro `embassy_executor::task` (in Nightly builds, run with -Z macro-backtrace for more info) -- cgit