From 896b8294f811b57927c37a30805ae3051fea46fc Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Sat, 23 Aug 2025 00:21:14 +0800 Subject: [PATCH 1/5] Examine timeout crash Signed-off-by: Michael X. Grey --- rclrs/src/executor.rs | 21 +++++++++++++++++++++ rclrs/src/wait_set.rs | 10 +++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/rclrs/src/executor.rs b/rclrs/src/executor.rs index 3c67c91cf..c94d86491 100644 --- a/rclrs/src/executor.rs +++ b/rclrs/src/executor.rs @@ -487,3 +487,24 @@ impl CreateBasicExecutor for Context { self.create_executor(runtime) } } + +#[cfg(test)] +mod tests { + use crate::*; + use std::time::{Duration, Instant}; + + #[test] + fn test_spin_once() { + let context = Context::default(); + let mut executor = context.create_basic_executor(); + let node = executor.create_node("spin_once").unwrap(); + let subscription = node.create_subscription("test", |msg: crate::vendor::example_interfaces::msg::Empty| { }).unwrap(); + + let start = Instant::now(); + + for _ in 0..10 { + println!("Spinning exeuctor: {:?}", start.elapsed()); + executor.spin(SpinOptions::default().timeout(Duration::from_secs(1))); + } + } +} diff --git a/rclrs/src/wait_set.rs b/rclrs/src/wait_set.rs index a7951f00a..793526fbf 100644 --- a/rclrs/src/wait_set.rs +++ b/rclrs/src/wait_set.rs @@ -114,11 +114,15 @@ impl WaitSet { }) } }; + + dbg!(timeout_ns); // SAFETY: The comments in rcl mention "This function cannot operate on the same wait set // in multiple threads, and the wait sets may not share content." - // We cannot currently guarantee that the wait sets may not share content, but it is - // mentioned in the doc comment for `add_subscription`. - // Also, the rcl_wait_set is obviously valid. + // * We guarantee that the wait sets do not share content by funneling + // the exeuctor of each primitive to one (and only one) WaitSet when + // the primitive gets constructed. The primitive executors are never + // allowed to transfer wait sets. + // * The rcl_wait_set is kept valid by the lifecycle of the WaitSet struct. match unsafe { rcl_wait(&mut self.handle.rcl_wait_set, timeout_ns) }.ok() { Ok(_) => (), Err(error) => match error { From 3dbd37e72ca2dbbe9bca009d66f4a6f687c7678d Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Sat, 23 Aug 2025 10:41:27 +0800 Subject: [PATCH 2/5] Fix executor timeout Signed-off-by: Michael X. Grey --- rclrs/src/executor.rs | 12 ++++-------- rclrs/src/wait_set.rs | 32 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/rclrs/src/executor.rs b/rclrs/src/executor.rs index c94d86491..c67fca583 100644 --- a/rclrs/src/executor.rs +++ b/rclrs/src/executor.rs @@ -491,20 +491,16 @@ impl CreateBasicExecutor for Context { #[cfg(test)] mod tests { use crate::*; - use std::time::{Duration, Instant}; + use std::time::Duration; #[test] - fn test_spin_once() { + fn test_timeout() { let context = Context::default(); let mut executor = context.create_basic_executor(); - let node = executor.create_node("spin_once").unwrap(); - let subscription = node.create_subscription("test", |msg: crate::vendor::example_interfaces::msg::Empty| { }).unwrap(); - - let start = Instant::now(); + let _node = executor.create_node(&format!("test_timeout_{}", line!())).unwrap(); for _ in 0..10 { - println!("Spinning exeuctor: {:?}", start.elapsed()); - executor.spin(SpinOptions::default().timeout(Duration::from_secs(1))); + executor.spin(SpinOptions::default().timeout(Duration::from_millis(1))); } } } diff --git a/rclrs/src/wait_set.rs b/rclrs/src/wait_set.rs index 793526fbf..c6eb7717c 100644 --- a/rclrs/src/wait_set.rs +++ b/rclrs/src/wait_set.rs @@ -115,24 +115,15 @@ impl WaitSet { } }; - dbg!(timeout_ns); // SAFETY: The comments in rcl mention "This function cannot operate on the same wait set // in multiple threads, and the wait sets may not share content." + // * The we have exclusive access to rcl_wait_set because this is a + // mutable borrow of WaitSet, which houses rcl_wait_set. // * We guarantee that the wait sets do not share content by funneling - // the exeuctor of each primitive to one (and only one) WaitSet when - // the primitive gets constructed. The primitive executors are never - // allowed to transfer wait sets. - // * The rcl_wait_set is kept valid by the lifecycle of the WaitSet struct. - match unsafe { rcl_wait(&mut self.handle.rcl_wait_set, timeout_ns) }.ok() { - Ok(_) => (), - Err(error) => match error { - RclrsError::RclError { code, msg } => match code { - RclReturnCode::WaitSetEmpty => (), - _ => return Err(RclrsError::RclError { code, msg }), - }, - _ => return Err(error), - }, - } + // the waitable of each primitive to one (and only one) WaitSet when + // the primitive gets constructed. The waitables are never allowed to + // move between wait sets. + let r = unsafe { rcl_wait(&mut self.handle.rcl_wait_set, timeout_ns) }.ok(); // Remove any waitables that are no longer being used for waitable in self.primitives.values_mut() { @@ -164,7 +155,16 @@ impl WaitSet { ); } - Ok(()) + match r { + Ok(_) => Ok(()), + Err(error) => match error { + RclrsError::RclError { code, msg } => match code { + RclReturnCode::WaitSetEmpty => Ok(()), + _ => Err(RclrsError::RclError { code, msg }), + }, + _ => Err(error), + }, + } } /// Get a count of the different kinds of entities in the wait set. From 70cf856f48cbd5aeda31daecdae0fc64a774c1d0 Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Sat, 23 Aug 2025 10:46:38 +0800 Subject: [PATCH 3/5] Add test assertions Signed-off-by: Michael X. Grey --- rclrs/src/executor.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rclrs/src/executor.rs b/rclrs/src/executor.rs index c67fca583..496571e8e 100644 --- a/rclrs/src/executor.rs +++ b/rclrs/src/executor.rs @@ -500,7 +500,9 @@ mod tests { let _node = executor.create_node(&format!("test_timeout_{}", line!())).unwrap(); for _ in 0..10 { - executor.spin(SpinOptions::default().timeout(Duration::from_millis(1))); + let r = executor.spin(SpinOptions::default().timeout(Duration::from_millis(1))); + assert_eq!(r.len(), 1); + assert!(matches!(r[0], RclrsError::RclError { code: RclReturnCode::Timeout, .. })); } } } From 63a1f9d4b45053482fb1abafb94e5cd6c66d97d9 Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Sat, 23 Aug 2025 10:52:27 +0800 Subject: [PATCH 4/5] Skip readiness check if an error is reported Signed-off-by: Michael X. Grey --- rclrs/src/wait_set.rs | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/rclrs/src/wait_set.rs b/rclrs/src/wait_set.rs index c6eb7717c..e57005405 100644 --- a/rclrs/src/wait_set.rs +++ b/rclrs/src/wait_set.rs @@ -123,18 +123,31 @@ impl WaitSet { // the waitable of each primitive to one (and only one) WaitSet when // the primitive gets constructed. The waitables are never allowed to // move between wait sets. - let r = unsafe { rcl_wait(&mut self.handle.rcl_wait_set, timeout_ns) }.ok(); + let r = match unsafe { rcl_wait(&mut self.handle.rcl_wait_set, timeout_ns) }.ok() + { + Ok(_) => Ok(()), + Err(error) => match error { + RclrsError::RclError { code, msg } => match code { + RclReturnCode::WaitSetEmpty => Ok(()), + _ => Err(RclrsError::RclError { code, msg }), + }, + _ => Err(error), + }, + }; // Remove any waitables that are no longer being used for waitable in self.primitives.values_mut() { waitable.retain(|w| w.in_use()); } - // For the remaining entities, check if they were activated and then run - // the callback for those that were. - for waiter in self.primitives.values_mut().flat_map(|v| v) { - if waiter.is_ready(&self.handle.rcl_wait_set) { - f(&mut *waiter.primitive)?; + // Do not check the readiness if an error was reported. + if !r.is_err() { + // For the remaining entities, check if they were activated and then run + // the callback for those that were. + for waiter in self.primitives.values_mut().flat_map(|v| v) { + if waiter.is_ready(&self.handle.rcl_wait_set) { + f(&mut *waiter.primitive)?; + } } } @@ -155,16 +168,7 @@ impl WaitSet { ); } - match r { - Ok(_) => Ok(()), - Err(error) => match error { - RclrsError::RclError { code, msg } => match code { - RclReturnCode::WaitSetEmpty => Ok(()), - _ => Err(RclrsError::RclError { code, msg }), - }, - _ => Err(error), - }, - } + r } /// Get a count of the different kinds of entities in the wait set. From 4b0daaf3e57d77ca3294d0cef69ec02aa81aee84 Mon Sep 17 00:00:00 2001 From: "Michael X. Grey" Date: Sat, 23 Aug 2025 11:04:40 +0800 Subject: [PATCH 5/5] fix formatting Signed-off-by: Michael X. Grey --- rclrs/src/executor.rs | 12 ++++++++++-- rclrs/src/wait_set.rs | 3 +-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/rclrs/src/executor.rs b/rclrs/src/executor.rs index 496571e8e..0d97f985f 100644 --- a/rclrs/src/executor.rs +++ b/rclrs/src/executor.rs @@ -497,12 +497,20 @@ mod tests { fn test_timeout() { let context = Context::default(); let mut executor = context.create_basic_executor(); - let _node = executor.create_node(&format!("test_timeout_{}", line!())).unwrap(); + let _node = executor + .create_node(&format!("test_timeout_{}", line!())) + .unwrap(); for _ in 0..10 { let r = executor.spin(SpinOptions::default().timeout(Duration::from_millis(1))); assert_eq!(r.len(), 1); - assert!(matches!(r[0], RclrsError::RclError { code: RclReturnCode::Timeout, .. })); + assert!(matches!( + r[0], + RclrsError::RclError { + code: RclReturnCode::Timeout, + .. + } + )); } } } diff --git a/rclrs/src/wait_set.rs b/rclrs/src/wait_set.rs index e57005405..90086ae4d 100644 --- a/rclrs/src/wait_set.rs +++ b/rclrs/src/wait_set.rs @@ -123,8 +123,7 @@ impl WaitSet { // the waitable of each primitive to one (and only one) WaitSet when // the primitive gets constructed. The waitables are never allowed to // move between wait sets. - let r = match unsafe { rcl_wait(&mut self.handle.rcl_wait_set, timeout_ns) }.ok() - { + let r = match unsafe { rcl_wait(&mut self.handle.rcl_wait_set, timeout_ns) }.ok() { Ok(_) => Ok(()), Err(error) => match error { RclrsError::RclError { code, msg } => match code {