diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs index 3e7aa172c..63eec4a32 100644 --- a/rclrs/src/client.rs +++ b/rclrs/src/client.rs @@ -15,7 +15,11 @@ use crate::{rcl_bindings::*, NodeHandle, RclrsError, ENTITY_LIFECYCLE_MUTEX}; // they are running in. Therefore, this type can be safely sent to another thread. unsafe impl Send for rcl_client_t {} -/// Internal struct used by clients. +/// Manage the lifecycle of an [`rcl_client_t`], including managing its dependencies +/// on [`rcl_node_t`] and [`rcl_context_t`] by ensuring that these dependencies are +/// [dropped after][1] the [`rcl_client_t`]. +/// +/// [1] https://doc.rust-lang.org/reference/destructors.html pub struct ClientHandle { rcl_client: Mutex, node_handle: Arc, @@ -33,7 +37,8 @@ impl Drop for ClientHandle { let rcl_client = self.rcl_client.get_mut().unwrap(); let mut rcl_node = self.node_handle.rcl_node.lock().unwrap(); let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); - // SAFETY: No preconditions for this function + // SAFETY: The entity lifecycle mutex is locked to protect against the risk of + // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { rcl_client_fini(rcl_client, &mut *rcl_node); } @@ -93,14 +98,18 @@ where // SAFETY: No preconditions for this function. let client_options = unsafe { rcl_client_get_default_options() }; - unsafe { - // SAFETY: The rcl_client is zero-initialized as expected by this function. - // The rcl_node is kept alive because it is co-owned by the client. - // The topic name and the options are copied by this function, so they can be dropped - // afterwards. - { - let rcl_node = node_handle.rcl_node.lock().unwrap(); - let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); + { + let rcl_node = node_handle.rcl_node.lock().unwrap(); + let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); + + // SAFETY: + // * The rcl_client was zero-initialized as expected by this function. + // * The rcl_node is kept alive by the NodeHandle because it is a dependency of the client. + // * The topic name and the options are copied by this function, so they can be dropped + // afterwards. + // * The entity lifecycle mutex is locked to protect against the risk of global + // variables in the rmw implementation being unsafely modified during initialization. + unsafe { rcl_client_init( &mut rcl_client, &*rcl_node, diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs index 7e69d7d09..17305b0b5 100644 --- a/rclrs/src/context.rs +++ b/rclrs/src/context.rs @@ -19,10 +19,12 @@ impl Drop for rcl_context_t { unsafe { // The context may be invalid when rcl_init failed, e.g. because of invalid command // line arguments. - // SAFETY: No preconditions for this function. + + // SAFETY: No preconditions for rcl_context_is_valid. if rcl_context_is_valid(self) { let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); - // SAFETY: These functions have no preconditions besides a valid rcl_context + // SAFETY: The entity lifecycle mutex is locked to protect against the risk of + // global variables in the rmw implementation being unsafely modified during cleanup. rcl_shutdown(self); rcl_context_fini(self); } @@ -50,10 +52,10 @@ pub struct Context { pub(crate) handle: Arc, } -/// This struct manages the lifetime and access to the rcl context. It will also +/// This struct manages the lifetime and access to the [`rcl_context_t`]. It will also /// account for the lifetimes of any dependencies, if we need to add /// dependencies in the future (currently there are none). It is not strictly -/// necessary to decompose `Context` and `ContextHandle` like this, but we are +/// necessary to decompose [`Context`] and [`ContextHandle`] like this, but we are /// doing it to be consistent with the lifecycle management of other rcl /// bindings in this library. pub(crate) struct ContextHandle { @@ -108,8 +110,11 @@ impl Context { // SAFETY: No preconditions for this function. let allocator = rcutils_get_default_allocator(); let mut rcl_init_options = options.into_rcl(allocator)?; - // SAFETY: This function does not store the ephemeral init_options and c_args - // pointers. Passing in a zero-initialized rcl_context is expected. + // SAFETY: + // * This function does not store the ephemeral init_options and c_args pointers. + // * Passing in a zero-initialized rcl_context is mandatory. + // * The entity lifecycle mutex is locked to protect against the risk of global variables + // in the rmw implementation being unsafely modified during initialization. let ret = { let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); rcl_init( diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs index 857694966..943bd395d 100644 --- a/rclrs/src/node.rs +++ b/rclrs/src/node.rs @@ -68,8 +68,11 @@ pub struct Node { pub(crate) handle: Arc, } -/// This struct manages the lifetime of the rcl node, and accounts for its -/// dependency on the lifetime of its context. +/// This struct manages the lifetime of an [`rcl_node_t`], and accounts for its +/// dependency on the lifetime of its [`rcl_context_t`] by ensuring that this +/// dependency is [dropped after][1] the [`rcl_node_t`]. +/// +/// [1] https://doc.rust-lang.org/reference/destructors.html pub(crate) struct NodeHandle { pub(crate) rcl_node: Mutex, pub(crate) context_handle: Arc, @@ -80,6 +83,8 @@ impl Drop for NodeHandle { let _context_lock = self.context_handle.rcl_context.lock().unwrap(); let mut rcl_node = self.rcl_node.lock().unwrap(); let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); + // SAFETY: The entity lifecycle mutex is locked to protect against the risk of + // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { rcl_node_fini(&mut *rcl_node) }; } } diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs index 27aacd7bb..cdd4ffc53 100644 --- a/rclrs/src/node/builder.rs +++ b/rclrs/src/node/builder.rs @@ -266,10 +266,12 @@ impl NodeBuilder { // SAFETY: Getting a zero-initialized value is always safe. let mut rcl_node = unsafe { rcl_get_zero_initialized_node() }; unsafe { - // SAFETY: The rcl_node is zero-initialized as expected by this function. - // The strings and node options are copied by this function, so we don't need - // to keep them alive. - // The rcl_context has to be kept alive because it is co-owned by the node. + // SAFETY: + // * The rcl_node is zero-initialized as mandated by this function. + // * The strings and node options are copied by this function, so we don't need to keep them alive. + // * The rcl_context is kept alive by the ContextHandle because it is a dependency of the node. + // * The entity lifecycle mutex is locked to protect against the risk of + // global variables in the rmw implementation being unsafely modified during cleanup. let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); rcl_node_init( &mut rcl_node, diff --git a/rclrs/src/publisher.rs b/rclrs/src/publisher.rs index cc0407e14..2178ab0e8 100644 --- a/rclrs/src/publisher.rs +++ b/rclrs/src/publisher.rs @@ -18,6 +18,11 @@ pub use loaned_message::*; // they are running in. Therefore, this type can be safely sent to another thread. unsafe impl Send for rcl_publisher_t {} +/// Manage the lifecycle of an [`rcl_publisher_t`], including managing its dependencies +/// on [`rcl_node_t`] and [`rcl_context_t`] by ensuring that these dependencies are +/// [dropped after][1] the [`rcl_publisher_t`]. +/// +/// [1] https://doc.rust-lang.org/reference/destructors.html struct PublisherHandle { rcl_publisher: Mutex, node_handle: Arc, @@ -25,10 +30,11 @@ struct PublisherHandle { impl Drop for PublisherHandle { fn drop(&mut self) { + let mut rcl_node = self.node_handle.rcl_node.lock().unwrap(); + let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); + // SAFETY: The entity lifecycle mutex is locked to protect against the risk of + // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { - // SAFETY: No preconditions for this function (besides the arguments being valid). - let mut rcl_node = self.node_handle.rcl_node.lock().unwrap(); - let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); rcl_publisher_fini(self.rcl_publisher.get_mut().unwrap(), &mut *rcl_node); } } @@ -89,22 +95,26 @@ where // SAFETY: No preconditions for this function. let mut publisher_options = unsafe { rcl_publisher_get_default_options() }; publisher_options.qos = qos.into(); - unsafe { - // SAFETY: The rcl_publisher is zero-initialized as expected by this function. - // The rcl_node is kept alive because it is co-owned by the subscription. - // The topic name and the options are copied by this function, so they can be dropped - // afterwards. - // TODO: type support? + + { let rcl_node = node_handle.rcl_node.lock().unwrap(); let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); - rcl_publisher_init( - &mut rcl_publisher, - &*rcl_node, - type_support_ptr, - topic_c_string.as_ptr(), - &publisher_options, - ) - .ok()?; + unsafe { + // SAFETY: + // * The rcl_publisher is zero-initialized as mandated by this function. + // * The rcl_node is kept alive by the NodeHandle because it is a dependency of the publisher. + // * The topic name and the options are copied by this function, so they can be dropped afterwards. + // * The entity lifecycle mutex is locked to protect against the risk of global + // variables in the rmw implementation being unsafely modified during cleanup. + rcl_publisher_init( + &mut rcl_publisher, + &*rcl_node, + type_support_ptr, + topic_c_string.as_ptr(), + &publisher_options, + ) + .ok()?; + } } Ok(Self { diff --git a/rclrs/src/service.rs b/rclrs/src/service.rs index cd30445fb..a2c964f6e 100644 --- a/rclrs/src/service.rs +++ b/rclrs/src/service.rs @@ -13,7 +13,11 @@ use crate::{NodeHandle, ENTITY_LIFECYCLE_MUTEX}; // they are running in. Therefore, this type can be safely sent to another thread. unsafe impl Send for rcl_service_t {} -/// Internal struct used by services. +/// Manage the lifecycle of an [`rcl_service_t`], including managing its dependencies +/// on [`rcl_node_t`] and [`rcl_context_t`] by ensuring that these dependencies are +/// [dropped after][1] the [`rcl_service_t`]. +/// +/// [1] https://doc.rust-lang.org/reference/destructors.html pub struct ServiceHandle { rcl_service: Mutex, node_handle: Arc, @@ -31,7 +35,8 @@ impl Drop for ServiceHandle { let rcl_service = self.rcl_service.get_mut().unwrap(); let mut rcl_node = self.node_handle.rcl_node.lock().unwrap(); let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); - // SAFETY: No preconditions for this function + // SAFETY: The entity lifecycle mutex is locked to protect against the risk of + // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { rcl_service_fini(rcl_service, &mut *rcl_node); } @@ -95,21 +100,26 @@ where // SAFETY: No preconditions for this function. let service_options = unsafe { rcl_service_get_default_options() }; - unsafe { - // SAFETY: The rcl_service is zero-initialized as expected by this function. - // The rcl_node is kept alive because it is co-owned by the service. - // The topic name and the options are copied by this function, so they can be dropped - // afterwards. + { let rcl_node = node_handle.rcl_node.lock().unwrap(); let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); - rcl_service_init( - &mut rcl_service, - &*rcl_node, - type_support, - topic_c_string.as_ptr(), - &service_options as *const _, - ) - .ok()?; + unsafe { + // SAFETY: + // * The rcl_service is zero-initialized as mandated by this function. + // * The rcl_node is kept alive by the NodeHandle it is a dependency of the service. + // * The topic name and the options are copied by this function, so they can be dropped + // afterwards. + // * The entity lifecycle mutex is locked to protect against the risk of global + // variables in the rmw implementation being unsafely modified during initialization. + rcl_service_init( + &mut rcl_service, + &*rcl_node, + type_support, + topic_c_string.as_ptr(), + &service_options as *const _, + ) + .ok()?; + } } let handle = Arc::new(ServiceHandle { diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs index 1bf45fded..2e792a657 100644 --- a/rclrs/src/subscription.rs +++ b/rclrs/src/subscription.rs @@ -21,7 +21,11 @@ pub use readonly_loaned_message::*; // they are running in. Therefore, this type can be safely sent to another thread. unsafe impl Send for rcl_subscription_t {} -/// Internal struct used by subscriptions. +/// Manage the lifecycle of an [`rcl_subscription_t`], including managing its dependencies +/// on [`rcl_node_t`] and [`rcl_context_t`] by ensuring that these dependencies are +/// [dropped after][1] the [`rcl_subscription_t`]. +/// +/// [1] https://doc.rust-lang.org/reference/destructors.html pub struct SubscriptionHandle { rcl_subscription: Mutex, node_handle: Arc, @@ -39,7 +43,8 @@ impl Drop for SubscriptionHandle { let rcl_subscription = self.rcl_subscription.get_mut().unwrap(); let mut rcl_node = self.node_handle.rcl_node.lock().unwrap(); let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); - // SAFETY: No preconditions for this function (besides the arguments being valid). + // SAFETY: The entity lifecycle mutex is locked to protect against the risk of + // global variables in the rmw implementation being unsafely modified during cleanup. unsafe { rcl_subscription_fini(rcl_subscription, &mut *rcl_node); } @@ -108,22 +113,26 @@ where // SAFETY: No preconditions for this function. let mut subscription_options = unsafe { rcl_subscription_get_default_options() }; subscription_options.qos = qos.into(); - unsafe { - // SAFETY: The rcl_subscription is zero-initialized as expected by this function. - // The rcl_node is kept alive because it is co-owned by the subscription. - // The topic name and the options are copied by this function, so they can be dropped - // afterwards. - // TODO: type support? + + { let rcl_node = node_handle.rcl_node.lock().unwrap(); let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap(); - rcl_subscription_init( - &mut rcl_subscription, - &*rcl_node, - type_support, - topic_c_string.as_ptr(), - &subscription_options, - ) - .ok()?; + unsafe { + // SAFETY: + // * The rcl_subscription is zero-initialized as mandated by this function. + // * The rcl_node is kept alive by the NodeHandle because it is a dependency of the subscription. + // * The topic name and the options are copied by this function, so they can be dropped afterwards. + // * The entity lifecycle mutex is locked to protect against the risk of global + // variables in the rmw implementation being unsafely modified during cleanup. + rcl_subscription_init( + &mut rcl_subscription, + &*rcl_node, + type_support, + topic_c_string.as_ptr(), + &subscription_options, + ) + .ok()?; + } } let handle = Arc::new(SubscriptionHandle { diff --git a/rclrs/src/wait.rs b/rclrs/src/wait.rs index 959689679..0b6ec7fbc 100644 --- a/rclrs/src/wait.rs +++ b/rclrs/src/wait.rs @@ -28,6 +28,11 @@ mod guard_condition; use exclusivity_guard::*; pub use guard_condition::*; +/// Manage the lifecycle of an [`rcl_wait_set_t`], including managing its dependency +/// on [`rcl_context_t`] by ensuring that this dependency is [dropped after][1] the +/// [`rcl_wait_set_t`]. +/// +/// [1] https://doc.rust-lang.org/reference/destructors.html struct WaitSetHandle { rcl_wait_set: rcl_wait_set_t, // Used to ensure the context is alive while the wait set is alive. diff --git a/rclrs/src/wait/guard_condition.rs b/rclrs/src/wait/guard_condition.rs index 497ddb817..eb3fc3daa 100644 --- a/rclrs/src/wait/guard_condition.rs +++ b/rclrs/src/wait/guard_condition.rs @@ -52,6 +52,11 @@ pub struct GuardCondition { pub(crate) in_use_by_wait_set: Arc, } +/// Manage the lifecycle of an [`rcl_guard_condition_t`], including managing its dependency +/// on [`rcl_context_t`] by ensuring that this dependency is [dropped after][1] the +/// [`rcl_guard_condition_t`]. +/// +/// [1] https://doc.rust-lang.org/reference/destructors.html pub(crate) struct GuardConditionHandle { pub(crate) rcl_guard_condition: Mutex, /// Keep the context alive for the whole lifecycle of the guard condition