Update documentation and safety info on rcl entity lifecycles

Signed-off-by: Michael X. Grey <[email protected]>

Author: mxgrey

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<rcl_client_t>,
     node_handle: Arc<NodeHandle>,
@@ -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 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,

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<ContextHandle>,
 }
 
-/// 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(

rclrs/src/node.rs

@@ -68,8 +68,11 @@ pub struct Node {
     pub(crate) handle: Arc<NodeHandle>,
 }
 
-/// 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<rcl_node_t>,
     pub(crate) context_handle: Arc<ContextHandle>,

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,

rclrs/src/publisher.rs

@@ -18,17 +18,23 @@ 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<rcl_publisher_t>,
     node_handle: Arc<NodeHandle>,
 }
 
 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 {

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<rcl_service_t>,
     node_handle: Arc<NodeHandle>,
@@ -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 {

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<rcl_subscription_t>,
     node_handle: Arc<NodeHandle>,
@@ -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 {

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.

rclrs/src/wait/guard_condition.rs

@@ -52,6 +52,11 @@ pub struct GuardCondition {
     pub(crate) in_use_by_wait_set: Arc<AtomicBool>,
 }
 
+/// 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<rcl_guard_condition_t>,
     /// Keep the context alive for the whole lifecycle of the guard condition