From b7dd6a2cc791edc3302fc69e735134ec1ddf8b36 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Wed, 3 Apr 2024 04:21:32 +0000
Subject: [PATCH 01/19] Reworking the lifecycle management of rcl bindings

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/context.rs | 12 +++++++++++-
 rclrs/src/node.rs    | 19 +++++++++++--------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index 7c5d6a763..4346363a2 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -39,7 +39,17 @@ unsafe impl Send for rcl_context_t {}
 /// - the allocator used (left as the default by `rclrs`)
 ///
 pub struct Context {
-    pub(crate) rcl_context_mtx: Arc<Mutex<rcl_context_t>>,
+    pub(crate) handle: Arc<ContextHandle>,
+}
+
+/// This struct manages the lifetime and access to the rcl context. 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
+/// doing it to be consistent with the lifecycle management of other rcl
+/// bindings in this library.
+pub(crate) struct ContextHandle {
+    handle: Mutex<rcl_context_t>,
 }
 
 impl Context {
diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index c89a1ef74..c786c4275 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -15,7 +15,7 @@ use crate::rcl_bindings::*;
 use crate::{
     Client, ClientBase, Clock, Context, GuardCondition, ParameterBuilder, ParameterInterface,
     ParameterVariant, Parameters, Publisher, QoSProfile, RclrsError, Service, ServiceBase,
-    Subscription, SubscriptionBase, SubscriptionCallback, TimeSource, ToResult,
+    Subscription, SubscriptionBase, SubscriptionCallback, TimeSource, ToResult, ContextHandle,
 };
 
 impl Drop for rcl_node_t {
@@ -71,18 +71,21 @@ pub struct Node {
     pub(crate) subscriptions_mtx: Mutex<Vec<Weak<dyn SubscriptionBase>>>,
     time_source: TimeSource,
     parameter: ParameterInterface,
-    // Note: it's important to have those last since `drop` will be called in order of declaration
-    // in the struct and both `TimeSource` and `ParameterInterface` contain subscriptions /
-    // services that will fail to be dropped if the context or node is destroyed first.
-    pub(crate) rcl_node_mtx: Arc<Mutex<rcl_node_t>>,
-    pub(crate) rcl_context_mtx: Arc<Mutex<rcl_context_t>>,
+    pub(crate) rcl_node: Arc<NodeHandle>,
+}
+
+/// This struct manages the lifetime of the rcl node, and accounts for its
+/// dependency on the lifetime of its context.
+pub(crate) struct NodeHandle {
+    handle: Mutex<rcl_node_t>,
+    rcl_context: Arc<ContextHandle>,
 }
 
 impl Eq for Node {}
 
 impl PartialEq for Node {
     fn eq(&self, other: &Self) -> bool {
-        Arc::ptr_eq(&self.rcl_node_mtx, &other.rcl_node_mtx)
+        Arc::ptr_eq(&self.rcl_node, &other.rcl_node)
     }
 }
 
@@ -182,7 +185,7 @@ impl Node {
         &self,
         getter: unsafe extern "C" fn(*const rcl_node_t) -> *const c_char,
     ) -> String {
-        unsafe { call_string_getter_with_handle(&self.rcl_node_mtx.lock().unwrap(), getter) }
+        unsafe { call_string_getter_with_handle(&self.rcl_node.handle.lock().unwrap(), getter) }
     }
 
     /// Creates a [`Client`][1].

From d3df842931fdd5249a6b59fab236a1c43bf068ce Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Wed, 3 Apr 2024 08:00:01 +0000
Subject: [PATCH 02/19] Manage all rcl bindings with Handle structs

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/client.rs                   | 26 ++++++++--------
 rclrs/src/context.rs                  |  8 +++--
 rclrs/src/executor.rs                 |  2 +-
 rclrs/src/node.rs                     | 24 +++++++--------
 rclrs/src/node/builder.rs             | 24 ++++++++-------
 rclrs/src/node/graph.rs               | 14 ++++-----
 rclrs/src/parameter.rs                |  5 ++-
 rclrs/src/parameter/value.rs          |  2 +-
 rclrs/src/publisher.rs                | 31 +++++++++++--------
 rclrs/src/publisher/loaned_message.rs |  4 +--
 rclrs/src/service.rs                  | 19 ++++++------
 rclrs/src/subscription.rs             | 20 ++++++------
 rclrs/src/wait.rs                     | 44 +++++++++++++++------------
 rclrs/src/wait/guard_condition.rs     |  4 +--
 14 files changed, 122 insertions(+), 105 deletions(-)

diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs
index b7db3a9c4..af9f9b5bc 100644
--- a/rclrs/src/client.rs
+++ b/rclrs/src/client.rs
@@ -9,7 +9,7 @@ use rosidl_runtime_rs::Message;
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::MessageCow;
-use crate::{rcl_bindings::*, RclrsError};
+use crate::{rcl_bindings::*, RclrsError, NodeHandle};
 
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
 // they are running in. Therefore, this type can be safely sent to another thread.
@@ -17,24 +17,24 @@ unsafe impl Send for rcl_client_t {}
 
 /// Internal struct used by clients.
 pub struct ClientHandle {
-    rcl_client_mtx: Mutex<rcl_client_t>,
-    rcl_node_mtx: Arc<Mutex<rcl_node_t>>,
+    rcl_client: Mutex<rcl_client_t>,
+    node_handle: Arc<NodeHandle>,
     pub(crate) in_use_by_wait_set: Arc<AtomicBool>,
 }
 
 impl ClientHandle {
     pub(crate) fn lock(&self) -> MutexGuard<rcl_client_t> {
-        self.rcl_client_mtx.lock().unwrap()
+        self.rcl_client.lock().unwrap()
     }
 }
 
 impl Drop for ClientHandle {
     fn drop(&mut self) {
-        let rcl_client = self.rcl_client_mtx.get_mut().unwrap();
-        let rcl_node_mtx = &mut *self.rcl_node_mtx.lock().unwrap();
+        let rcl_client = self.rcl_client.get_mut().unwrap();
+        let rcl_node = &mut *self.node_handle.rcl_node.lock().unwrap();
         // SAFETY: No preconditions for this function
         unsafe {
-            rcl_client_fini(rcl_client, rcl_node_mtx);
+            rcl_client_fini(rcl_client, rcl_node);
         }
     }
 }
@@ -74,7 +74,7 @@ where
     T: rosidl_runtime_rs::Service,
 {
     /// Creates a new client.
-    pub(crate) fn new(rcl_node_mtx: Arc<Mutex<rcl_node_t>>, topic: &str) -> Result<Self, RclrsError>
+    pub(crate) fn new(node_handle: Arc<NodeHandle>, topic: &str) -> Result<Self, RclrsError>
     // This uses pub(crate) visibility to avoid instantiating this struct outside
     // [`Node::create_client`], see the struct's documentation for the rationale
     where
@@ -99,7 +99,7 @@ where
             // afterwards.
             rcl_client_init(
                 &mut rcl_client,
-                &*rcl_node_mtx.lock().unwrap(),
+                &*node_handle.rcl_node.lock().unwrap(),
                 type_support,
                 topic_c_string.as_ptr(),
                 &client_options,
@@ -108,8 +108,8 @@ where
         }
 
         let handle = Arc::new(ClientHandle {
-            rcl_client_mtx: Mutex::new(rcl_client),
-            rcl_node_mtx,
+            rcl_client: Mutex::new(rcl_client),
+            node_handle,
             in_use_by_wait_set: Arc::new(AtomicBool::new(false)),
         });
 
@@ -245,8 +245,8 @@ where
     ///
     pub fn service_is_ready(&self) -> Result<bool, RclrsError> {
         let mut is_ready = false;
-        let client = &mut *self.handle.rcl_client_mtx.lock().unwrap();
-        let node = &mut *self.handle.rcl_node_mtx.lock().unwrap();
+        let client = &mut *self.handle.rcl_client.lock().unwrap();
+        let node = &mut *self.handle.node_handle.rcl_node.lock().unwrap();
 
         unsafe {
             // SAFETY both node and client are guaranteed to be valid here
diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index 4346363a2..2f9ce3c53 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -49,7 +49,7 @@ pub struct Context {
 /// doing it to be consistent with the lifecycle management of other rcl
 /// bindings in this library.
 pub(crate) struct ContextHandle {
-    handle: Mutex<rcl_context_t>,
+    pub(crate) rcl_context: Mutex<rcl_context_t>,
 }
 
 impl Context {
@@ -109,7 +109,9 @@ impl Context {
             ret?;
         }
         Ok(Self {
-            rcl_context_mtx: Arc::new(Mutex::new(rcl_context)),
+            handle: Arc::new(ContextHandle {
+                rcl_context: Mutex::new(rcl_context),
+            })
         })
     }
 
@@ -120,7 +122,7 @@ impl Context {
     pub fn ok(&self) -> bool {
         // This will currently always return true, but once we have a signal handler, the signal
         // handler could call `rcl_shutdown()`, hence making the context invalid.
-        let rcl_context = &mut *self.rcl_context_mtx.lock().unwrap();
+        let rcl_context = &mut *self.handle.rcl_context.lock().unwrap();
         // SAFETY: No preconditions for this function.
         unsafe { rcl_context_is_valid(rcl_context) }
     }
diff --git a/rclrs/src/executor.rs b/rclrs/src/executor.rs
index c9469d7a7..79171e329 100644
--- a/rclrs/src/executor.rs
+++ b/rclrs/src/executor.rs
@@ -42,7 +42,7 @@ impl SingleThreadedExecutor {
         for node in { self.nodes_mtx.lock().unwrap() }
             .iter()
             .filter_map(Weak::upgrade)
-            .filter(|node| unsafe { rcl_context_is_valid(&*node.rcl_context_mtx.lock().unwrap()) })
+            .filter(|node| unsafe { rcl_context_is_valid(&*node.handle.context_handle.rcl_context.lock().unwrap()) })
         {
             let wait_set = WaitSet::new_for_node(&node)?;
             let ready_entities = wait_set.wait(timeout)?;
diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index c786c4275..51df3b4f7 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -71,21 +71,21 @@ pub struct Node {
     pub(crate) subscriptions_mtx: Mutex<Vec<Weak<dyn SubscriptionBase>>>,
     time_source: TimeSource,
     parameter: ParameterInterface,
-    pub(crate) rcl_node: Arc<NodeHandle>,
+    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.
 pub(crate) struct NodeHandle {
-    handle: Mutex<rcl_node_t>,
-    rcl_context: Arc<ContextHandle>,
+    pub(crate) rcl_node: Mutex<rcl_node_t>,
+    pub(crate) context_handle: Arc<ContextHandle>,
 }
 
 impl Eq for Node {}
 
 impl PartialEq for Node {
     fn eq(&self, other: &Self) -> bool {
-        Arc::ptr_eq(&self.rcl_node, &other.rcl_node)
+        Arc::ptr_eq(&self.handle, &other.handle)
     }
 }
 
@@ -185,7 +185,7 @@ impl Node {
         &self,
         getter: unsafe extern "C" fn(*const rcl_node_t) -> *const c_char,
     ) -> String {
-        unsafe { call_string_getter_with_handle(&self.rcl_node.handle.lock().unwrap(), getter) }
+        unsafe { call_string_getter_with_handle(&self.handle.rcl_node.lock().unwrap(), getter) }
     }
 
     /// Creates a [`Client`][1].
@@ -196,7 +196,7 @@ impl Node {
     where
         T: rosidl_runtime_rs::Service,
     {
-        let client = Arc::new(Client::<T>::new(Arc::clone(&self.rcl_node_mtx), topic)?);
+        let client = Arc::new(Client::<T>::new(Arc::clone(&self.handle), topic)?);
         { self.clients_mtx.lock().unwrap() }.push(Arc::downgrade(&client) as Weak<dyn ClientBase>);
         Ok(client)
     }
@@ -212,7 +212,7 @@ impl Node {
     /// [2]: crate::spin_once
     pub fn create_guard_condition(&self) -> Arc<GuardCondition> {
         let guard_condition = Arc::new(GuardCondition::new_with_rcl_context(
-            &mut self.rcl_context_mtx.lock().unwrap(),
+            &mut self.handle.context_handle.rcl_context.lock().unwrap(),
             None,
         ));
         { self.guard_conditions_mtx.lock().unwrap() }
@@ -234,7 +234,7 @@ impl Node {
         F: Fn() + Send + Sync + 'static,
     {
         let guard_condition = Arc::new(GuardCondition::new_with_rcl_context(
-            &mut self.rcl_context_mtx.lock().unwrap(),
+            &mut self.handle.context_handle.rcl_context.lock().unwrap(),
             Some(Box::new(callback) as Box<dyn Fn() + Send + Sync>),
         ));
         { self.guard_conditions_mtx.lock().unwrap() }
@@ -255,7 +255,7 @@ impl Node {
         T: Message,
     {
         let publisher = Arc::new(Publisher::<T>::new(
-            Arc::clone(&self.rcl_node_mtx),
+            Arc::clone(&self.handle),
             topic,
             qos,
         )?);
@@ -276,7 +276,7 @@ impl Node {
         F: Fn(&rmw_request_id_t, T::Request) -> T::Response + 'static + Send,
     {
         let service = Arc::new(Service::<T>::new(
-            Arc::clone(&self.rcl_node_mtx),
+            Arc::clone(&self.handle),
             topic,
             callback,
         )?);
@@ -299,7 +299,7 @@ impl Node {
         T: Message,
     {
         let subscription = Arc::new(Subscription::<T>::new(
-            Arc::clone(&self.rcl_node_mtx),
+            Arc::clone(&self.handle),
             topic,
             qos,
             callback,
@@ -361,7 +361,7 @@ impl Node {
     // add description about this function is for getting actual domain_id
     // and about override of domain_id via node option
     pub fn domain_id(&self) -> usize {
-        let rcl_node = &*self.rcl_node_mtx.lock().unwrap();
+        let rcl_node = &*self.handle.rcl_node.lock().unwrap();
         let mut domain_id: usize = 0;
         let ret = unsafe {
             // SAFETY: No preconditions for this function.
diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 2dd82a79c..5ce52786d 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -3,7 +3,7 @@ use std::sync::{Arc, Mutex};
 
 use crate::rcl_bindings::*;
 use crate::{
-    ClockType, Context, Node, ParameterInterface, QoSProfile, RclrsError, TimeSource, ToResult,
+    ClockType, Context, ContextHandle, Node, NodeHandle, ParameterInterface, QoSProfile, RclrsError, TimeSource, ToResult,
     QOS_PROFILE_CLOCK,
 };
 
@@ -42,7 +42,7 @@ use crate::{
 /// [1]: crate::Node
 /// [2]: crate::Node::builder
 pub struct NodeBuilder {
-    context: Arc<Mutex<rcl_context_t>>,
+    context: Arc<ContextHandle>,
     name: String,
     namespace: String,
     use_global_arguments: bool,
@@ -83,14 +83,14 @@ impl NodeBuilder {
     ///     RclrsError::RclError { code: RclReturnCode::NodeInvalidName, .. }
     /// ));
     /// # Ok::<(), RclrsError>(())
-    /// ```    
-    ///    
+    /// ```
+    ///
     /// [1]: crate::Node#naming
     /// [2]: https://docs.ros2.org/latest/api/rmw/validate__node__name_8h.html#a5690a285aed9735f89ef11950b6e39e3
     /// [3]: NodeBuilder::build
     pub fn new(context: &Context, name: &str) -> NodeBuilder {
         NodeBuilder {
-            context: context.rcl_context_mtx.clone(),
+            context: Arc::clone(&context.handle),
             name: name.to_string(),
             namespace: "/".to_string(),
             use_global_arguments: true,
@@ -193,7 +193,7 @@ impl NodeBuilder {
     /// used in creating the context.
     ///
     /// For more details about command line arguments, see [here][2].
-    ///    
+    ///
     /// # Example
     /// ```
     /// # use rclrs::{Context, Node, NodeBuilder, RclrsError};
@@ -261,7 +261,7 @@ impl NodeBuilder {
                 s: self.namespace.clone(),
             })?;
         let rcl_node_options = self.create_rcl_node_options()?;
-        let rcl_context = &mut *self.context.lock().unwrap();
+        let rcl_context = &mut *self.context.rcl_context.lock().unwrap();
 
         // SAFETY: Getting a zero-initialized value is always safe.
         let mut rcl_node = unsafe { rcl_get_zero_initialized_node() };
@@ -280,15 +280,17 @@ impl NodeBuilder {
             .ok()?;
         };
 
-        let rcl_node_mtx = Arc::new(Mutex::new(rcl_node));
+        let handle = Arc::new(NodeHandle {
+            rcl_node: Mutex::new(rcl_node),
+            context_handle: Arc::clone(&self.context),
+        });
         let parameter = ParameterInterface::new(
-            &rcl_node_mtx,
+            &*handle.rcl_node.lock().unwrap(),
             &rcl_node_options.arguments,
             &rcl_context.global_arguments,
         )?;
         let node = Arc::new(Node {
-            rcl_node_mtx,
-            rcl_context_mtx: self.context.clone(),
+            handle,
             clients_mtx: Mutex::new(vec![]),
             guard_conditions_mtx: Mutex::new(vec![]),
             services_mtx: Mutex::new(vec![]),
diff --git a/rclrs/src/node/graph.rs b/rclrs/src/node/graph.rs
index 968bb5c4b..30fdeabb0 100644
--- a/rclrs/src/node/graph.rs
+++ b/rclrs/src/node/graph.rs
@@ -138,7 +138,7 @@ impl Node {
         // SAFETY: rcl_names_and_types is zero-initialized as expected by this call
         unsafe {
             rcl_get_topic_names_and_types(
-                &*self.rcl_node_mtx.lock().unwrap(),
+                &*self.handle.rcl_node.lock().unwrap(),
                 &mut rcutils_get_default_allocator(),
                 false,
                 &mut rcl_names_and_types,
@@ -167,7 +167,7 @@ impl Node {
         // SAFETY: node_names and node_namespaces are zero-initialized as expected by this call.
         unsafe {
             rcl_get_node_names(
-                &*self.rcl_node_mtx.lock().unwrap(),
+                &*self.handle.rcl_node.lock().unwrap(),
                 rcutils_get_default_allocator(),
                 &mut rcl_names,
                 &mut rcl_namespaces,
@@ -214,7 +214,7 @@ impl Node {
         // SAFETY: The node_names, namespaces, and enclaves are zero-initialized as expected by this call.
         unsafe {
             rcl_get_node_names_with_enclaves(
-                &*self.rcl_node_mtx.lock().unwrap(),
+                &*self.handle.rcl_node.lock().unwrap(),
                 rcutils_get_default_allocator(),
                 &mut rcl_names,
                 &mut rcl_namespaces,
@@ -263,7 +263,7 @@ impl Node {
         // SAFETY: The topic_name string was correctly allocated previously
         unsafe {
             rcl_count_publishers(
-                &*self.rcl_node_mtx.lock().unwrap(),
+                &*self.handle.rcl_node.lock().unwrap(),
                 topic_name.as_ptr(),
                 &mut count,
             )
@@ -283,7 +283,7 @@ impl Node {
         // SAFETY: The topic_name string was correctly allocated previously
         unsafe {
             rcl_count_subscribers(
-                &*self.rcl_node_mtx.lock().unwrap(),
+                &*self.handle.rcl_node.lock().unwrap(),
                 topic_name.as_ptr(),
                 &mut count,
             )
@@ -337,7 +337,7 @@ impl Node {
         // SAFETY: node_name and node_namespace have been zero-initialized.
         unsafe {
             getter(
-                &*self.rcl_node_mtx.lock().unwrap(),
+                &*self.handle.rcl_node.lock().unwrap(),
                 &mut rcutils_get_default_allocator(),
                 node_name.as_ptr(),
                 node_namespace.as_ptr(),
@@ -372,7 +372,7 @@ impl Node {
         // SAFETY: topic has been zero-initialized
         unsafe {
             getter(
-                &*self.rcl_node_mtx.lock().unwrap(),
+                &*self.handle.rcl_node.lock().unwrap(),
                 &mut rcutils_get_default_allocator(),
                 topic.as_ptr(),
                 false,
diff --git a/rclrs/src/parameter.rs b/rclrs/src/parameter.rs
index 75256d869..cbd9c8f3c 100644
--- a/rclrs/src/parameter.rs
+++ b/rclrs/src/parameter.rs
@@ -776,13 +776,12 @@ pub(crate) struct ParameterInterface {
 
 impl ParameterInterface {
     pub(crate) fn new(
-        rcl_node_mtx: &Arc<Mutex<rcl_node_t>>,
+        rcl_node: &rcl_node_t,
         node_arguments: &rcl_arguments_t,
         global_arguments: &rcl_arguments_t,
     ) -> Result<Self, RclrsError> {
-        let rcl_node = rcl_node_mtx.lock().unwrap();
         let override_map = unsafe {
-            let fqn = call_string_getter_with_handle(&rcl_node, rcl_node_get_fully_qualified_name);
+            let fqn = call_string_getter_with_handle(rcl_node, rcl_node_get_fully_qualified_name);
             resolve_parameter_overrides(&fqn, node_arguments, global_arguments)?
         };
 
diff --git a/rclrs/src/parameter/value.rs b/rclrs/src/parameter/value.rs
index 1802145f5..a56e77ffc 100644
--- a/rclrs/src/parameter/value.rs
+++ b/rclrs/src/parameter/value.rs
@@ -433,7 +433,7 @@ mod tests {
             let mut rcl_params = std::ptr::null_mut();
             unsafe {
                 rcl_arguments_get_param_overrides(
-                    &ctx.rcl_context_mtx.lock().unwrap().global_arguments,
+                    &ctx.handle.rcl_context.lock().unwrap().global_arguments,
                     &mut rcl_params,
                 )
                 .ok()?;
diff --git a/rclrs/src/publisher.rs b/rclrs/src/publisher.rs
index 0f512c5f6..f0438cbf2 100644
--- a/rclrs/src/publisher.rs
+++ b/rclrs/src/publisher.rs
@@ -9,6 +9,7 @@ use rosidl_runtime_rs::{Message, RmwMessage};
 use crate::error::{RclrsError, ToResult};
 use crate::qos::QoSProfile;
 use crate::rcl_bindings::*;
+use crate::NodeHandle;
 
 mod loaned_message;
 pub use loaned_message::*;
@@ -17,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 {}
 
+struct PublisherHandle {
+    rcl_publisher: Mutex<rcl_publisher_t>,
+    node_handle: Arc<NodeHandle>,
+}
+
 /// Struct for sending messages of type `T`.
 ///
 /// Multiple publishers can be created for the same topic, in different nodes or the same node.
@@ -31,12 +37,11 @@ pub struct Publisher<T>
 where
     T: Message,
 {
-    rcl_publisher_mtx: Mutex<rcl_publisher_t>,
-    rcl_node_mtx: Arc<Mutex<rcl_node_t>>,
     // The data pointed to by type_support_ptr has static lifetime;
     // it is global data in the type support library.
     type_support_ptr: *const rosidl_message_type_support_t,
     message: PhantomData<T>,
+    handle: PublisherHandle,
 }
 
 impl<T> Drop for Publisher<T>
@@ -47,8 +52,8 @@ where
         unsafe {
             // SAFETY: No preconditions for this function (besides the arguments being valid).
             rcl_publisher_fini(
-                self.rcl_publisher_mtx.get_mut().unwrap(),
-                &mut *self.rcl_node_mtx.lock().unwrap(),
+                self.handle.rcl_publisher.get_mut().unwrap(),
+                &mut *self.handle.node_handle.rcl_node.lock().unwrap(),
             );
         }
     }
@@ -68,8 +73,8 @@ where
     /// Creates a new `Publisher`.
     ///
     /// Node and namespace changes are always applied _before_ topic remapping.
-    pub fn new(
-        rcl_node_mtx: Arc<Mutex<rcl_node_t>>,
+    pub(crate) fn new(
+        node_handle: Arc<NodeHandle>,
         topic: &str,
         qos: QoSProfile,
     ) -> Result<Self, RclrsError>
@@ -96,7 +101,7 @@ where
             // TODO: type support?
             rcl_publisher_init(
                 &mut rcl_publisher,
-                &*rcl_node_mtx.lock().unwrap(),
+                &*node_handle.rcl_node.lock().unwrap(),
                 type_support_ptr,
                 topic_c_string.as_ptr(),
                 &publisher_options,
@@ -105,10 +110,12 @@ where
         }
 
         Ok(Self {
-            rcl_publisher_mtx: Mutex::new(rcl_publisher),
-            rcl_node_mtx,
             type_support_ptr,
             message: PhantomData,
+            handle: PublisherHandle {
+                rcl_publisher: Mutex::new(rcl_publisher),
+                node_handle,
+            }
         })
     }
 
@@ -121,7 +128,7 @@ where
         // The unsafe variables created get converted to safe types before being returned
         unsafe {
             let raw_topic_pointer =
-                rcl_publisher_get_topic_name(&*self.rcl_publisher_mtx.lock().unwrap());
+                rcl_publisher_get_topic_name(&*self.handle.rcl_publisher.lock().unwrap());
             CStr::from_ptr(raw_topic_pointer)
                 .to_string_lossy()
                 .into_owned()
@@ -146,7 +153,7 @@ where
     /// [1]: https://github.com/ros2/ros2/issues/255
     pub fn publish<'a, M: MessageCow<'a, T>>(&self, message: M) -> Result<(), RclrsError> {
         let rmw_message = T::into_rmw_message(message.into_cow());
-        let rcl_publisher = &mut *self.rcl_publisher_mtx.lock().unwrap();
+        let rcl_publisher = &mut *self.handle.rcl_publisher.lock().unwrap();
         unsafe {
             // SAFETY: The message type is guaranteed to match the publisher type by the type system.
             // The message does not need to be valid beyond the duration of this function call.
@@ -200,7 +207,7 @@ where
         unsafe {
             // SAFETY: msg_ptr contains a null ptr as expected by this function.
             rcl_borrow_loaned_message(
-                &*self.rcl_publisher_mtx.lock().unwrap(),
+                &*self.handle.rcl_publisher.lock().unwrap(),
                 self.type_support_ptr,
                 &mut msg_ptr,
             )
diff --git a/rclrs/src/publisher/loaned_message.rs b/rclrs/src/publisher/loaned_message.rs
index 350c1ad23..742b1e7fe 100644
--- a/rclrs/src/publisher/loaned_message.rs
+++ b/rclrs/src/publisher/loaned_message.rs
@@ -55,7 +55,7 @@ where
             unsafe {
                 // SAFETY: These two pointers are valid, and the msg_ptr is not used afterwards.
                 rcl_return_loaned_message_from_publisher(
-                    &*self.publisher.rcl_publisher_mtx.lock().unwrap(),
+                    &*self.publisher.handle.rcl_publisher.lock().unwrap(),
                     self.msg_ptr as *mut _,
                 )
                 .ok()
@@ -80,7 +80,7 @@ where
         unsafe {
             // SAFETY: These two pointers are valid, and the msg_ptr is not used afterwards.
             rcl_publish_loaned_message(
-                &*self.publisher.rcl_publisher_mtx.lock().unwrap(),
+                &*self.publisher.handle.rcl_publisher.lock().unwrap(),
                 self.msg_ptr as *mut _,
                 std::ptr::null_mut(),
             )
diff --git a/rclrs/src/service.rs b/rclrs/src/service.rs
index 6e55094d6..e0d769aad 100644
--- a/rclrs/src/service.rs
+++ b/rclrs/src/service.rs
@@ -7,6 +7,7 @@ use rosidl_runtime_rs::Message;
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::{rcl_bindings::*, MessageCow, RclrsError};
+use crate::NodeHandle;
 
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
 // they are running in. Therefore, this type can be safely sent to another thread.
@@ -14,21 +15,21 @@ unsafe impl Send for rcl_service_t {}
 
 /// Internal struct used by services.
 pub struct ServiceHandle {
-    rcl_service_mtx: Mutex<rcl_service_t>,
-    rcl_node_mtx: Arc<Mutex<rcl_node_t>>,
+    rcl_service: Mutex<rcl_service_t>,
+    node_handle: Arc<NodeHandle>,
     pub(crate) in_use_by_wait_set: Arc<AtomicBool>,
 }
 
 impl ServiceHandle {
     pub(crate) fn lock(&self) -> MutexGuard<rcl_service_t> {
-        self.rcl_service_mtx.lock().unwrap()
+        self.rcl_service.lock().unwrap()
     }
 }
 
 impl Drop for ServiceHandle {
     fn drop(&mut self) {
-        let rcl_service = self.rcl_service_mtx.get_mut().unwrap();
-        let rcl_node = &mut *self.rcl_node_mtx.lock().unwrap();
+        let rcl_service = self.rcl_service.get_mut().unwrap();
+        let rcl_node = &mut *self.node_handle.rcl_node.lock().unwrap();
         // SAFETY: No preconditions for this function
         unsafe {
             rcl_service_fini(rcl_service, rcl_node);
@@ -71,7 +72,7 @@ where
 {
     /// Creates a new service.
     pub(crate) fn new<F>(
-        rcl_node_mtx: Arc<Mutex<rcl_node_t>>,
+        node_handle: Arc<NodeHandle>,
         topic: &str,
         callback: F,
     ) -> Result<Self, RclrsError>
@@ -100,7 +101,7 @@ where
             // afterwards.
             rcl_service_init(
                 &mut rcl_service,
-                &*rcl_node_mtx.lock().unwrap(),
+                &*node_handle.rcl_node.lock().unwrap(),
                 type_support,
                 topic_c_string.as_ptr(),
                 &service_options as *const _,
@@ -109,8 +110,8 @@ where
         }
 
         let handle = Arc::new(ServiceHandle {
-            rcl_service_mtx: Mutex::new(rcl_service),
-            rcl_node_mtx,
+            rcl_service: Mutex::new(rcl_service),
+            node_handle,
             in_use_by_wait_set: Arc::new(AtomicBool::new(false)),
         });
 
diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs
index 7e3439df1..cd235e970 100644
--- a/rclrs/src/subscription.rs
+++ b/rclrs/src/subscription.rs
@@ -8,7 +8,7 @@ use rosidl_runtime_rs::{Message, RmwMessage};
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::qos::QoSProfile;
-use crate::{rcl_bindings::*, RclrsError};
+use crate::{rcl_bindings::*, RclrsError, NodeHandle};
 
 mod callback;
 mod message_info;
@@ -23,21 +23,21 @@ unsafe impl Send for rcl_subscription_t {}
 
 /// Internal struct used by subscriptions.
 pub struct SubscriptionHandle {
-    rcl_subscription_mtx: Mutex<rcl_subscription_t>,
-    rcl_node_mtx: Arc<Mutex<rcl_node_t>>,
+    rcl_subscription: Mutex<rcl_subscription_t>,
+    node_handle: Arc<NodeHandle>,
     pub(crate) in_use_by_wait_set: Arc<AtomicBool>,
 }
 
 impl SubscriptionHandle {
     pub(crate) fn lock(&self) -> MutexGuard<rcl_subscription_t> {
-        self.rcl_subscription_mtx.lock().unwrap()
+        self.rcl_subscription.lock().unwrap()
     }
 }
 
 impl Drop for SubscriptionHandle {
     fn drop(&mut self) {
-        let rcl_subscription = self.rcl_subscription_mtx.get_mut().unwrap();
-        let rcl_node = &mut *self.rcl_node_mtx.lock().unwrap();
+        let rcl_subscription = self.rcl_subscription.get_mut().unwrap();
+        let rcl_node = &mut *self.node_handle.rcl_node.lock().unwrap();
         // SAFETY: No preconditions for this function (besides the arguments being valid).
         unsafe {
             rcl_subscription_fini(rcl_subscription, rcl_node);
@@ -85,7 +85,7 @@ where
 {
     /// Creates a new subscription.
     pub(crate) fn new<Args>(
-        rcl_node_mtx: Arc<Mutex<rcl_node_t>>,
+        rcl_node: Arc<NodeHandle>,
         topic: &str,
         qos: QoSProfile,
         callback: impl SubscriptionCallback<T, Args>,
@@ -115,7 +115,7 @@ where
             // TODO: type support?
             rcl_subscription_init(
                 &mut rcl_subscription,
-                &*rcl_node_mtx.lock().unwrap(),
+                &*rcl_node.rcl_node.lock().unwrap(),
                 type_support,
                 topic_c_string.as_ptr(),
                 &subscription_options,
@@ -124,8 +124,8 @@ where
         }
 
         let handle = Arc::new(SubscriptionHandle {
-            rcl_subscription_mtx: Mutex::new(rcl_subscription),
-            rcl_node_mtx,
+            rcl_subscription: Mutex::new(rcl_subscription),
+            node_handle: rcl_node,
             in_use_by_wait_set: Arc::new(AtomicBool::new(false)),
         });
 
diff --git a/rclrs/src/wait.rs b/rclrs/src/wait.rs
index 94811c75a..a605633e6 100644
--- a/rclrs/src/wait.rs
+++ b/rclrs/src/wait.rs
@@ -15,24 +15,27 @@
 // DISTRIBUTION A. Approved for public release; distribution unlimited.
 // OPSEC #4584.
 
-use std::sync::{Arc, Mutex};
+use std::sync::Arc;
 use std::time::Duration;
 use std::vec::Vec;
 
 use crate::error::{to_rclrs_result, RclReturnCode, RclrsError, ToResult};
 use crate::rcl_bindings::*;
-use crate::{ClientBase, Context, Node, ServiceBase, SubscriptionBase};
+use crate::{ClientBase, Context, Node, ServiceBase, SubscriptionBase, ContextHandle};
 
 mod exclusivity_guard;
 mod guard_condition;
 use exclusivity_guard::*;
 pub use guard_condition::*;
 
-/// A struct for waiting on subscriptions and other waitable entities to become ready.
-pub struct WaitSet {
+struct WaitSetHandle {
     rcl_wait_set: rcl_wait_set_t,
     // Used to ensure the context is alive while the wait set is alive.
-    _rcl_context_mtx: Arc<Mutex<rcl_context_t>>,
+    _rcl_context_mtx: Arc<ContextHandle>,
+}
+
+/// A struct for waiting on subscriptions and other waitable entities to become ready.
+pub struct WaitSet {
     // The subscriptions that are currently registered in the wait set.
     // This correspondence is an invariant that must be maintained by all functions,
     // even in the error case.
@@ -41,6 +44,7 @@ pub struct WaitSet {
     // The guard conditions that are currently registered in the wait set.
     guard_conditions: Vec<ExclusivityGuard<Arc<GuardCondition>>>,
     services: Vec<ExclusivityGuard<Arc<dyn ServiceBase>>>,
+    handle: WaitSetHandle,
 }
 
 /// A list of entities that are ready, returned by [`WaitSet::wait`].
@@ -101,19 +105,21 @@ impl WaitSet {
                 number_of_clients,
                 number_of_services,
                 number_of_events,
-                &mut *context.rcl_context_mtx.lock().unwrap(),
+                &mut *context.handle.rcl_context.lock().unwrap(),
                 rcutils_get_default_allocator(),
             )
             .ok()?;
             rcl_wait_set
         };
         Ok(Self {
-            rcl_wait_set,
-            _rcl_context_mtx: context.rcl_context_mtx.clone(),
             subscriptions: Vec::new(),
             guard_conditions: Vec::new(),
             clients: Vec::new(),
             services: Vec::new(),
+            handle: WaitSetHandle {
+                rcl_wait_set,
+                _rcl_context_mtx: Arc::clone(&context.handle),
+            },
         })
     }
 
@@ -126,7 +132,7 @@ impl WaitSet {
         let live_guard_conditions = node.live_guard_conditions();
         let live_services = node.live_services();
         let ctx = Context {
-            rcl_context_mtx: node.rcl_context_mtx.clone(),
+            handle: Arc::clone(&node.handle.context_handle),
         };
         let mut wait_set = WaitSet::new(
             live_subscriptions.len(),
@@ -169,7 +175,7 @@ impl WaitSet {
         // valid, which it always is in our case. Hence, only debug_assert instead of returning
         // Result.
         // SAFETY: No preconditions for this function (besides passing in a valid wait set).
-        let ret = unsafe { rcl_wait_set_clear(&mut self.rcl_wait_set) };
+        let ret = unsafe { rcl_wait_set_clear(&mut self.handle.rcl_wait_set) };
         debug_assert_eq!(ret, 0);
     }
 
@@ -196,7 +202,7 @@ impl WaitSet {
             // for as long as the wait set exists, because it's stored in self.subscriptions.
             // Passing in a null pointer for the third argument is explicitly allowed.
             rcl_wait_set_add_subscription(
-                &mut self.rcl_wait_set,
+                &mut self.handle.rcl_wait_set,
                 &*subscription.handle().lock(),
                 std::ptr::null_mut(),
             )
@@ -228,7 +234,7 @@ impl WaitSet {
         unsafe {
             // SAFETY: Safe if the wait set and guard condition are initialized
             rcl_wait_set_add_guard_condition(
-                &mut self.rcl_wait_set,
+                &mut self.handle.rcl_wait_set,
                 &*guard_condition.rcl_guard_condition.lock().unwrap(),
                 std::ptr::null_mut(),
             )
@@ -258,7 +264,7 @@ impl WaitSet {
             // for as long as the wait set exists, because it's stored in self.clients.
             // Passing in a null pointer for the third argument is explicitly allowed.
             rcl_wait_set_add_client(
-                &mut self.rcl_wait_set,
+                &mut self.handle.rcl_wait_set,
                 &*client.handle().lock() as *const _,
                 core::ptr::null_mut(),
             )
@@ -288,7 +294,7 @@ impl WaitSet {
             // for as long as the wait set exists, because it's stored in self.services.
             // Passing in a null pointer for the third argument is explicitly allowed.
             rcl_wait_set_add_service(
-                &mut self.rcl_wait_set,
+                &mut self.handle.rcl_wait_set,
                 &*service.handle().lock() as *const _,
                 core::ptr::null_mut(),
             )
@@ -337,7 +343,7 @@ impl WaitSet {
         // 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.
-        match unsafe { rcl_wait(&mut self.rcl_wait_set, timeout_ns) }.ok() {
+        match unsafe { rcl_wait(&mut self.handle.rcl_wait_set, timeout_ns) }.ok() {
             Ok(_) => (),
             Err(error) => match error {
                 RclrsError::RclError { code, msg } => match code {
@@ -357,7 +363,7 @@ impl WaitSet {
             // SAFETY: The `subscriptions` entry is an array of pointers, and this dereferencing is
             // equivalent to
             // https://github.com/ros2/rcl/blob/35a31b00a12f259d492bf53c0701003bd7f1745c/rcl/include/rcl/wait.h#L419
-            let wait_set_entry = unsafe { *self.rcl_wait_set.subscriptions.add(i) };
+            let wait_set_entry = unsafe { *self.handle.rcl_wait_set.subscriptions.add(i) };
             if !wait_set_entry.is_null() {
                 ready_entities
                     .subscriptions
@@ -369,7 +375,7 @@ impl WaitSet {
             // SAFETY: The `clients` entry is an array of pointers, and this dereferencing is
             // equivalent to
             // https://github.com/ros2/rcl/blob/35a31b00a12f259d492bf53c0701003bd7f1745c/rcl/include/rcl/wait.h#L419
-            let wait_set_entry = unsafe { *self.rcl_wait_set.clients.add(i) };
+            let wait_set_entry = unsafe { *self.handle.rcl_wait_set.clients.add(i) };
             if !wait_set_entry.is_null() {
                 ready_entities.clients.push(Arc::clone(&client.waitable));
             }
@@ -379,7 +385,7 @@ impl WaitSet {
             // SAFETY: The `clients` entry is an array of pointers, and this dereferencing is
             // equivalent to
             // https://github.com/ros2/rcl/blob/35a31b00a12f259d492bf53c0701003bd7f1745c/rcl/include/rcl/wait.h#L419
-            let wait_set_entry = unsafe { *self.rcl_wait_set.guard_conditions.add(i) };
+            let wait_set_entry = unsafe { *self.handle.rcl_wait_set.guard_conditions.add(i) };
             if !wait_set_entry.is_null() {
                 ready_entities
                     .guard_conditions
@@ -391,7 +397,7 @@ impl WaitSet {
             // SAFETY: The `services` entry is an array of pointers, and this dereferencing is
             // equivalent to
             // https://github.com/ros2/rcl/blob/35a31b00a12f259d492bf53c0701003bd7f1745c/rcl/include/rcl/wait.h#L419
-            let wait_set_entry = unsafe { *self.rcl_wait_set.services.add(i) };
+            let wait_set_entry = unsafe { *self.handle.rcl_wait_set.services.add(i) };
             if !wait_set_entry.is_null() {
                 ready_entities.services.push(Arc::clone(&service.waitable));
             }
diff --git a/rclrs/src/wait/guard_condition.rs b/rclrs/src/wait/guard_condition.rs
index f4d9f651d..7de76c8be 100644
--- a/rclrs/src/wait/guard_condition.rs
+++ b/rclrs/src/wait/guard_condition.rs
@@ -80,7 +80,7 @@ unsafe impl Send for rcl_guard_condition_t {}
 impl GuardCondition {
     /// Creates a new guard condition with no callback.
     pub fn new(context: &Context) -> Self {
-        Self::new_with_rcl_context(&mut context.rcl_context_mtx.lock().unwrap(), None)
+        Self::new_with_rcl_context(&mut context.handle.rcl_context.lock().unwrap(), None)
     }
 
     /// Creates a new guard condition with a callback.
@@ -89,7 +89,7 @@ impl GuardCondition {
         F: Fn() + Send + Sync + 'static,
     {
         Self::new_with_rcl_context(
-            &mut context.rcl_context_mtx.lock().unwrap(),
+            &mut context.handle.rcl_context.lock().unwrap(),
             Some(Box::new(callback) as Box<dyn Fn() + Send + Sync>),
         )
     }

From 10b2bcb3b0b0fa2dbc142a823f9ec2bfe8ddc652 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Wed, 3 Apr 2024 08:34:29 +0000
Subject: [PATCH 03/19] Keep context alive for guard conditions

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/node.rs                 |  8 ++---
 rclrs/src/wait.rs                 |  7 ++--
 rclrs/src/wait/guard_condition.rs | 59 +++++++++++++++++++------------
 3 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index 51df3b4f7..6390b09ea 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -211,8 +211,8 @@ impl Node {
     /// [1]: crate::GuardCondition
     /// [2]: crate::spin_once
     pub fn create_guard_condition(&self) -> Arc<GuardCondition> {
-        let guard_condition = Arc::new(GuardCondition::new_with_rcl_context(
-            &mut self.handle.context_handle.rcl_context.lock().unwrap(),
+        let guard_condition = Arc::new(GuardCondition::new_with_context_handle(
+            Arc::clone(&self.handle.context_handle),
             None,
         ));
         { self.guard_conditions_mtx.lock().unwrap() }
@@ -233,8 +233,8 @@ impl Node {
     where
         F: Fn() + Send + Sync + 'static,
     {
-        let guard_condition = Arc::new(GuardCondition::new_with_rcl_context(
-            &mut self.handle.context_handle.rcl_context.lock().unwrap(),
+        let guard_condition = Arc::new(GuardCondition::new_with_context_handle(
+            Arc::clone(&self.handle.context_handle),
             Some(Box::new(callback) as Box<dyn Fn() + Send + Sync>),
         ));
         { self.guard_conditions_mtx.lock().unwrap() }
diff --git a/rclrs/src/wait.rs b/rclrs/src/wait.rs
index a605633e6..56a2c52bf 100644
--- a/rclrs/src/wait.rs
+++ b/rclrs/src/wait.rs
@@ -31,7 +31,8 @@ pub use guard_condition::*;
 struct WaitSetHandle {
     rcl_wait_set: rcl_wait_set_t,
     // Used to ensure the context is alive while the wait set is alive.
-    _rcl_context_mtx: Arc<ContextHandle>,
+    #[allow(dead_code)]
+    context_handle: Arc<ContextHandle>,
 }
 
 /// A struct for waiting on subscriptions and other waitable entities to become ready.
@@ -118,7 +119,7 @@ impl WaitSet {
             services: Vec::new(),
             handle: WaitSetHandle {
                 rcl_wait_set,
-                _rcl_context_mtx: Arc::clone(&context.handle),
+                context_handle: Arc::clone(&context.handle),
             },
         })
     }
@@ -235,7 +236,7 @@ impl WaitSet {
             // SAFETY: Safe if the wait set and guard condition are initialized
             rcl_wait_set_add_guard_condition(
                 &mut self.handle.rcl_wait_set,
-                &*guard_condition.rcl_guard_condition.lock().unwrap(),
+                &*guard_condition.handle.rcl_guard_condition.lock().unwrap(),
                 std::ptr::null_mut(),
             )
             .ok()?;
diff --git a/rclrs/src/wait/guard_condition.rs b/rclrs/src/wait/guard_condition.rs
index 7de76c8be..b3c7001a7 100644
--- a/rclrs/src/wait/guard_condition.rs
+++ b/rclrs/src/wait/guard_condition.rs
@@ -1,7 +1,7 @@
 use std::sync::{atomic::AtomicBool, Arc, Mutex};
 
 use crate::rcl_bindings::*;
-use crate::{Context, RclrsError, ToResult};
+use crate::{Context, ContextHandle, RclrsError, ToResult};
 
 /// A waitable entity used for waking up a wait set manually.
 ///
@@ -45,18 +45,25 @@ use crate::{Context, RclrsError, ToResult};
 /// ```
 pub struct GuardCondition {
     /// The rcl_guard_condition_t that this struct encapsulates.
-    pub(crate) rcl_guard_condition: Mutex<rcl_guard_condition_t>,
+    pub(crate) handle: GuardConditionHandle,
     /// An optional callback to call when this guard condition is triggered.
     callback: Option<Box<dyn Fn() + Send + Sync>>,
     /// A flag to indicate if this guard condition has already been assigned to a wait set.
     pub(crate) in_use_by_wait_set: Arc<AtomicBool>,
 }
 
+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
+    #[allow(dead_code)]
+    pub(crate) context_handle: Arc<ContextHandle>,
+}
+
 impl Drop for GuardCondition {
     fn drop(&mut self) {
         unsafe {
             // SAFETY: No precondition for this function (besides passing in a valid guard condition)
-            rcl_guard_condition_fini(&mut *self.rcl_guard_condition.lock().unwrap());
+            rcl_guard_condition_fini(&mut *self.handle.rcl_guard_condition.lock().unwrap());
         }
     }
 }
@@ -66,8 +73,8 @@ impl PartialEq for GuardCondition {
         // Because GuardCondition controls the creation of the rcl_guard_condition, each unique GuardCondition should have a unique
         // rcl_guard_condition. Thus comparing equality of this member should be enough.
         std::ptr::eq(
-            &self.rcl_guard_condition.lock().unwrap().impl_,
-            &other.rcl_guard_condition.lock().unwrap().impl_,
+            &self.handle.rcl_guard_condition.lock().unwrap().impl_,
+            &other.handle.rcl_guard_condition.lock().unwrap().impl_,
         )
     }
 }
@@ -80,7 +87,7 @@ unsafe impl Send for rcl_guard_condition_t {}
 impl GuardCondition {
     /// Creates a new guard condition with no callback.
     pub fn new(context: &Context) -> Self {
-        Self::new_with_rcl_context(&mut context.handle.rcl_context.lock().unwrap(), None)
+        Self::new_with_context_handle(Arc::clone(&context.handle), None)
     }
 
     /// Creates a new guard condition with a callback.
@@ -88,8 +95,8 @@ impl GuardCondition {
     where
         F: Fn() + Send + Sync + 'static,
     {
-        Self::new_with_rcl_context(
-            &mut context.handle.rcl_context.lock().unwrap(),
+        Self::new_with_context_handle(
+            Arc::clone(&context.handle),
             Some(Box::new(callback) as Box<dyn Fn() + Send + Sync>),
         )
     }
@@ -98,23 +105,31 @@ impl GuardCondition {
     /// Note this function enables calling `Node::create_guard_condition`[1] without providing the Context separately
     ///
     /// [1]: Node::create_guard_condition
-    pub(crate) fn new_with_rcl_context(
-        context: &mut rcl_context_t,
+    pub(crate) fn new_with_context_handle(
+        context_handle: Arc<ContextHandle>,
         callback: Option<Box<dyn Fn() + Send + Sync>>,
     ) -> Self {
-        // SAFETY: Getting a zero initialized value is always safe
-        let mut guard_condition = unsafe { rcl_get_zero_initialized_guard_condition() };
-        unsafe {
-            // SAFETY: The context must be valid, and the guard condition must be zero-initialized
-            rcl_guard_condition_init(
-                &mut guard_condition,
-                context,
-                rcl_guard_condition_get_default_options(),
-            );
-        }
+        let rcl_guard_condition = {
+            let mut rcl_context = context_handle.rcl_context.lock().unwrap();
+            // SAFETY: Getting a zero initialized value is always safe
+            let mut guard_condition = unsafe { rcl_get_zero_initialized_guard_condition() };
+            unsafe {
+                // SAFETY: The context must be valid, and the guard condition must be zero-initialized
+                rcl_guard_condition_init(
+                    &mut guard_condition,
+                    &mut *rcl_context,
+                    rcl_guard_condition_get_default_options(),
+                );
+            }
+
+            Mutex::new(guard_condition)
+        };
 
         Self {
-            rcl_guard_condition: Mutex::new(guard_condition),
+            handle: GuardConditionHandle {
+                rcl_guard_condition,
+                context_handle,
+            },
             callback,
             in_use_by_wait_set: Arc::new(AtomicBool::new(false)),
         }
@@ -124,7 +139,7 @@ impl GuardCondition {
     pub fn trigger(&self) -> Result<(), RclrsError> {
         unsafe {
             // SAFETY: The rcl_guard_condition_t is valid.
-            rcl_trigger_guard_condition(&mut *self.rcl_guard_condition.lock().unwrap()).ok()?;
+            rcl_trigger_guard_condition(&mut *self.handle.rcl_guard_condition.lock().unwrap()).ok()?;
         }
         if let Some(callback) = &self.callback {
             callback();

From 81c065e1672529cc2b2107caab222c65dfe734ee Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Wed, 3 Apr 2024 16:15:42 +0000
Subject: [PATCH 04/19] Introduce InitOptions to allow manually setting domain
 ID

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/context.rs | 97 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 90 insertions(+), 7 deletions(-)

diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index 2f9ce3c53..485df1f2c 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -55,10 +55,10 @@ pub(crate) struct ContextHandle {
 impl Context {
     /// Creates a new context.
     ///
-    /// Usually, this would be called with `std::env::args()`, analogously to `rclcpp::init()`.
+    /// Usually this would be called with `std::env::args()`, analogously to `rclcpp::init()`.
     /// See also the official "Passing ROS arguments to nodes via the command-line" tutorial.
     ///
-    /// Creating a context can fail in case the args contain invalid ROS arguments.
+    /// Creating a context will fail if the args contain invalid ROS arguments.
     ///
     /// # Example
     /// ```
@@ -68,6 +68,21 @@ impl Context {
     /// assert!(Context::new(invalid_remapping).is_err());
     /// ```
     pub fn new(args: impl IntoIterator<Item = String>) -> Result<Self, RclrsError> {
+        Self::new_with_options(args, InitOptions::new())
+    }
+
+    /// Same as [`Context::new`] except you can additionally provide initialization options.
+    ///
+    /// # Example
+    /// ```
+    /// use rclrs::{Context, InitOptions};
+    /// let context = Context::new_with_options([], InitOptions::new().with_domain_id(Some(5))).unwrap();
+    /// assert_eq!(context.domain_id(), 5);
+    /// ````
+    pub fn new_with_options(
+        args: impl IntoIterator<Item=String>,
+        options: InitOptions,
+    ) -> Result<Self, RclrsError> {
         // SAFETY: Getting a zero-initialized value is always safe
         let mut rcl_context = unsafe { rcl_get_zero_initialized_context() };
         let cstring_args: Vec<CString> = args
@@ -84,11 +99,7 @@ impl Context {
         unsafe {
             // SAFETY: No preconditions for this function.
             let allocator = rcutils_get_default_allocator();
-            // SAFETY: Getting a zero-initialized value is always safe.
-            let mut rcl_init_options = rcl_get_zero_initialized_init_options();
-            // SAFETY: Passing in a zero-initialized value is expected.
-            // In the case where this returns not ok, there's nothing to clean up.
-            rcl_init_options_init(&mut rcl_init_options, allocator).ok()?;
+            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.
             let ret = rcl_init(
@@ -115,6 +126,25 @@ impl Context {
         })
     }
 
+    /// Returns the ROS domain ID that the context is using.
+    ///
+    /// The domain ID controls which nodes can send messages to each other, see the [ROS 2 concept article][1].
+    /// It can be set through the `ROS_DOMAIN_ID` environment variable.
+    ///
+    /// [1]: https://docs.ros.org/en/rolling/Concepts/About-Domain-ID.html
+    pub fn domain_id(&self) -> u8 {
+        let mut domain_id: usize = 0;
+        let ret = unsafe {
+            rcl_context_get_domain_id(
+                &mut *self.handle.rcl_context.lock().unwrap(),
+                &mut domain_id
+            )
+        };
+
+        debug_assert_eq!(ret, 0);
+        domain_id as u8
+    }
+
     /// Checks if the context is still valid.
     ///
     /// This will return `false` when a signal has caused the context to shut down (currently
@@ -128,6 +158,59 @@ impl Context {
     }
 }
 
+/// Additional options for initializing the Context.
+#[derive(Default, Clone)]
+pub struct InitOptions {
+    /// The domain ID that should be used by the Context. Set to None to ask for
+    /// the default behavior, which is to set the domain ID according to the
+    /// [ROS_DOMAIN_ID][1] environment variable.
+    ///
+    /// [1]: https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Domain-ID.html#the-ros-domain-id
+    domain_id: Option<u8>,
+}
+
+impl InitOptions {
+    /// Create a new InitOptions with all default values.
+    pub fn new() -> InitOptions {
+        Self::default()
+    }
+
+    /// Transform an InitOptions into a new one with a certain domain_id
+    pub fn with_domain_id(mut self, domain_id: Option<u8>) -> InitOptions {
+        self.domain_id = domain_id;
+        self
+    }
+
+    /// Set the domain_id of an InitOptions, or reset it to the default behavior
+    /// (determined by environment variables) by providing None.
+    pub fn set_domain_id(&mut self, domain_id: Option<u8>) {
+        self.domain_id = domain_id;
+    }
+
+    /// Get the domain_id that will be provided by these InitOptions.
+    pub fn domain_id(&self) -> Option<u8> {
+        self.domain_id
+    }
+
+    fn into_rcl(self, allocator: rcutils_allocator_s) -> Result<rcl_init_options_t, RclrsError> {
+        unsafe {
+            // SAFETY: Getting a zero-initialized value is always safe.
+            let mut rcl_init_options = rcl_get_zero_initialized_init_options();
+            // SAFETY: Passing in a zero-initialized value is expected.
+            // In the case where this returns not ok, there's nothing to clean up.
+            rcl_init_options_init(&mut rcl_init_options, allocator).ok()?;
+
+            // We only need to set the domain_id if the user asked for something
+            // other than None. When the user asks for None, that is equivalent
+            // to the default value in rcl_init_options.
+            if let Some(domain_id) = self.domain_id {
+                rcl_init_options_set_domain_id(&mut rcl_init_options, domain_id as usize);
+            }
+            return Ok(rcl_init_options);
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

From f30a284e5f8cdf110c4c016b5cfdf52a006608b8 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Thu, 4 Apr 2024 01:55:14 +0000
Subject: [PATCH 05/19] Ensure that mutex guards are not being dropped
 prematurely

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/client.rs       | 23 +++++++++++++----------
 rclrs/src/node.rs         | 24 +++++++++++++-----------
 rclrs/src/node/builder.rs | 13 ++++++++-----
 rclrs/src/node/graph.rs   | 29 +++++++++++++++++++----------
 rclrs/src/publisher.rs    | 31 +++++++++++++++----------------
 rclrs/src/service.rs      |  7 ++++---
 rclrs/src/subscription.rs | 11 ++++++-----
 7 files changed, 78 insertions(+), 60 deletions(-)

diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs
index af9f9b5bc..140aeafb3 100644
--- a/rclrs/src/client.rs
+++ b/rclrs/src/client.rs
@@ -31,10 +31,10 @@ impl ClientHandle {
 impl Drop for ClientHandle {
     fn drop(&mut self) {
         let rcl_client = self.rcl_client.get_mut().unwrap();
-        let rcl_node = &mut *self.node_handle.rcl_node.lock().unwrap();
+        let mut rcl_node = self.node_handle.rcl_node.lock().unwrap();
         // SAFETY: No preconditions for this function
         unsafe {
-            rcl_client_fini(rcl_client, rcl_node);
+            rcl_client_fini(rcl_client, &mut *rcl_node);
         }
     }
 }
@@ -97,14 +97,17 @@ where
             // 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.
-            rcl_client_init(
-                &mut rcl_client,
-                &*node_handle.rcl_node.lock().unwrap(),
-                type_support,
-                topic_c_string.as_ptr(),
-                &client_options,
-            )
-            .ok()?;
+            {
+                let mut rcl_node = node_handle.rcl_node.lock().unwrap();
+                rcl_client_init(
+                    &mut rcl_client,
+                    &mut *rcl_node,
+                    type_support,
+                    topic_c_string.as_ptr(),
+                    &client_options,
+                )
+                .ok()?;
+            }
         }
 
         let handle = Arc::new(ClientHandle {
diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index 6390b09ea..386c36588 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -15,16 +15,9 @@ use crate::rcl_bindings::*;
 use crate::{
     Client, ClientBase, Clock, Context, GuardCondition, ParameterBuilder, ParameterInterface,
     ParameterVariant, Parameters, Publisher, QoSProfile, RclrsError, Service, ServiceBase,
-    Subscription, SubscriptionBase, SubscriptionCallback, TimeSource, ToResult, ContextHandle,
+    Subscription, SubscriptionBase, SubscriptionCallback, TimeSource, ContextHandle,
 };
 
-impl Drop for rcl_node_t {
-    fn drop(&mut self) {
-        // SAFETY: No preconditions for this function
-        unsafe { rcl_node_fini(self).ok().unwrap() };
-    }
-}
-
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
 // they are running in. Therefore, this type can be safely sent to another thread.
 unsafe impl Send for rcl_node_t {}
@@ -81,6 +74,14 @@ pub(crate) struct NodeHandle {
     pub(crate) context_handle: Arc<ContextHandle>,
 }
 
+impl Drop for NodeHandle {
+    fn drop(&mut self) {
+        let _context_lock = self.context_handle.rcl_context.lock().unwrap();
+        let mut rcl_node = self.rcl_node.lock().unwrap();
+        unsafe { rcl_node_fini(&mut *rcl_node) };
+    }
+}
+
 impl Eq for Node {}
 
 impl PartialEq for Node {
@@ -185,7 +186,8 @@ impl Node {
         &self,
         getter: unsafe extern "C" fn(*const rcl_node_t) -> *const c_char,
     ) -> String {
-        unsafe { call_string_getter_with_handle(&self.handle.rcl_node.lock().unwrap(), getter) }
+        let rcl_node = self.handle.rcl_node.lock().unwrap();
+        unsafe { call_string_getter_with_handle(&rcl_node, getter) }
     }
 
     /// Creates a [`Client`][1].
@@ -361,11 +363,11 @@ impl Node {
     // add description about this function is for getting actual domain_id
     // and about override of domain_id via node option
     pub fn domain_id(&self) -> usize {
-        let rcl_node = &*self.handle.rcl_node.lock().unwrap();
+        let rcl_node = self.handle.rcl_node.lock().unwrap();
         let mut domain_id: usize = 0;
         let ret = unsafe {
             // SAFETY: No preconditions for this function.
-            rcl_node_get_domain_id(rcl_node, &mut domain_id)
+            rcl_node_get_domain_id(&*rcl_node, &mut domain_id)
         };
 
         debug_assert_eq!(ret, 0);
diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 5ce52786d..5fe267495 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -284,11 +284,14 @@ impl NodeBuilder {
             rcl_node: Mutex::new(rcl_node),
             context_handle: Arc::clone(&self.context),
         });
-        let parameter = ParameterInterface::new(
-            &*handle.rcl_node.lock().unwrap(),
-            &rcl_node_options.arguments,
-            &rcl_context.global_arguments,
-        )?;
+        let parameter = {
+            let rcl_node = handle.rcl_node.lock().unwrap();
+            ParameterInterface::new(
+                &*rcl_node,
+                &rcl_node_options.arguments,
+                &rcl_context.global_arguments,
+            )?
+        };
         let node = Arc::new(Node {
             handle,
             clients_mtx: Mutex::new(vec![]),
diff --git a/rclrs/src/node/graph.rs b/rclrs/src/node/graph.rs
index 30fdeabb0..3190c1926 100644
--- a/rclrs/src/node/graph.rs
+++ b/rclrs/src/node/graph.rs
@@ -137,8 +137,9 @@ impl Node {
 
         // SAFETY: rcl_names_and_types is zero-initialized as expected by this call
         unsafe {
+            let rcl_node = self.handle.rcl_node.lock().unwrap();
             rcl_get_topic_names_and_types(
-                &*self.handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 &mut rcutils_get_default_allocator(),
                 false,
                 &mut rcl_names_and_types,
@@ -166,8 +167,9 @@ impl Node {
 
         // SAFETY: node_names and node_namespaces are zero-initialized as expected by this call.
         unsafe {
+            let rcl_node = self.handle.rcl_node.lock().unwrap();
             rcl_get_node_names(
-                &*self.handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 rcutils_get_default_allocator(),
                 &mut rcl_names,
                 &mut rcl_namespaces,
@@ -213,8 +215,9 @@ impl Node {
 
         // SAFETY: The node_names, namespaces, and enclaves are zero-initialized as expected by this call.
         unsafe {
+            let rcl_node = self.handle.rcl_node.lock().unwrap();
             rcl_get_node_names_with_enclaves(
-                &*self.handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 rcutils_get_default_allocator(),
                 &mut rcl_names,
                 &mut rcl_namespaces,
@@ -262,8 +265,9 @@ impl Node {
 
         // SAFETY: The topic_name string was correctly allocated previously
         unsafe {
+            let rcl_node = self.handle.rcl_node.lock().unwrap();
             rcl_count_publishers(
-                &*self.handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 topic_name.as_ptr(),
                 &mut count,
             )
@@ -282,8 +286,9 @@ impl Node {
 
         // SAFETY: The topic_name string was correctly allocated previously
         unsafe {
+            let rcl_node = self.handle.rcl_node.lock().unwrap();
             rcl_count_subscribers(
-                &*self.handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 topic_name.as_ptr(),
                 &mut count,
             )
@@ -336,8 +341,9 @@ impl Node {
 
         // SAFETY: node_name and node_namespace have been zero-initialized.
         unsafe {
+            let rcl_node = self.handle.rcl_node.lock().unwrap();
             getter(
-                &*self.handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 &mut rcutils_get_default_allocator(),
                 node_name.as_ptr(),
                 node_namespace.as_ptr(),
@@ -371,8 +377,9 @@ impl Node {
 
         // SAFETY: topic has been zero-initialized
         unsafe {
+            let rcl_node = self.handle.rcl_node.lock().unwrap();
             getter(
-                &*self.handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 &mut rcutils_get_default_allocator(),
                 topic.as_ptr(),
                 false,
@@ -458,14 +465,16 @@ fn convert_names_and_types(
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::Context;
+    use crate::{Context, InitOptions};
 
     #[test]
     fn test_graph_empty() {
-        let context = Context::new([]).unwrap();
+        let context = Context::new_with_options(
+            [],
+            InitOptions::new().with_domain_id(Some(99))
+        ).unwrap();
         let node_name = "test_publisher_names_and_types";
         let node = Node::new(&context, node_name).unwrap();
-
         // Test that the graph has no publishers
         let names_and_topics = node
             .get_publisher_names_and_types_by_node(node_name, "")
diff --git a/rclrs/src/publisher.rs b/rclrs/src/publisher.rs
index f0438cbf2..390894d24 100644
--- a/rclrs/src/publisher.rs
+++ b/rclrs/src/publisher.rs
@@ -23,6 +23,19 @@ struct PublisherHandle {
     node_handle: Arc<NodeHandle>,
 }
 
+impl Drop for PublisherHandle {
+    fn drop(&mut self) {
+        unsafe {
+            // SAFETY: No preconditions for this function (besides the arguments being valid).
+            let mut rcl_node = self.node_handle.rcl_node.lock().unwrap();
+            rcl_publisher_fini(
+                self.rcl_publisher.get_mut().unwrap(),
+                &mut *rcl_node,
+            );
+        }
+    }
+}
+
 /// Struct for sending messages of type `T`.
 ///
 /// Multiple publishers can be created for the same topic, in different nodes or the same node.
@@ -44,21 +57,6 @@ where
     handle: PublisherHandle,
 }
 
-impl<T> Drop for Publisher<T>
-where
-    T: Message,
-{
-    fn drop(&mut self) {
-        unsafe {
-            // SAFETY: No preconditions for this function (besides the arguments being valid).
-            rcl_publisher_fini(
-                self.handle.rcl_publisher.get_mut().unwrap(),
-                &mut *self.handle.node_handle.rcl_node.lock().unwrap(),
-            );
-        }
-    }
-}
-
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
 // they are running in. Therefore, this type can be safely sent to another thread.
 unsafe impl<T> Send for Publisher<T> where T: Message {}
@@ -99,9 +97,10 @@ where
             // 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();
             rcl_publisher_init(
                 &mut rcl_publisher,
-                &*node_handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 type_support_ptr,
                 topic_c_string.as_ptr(),
                 &publisher_options,
diff --git a/rclrs/src/service.rs b/rclrs/src/service.rs
index e0d769aad..1477ebcdf 100644
--- a/rclrs/src/service.rs
+++ b/rclrs/src/service.rs
@@ -29,10 +29,10 @@ impl ServiceHandle {
 impl Drop for ServiceHandle {
     fn drop(&mut self) {
         let rcl_service = self.rcl_service.get_mut().unwrap();
-        let rcl_node = &mut *self.node_handle.rcl_node.lock().unwrap();
+        let mut rcl_node = self.node_handle.rcl_node.lock().unwrap();
         // SAFETY: No preconditions for this function
         unsafe {
-            rcl_service_fini(rcl_service, rcl_node);
+            rcl_service_fini(rcl_service, &mut *rcl_node);
         }
     }
 }
@@ -99,9 +99,10 @@ where
             // 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();
             rcl_service_init(
                 &mut rcl_service,
-                &*node_handle.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 type_support,
                 topic_c_string.as_ptr(),
                 &service_options as *const _,
diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs
index cd235e970..197847f06 100644
--- a/rclrs/src/subscription.rs
+++ b/rclrs/src/subscription.rs
@@ -37,10 +37,10 @@ impl SubscriptionHandle {
 impl Drop for SubscriptionHandle {
     fn drop(&mut self) {
         let rcl_subscription = self.rcl_subscription.get_mut().unwrap();
-        let rcl_node = &mut *self.node_handle.rcl_node.lock().unwrap();
+        let mut rcl_node = self.node_handle.rcl_node.lock().unwrap();
         // SAFETY: No preconditions for this function (besides the arguments being valid).
         unsafe {
-            rcl_subscription_fini(rcl_subscription, rcl_node);
+            rcl_subscription_fini(rcl_subscription, &mut *rcl_node);
         }
     }
 }
@@ -85,7 +85,7 @@ where
 {
     /// Creates a new subscription.
     pub(crate) fn new<Args>(
-        rcl_node: Arc<NodeHandle>,
+        node_handle: Arc<NodeHandle>,
         topic: &str,
         qos: QoSProfile,
         callback: impl SubscriptionCallback<T, Args>,
@@ -113,9 +113,10 @@ where
             // 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();
             rcl_subscription_init(
                 &mut rcl_subscription,
-                &*rcl_node.rcl_node.lock().unwrap(),
+                &*rcl_node,
                 type_support,
                 topic_c_string.as_ptr(),
                 &subscription_options,
@@ -125,7 +126,7 @@ where
 
         let handle = Arc::new(SubscriptionHandle {
             rcl_subscription: Mutex::new(rcl_subscription),
-            node_handle: rcl_node,
+            node_handle,
             in_use_by_wait_set: Arc::new(AtomicBool::new(false)),
         });
 

From 2adcf3e1d0df3e319533e82e2d2520fe33fa09d6 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Thu, 4 Apr 2024 13:25:02 +0000
Subject: [PATCH 06/19] Apply lifecycle lock to all middleware entities

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/Cargo.toml                  |  8 ++++++-
 rclrs/src/client.rs               |  4 +++-
 rclrs/src/context.rs              | 35 +++++++++++++++++++++----------
 rclrs/src/node.rs                 |  3 ++-
 rclrs/src/node/builder.rs         |  3 ++-
 rclrs/src/publisher.rs            |  4 +++-
 rclrs/src/service.rs              |  4 +++-
 rclrs/src/subscription.rs         |  4 +++-
 rclrs/src/wait.rs                 |  3 ++-
 rclrs/src/wait/guard_condition.rs |  2 +-
 10 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/rclrs/Cargo.toml b/rclrs/Cargo.toml
index 69319f659..14a8e9e91 100644
--- a/rclrs/Cargo.toml
+++ b/rclrs/Cargo.toml
@@ -14,15 +14,21 @@ path = "src/lib.rs"
 # Please keep the list of dependencies alphabetically sorted,
 # and also state why each dependency is needed.
 [dependencies]
-# Needed for dynamically finding type support libraries 
+# Needed for dynamically finding type support libraries
 ament_rs = { version = "0.2", optional = true }
+
 # Needed for uploading documentation to docs.rs
 cfg-if = "1.0.0"
 
 # Needed for clients
 futures = "0.3"
+
+# Needed to create a global mutex for managing the lifecycles of middleware entities
+lazy_static = "1.4"
+
 # Needed for dynamic messages
 libloading = { version = "0.8", optional = true }
+
 # Needed for the Message trait, among others
 rosidl_runtime_rs = "0.4"
 
diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs
index 140aeafb3..86cefb03a 100644
--- a/rclrs/src/client.rs
+++ b/rclrs/src/client.rs
@@ -9,7 +9,7 @@ use rosidl_runtime_rs::Message;
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::MessageCow;
-use crate::{rcl_bindings::*, RclrsError, NodeHandle};
+use crate::{rcl_bindings::*, RclrsError, NodeHandle, ENTITY_LIFECYCLE_MUTEX};
 
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
 // they are running in. Therefore, this type can be safely sent to another thread.
@@ -32,6 +32,7 @@ impl Drop for ClientHandle {
     fn drop(&mut self) {
         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
         unsafe {
             rcl_client_fini(rcl_client, &mut *rcl_node);
@@ -99,6 +100,7 @@ where
             // afterwards.
             {
                 let mut rcl_node = node_handle.rcl_node.lock().unwrap();
+                let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
                 rcl_client_init(
                     &mut rcl_client,
                     &mut *rcl_node,
diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index 485df1f2c..012db600d 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -7,6 +7,15 @@ use std::vec::Vec;
 use crate::rcl_bindings::*;
 use crate::{RclrsError, ToResult};
 
+lazy_static::lazy_static! {
+    /// This is locked whenever initializing or dropping any middleware entity
+    /// because we have found issues in RCL and some RMW implementations that
+    /// make it unsafe to simultaneously initialize and/or drop various types of
+    /// entities. It seems these C and C++ based libraries will regularly use
+    /// unprotected global variables in their object initialization and cleanup.
+    pub(crate) static ref ENTITY_LIFECYCLE_MUTEX: Mutex<()> = Mutex::new(());
+}
+
 impl Drop for rcl_context_t {
     fn drop(&mut self) {
         unsafe {
@@ -14,6 +23,7 @@ impl Drop for rcl_context_t {
             // line arguments.
             // SAFETY: No preconditions for this function.
             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
                 rcl_shutdown(self);
                 rcl_context_fini(self);
@@ -102,17 +112,20 @@ impl Context {
             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.
-            let ret = rcl_init(
-                c_args.len() as i32,
-                if c_args.is_empty() {
-                    std::ptr::null()
-                } else {
-                    c_args.as_ptr()
-                },
-                &rcl_init_options,
-                &mut rcl_context,
-            )
-            .ok();
+            let ret = {
+                let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
+                rcl_init(
+                    c_args.len() as i32,
+                    if c_args.is_empty() {
+                        std::ptr::null()
+                    } else {
+                        c_args.as_ptr()
+                    },
+                    &rcl_init_options,
+                    &mut rcl_context,
+                )
+                .ok()
+            };
             // SAFETY: It's safe to pass in an initialized object.
             // Early return will not leak memory, because this is the last fini function.
             rcl_init_options_fini(&mut rcl_init_options).ok()?;
diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index 386c36588..37c6ec695 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -13,7 +13,7 @@ pub use self::builder::*;
 pub use self::graph::*;
 use crate::rcl_bindings::*;
 use crate::{
-    Client, ClientBase, Clock, Context, GuardCondition, ParameterBuilder, ParameterInterface,
+    Client, ClientBase, Clock, Context, ENTITY_LIFECYCLE_MUTEX, GuardCondition, ParameterBuilder, ParameterInterface,
     ParameterVariant, Parameters, Publisher, QoSProfile, RclrsError, Service, ServiceBase,
     Subscription, SubscriptionBase, SubscriptionCallback, TimeSource, ContextHandle,
 };
@@ -78,6 +78,7 @@ impl Drop for NodeHandle {
     fn drop(&mut self) {
         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();
         unsafe { rcl_node_fini(&mut *rcl_node) };
     }
 }
diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 5fe267495..6a16fd0a9 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -3,7 +3,7 @@ use std::sync::{Arc, Mutex};
 
 use crate::rcl_bindings::*;
 use crate::{
-    ClockType, Context, ContextHandle, Node, NodeHandle, ParameterInterface, QoSProfile, RclrsError, TimeSource, ToResult,
+    ClockType, Context, ContextHandle, ENTITY_LIFECYCLE_MUTEX, Node, NodeHandle, ParameterInterface, QoSProfile, RclrsError, TimeSource, ToResult,
     QOS_PROFILE_CLOCK,
 };
 
@@ -270,6 +270,7 @@ impl NodeBuilder {
             // 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.
+            let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
             rcl_node_init(
                 &mut rcl_node,
                 node_name.as_ptr(),
diff --git a/rclrs/src/publisher.rs b/rclrs/src/publisher.rs
index 390894d24..ce5774ee6 100644
--- a/rclrs/src/publisher.rs
+++ b/rclrs/src/publisher.rs
@@ -9,7 +9,7 @@ use rosidl_runtime_rs::{Message, RmwMessage};
 use crate::error::{RclrsError, ToResult};
 use crate::qos::QoSProfile;
 use crate::rcl_bindings::*;
-use crate::NodeHandle;
+use crate::{NodeHandle, ENTITY_LIFECYCLE_MUTEX};
 
 mod loaned_message;
 pub use loaned_message::*;
@@ -28,6 +28,7 @@ impl Drop for PublisherHandle {
         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,
@@ -98,6 +99,7 @@ where
             // 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,
diff --git a/rclrs/src/service.rs b/rclrs/src/service.rs
index 1477ebcdf..cd30445fb 100644
--- a/rclrs/src/service.rs
+++ b/rclrs/src/service.rs
@@ -7,7 +7,7 @@ use rosidl_runtime_rs::Message;
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::{rcl_bindings::*, MessageCow, RclrsError};
-use crate::NodeHandle;
+use crate::{NodeHandle, ENTITY_LIFECYCLE_MUTEX};
 
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
 // they are running in. Therefore, this type can be safely sent to another thread.
@@ -30,6 +30,7 @@ impl Drop for ServiceHandle {
     fn drop(&mut self) {
         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
         unsafe {
             rcl_service_fini(rcl_service, &mut *rcl_node);
@@ -100,6 +101,7 @@ where
             // 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,
diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs
index 197847f06..d72f25f84 100644
--- a/rclrs/src/subscription.rs
+++ b/rclrs/src/subscription.rs
@@ -8,7 +8,7 @@ use rosidl_runtime_rs::{Message, RmwMessage};
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::qos::QoSProfile;
-use crate::{rcl_bindings::*, RclrsError, NodeHandle};
+use crate::{rcl_bindings::*, RclrsError, NodeHandle, ENTITY_LIFECYCLE_MUTEX};
 
 mod callback;
 mod message_info;
@@ -38,6 +38,7 @@ impl Drop for SubscriptionHandle {
     fn drop(&mut self) {
         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).
         unsafe {
             rcl_subscription_fini(rcl_subscription, &mut *rcl_node);
@@ -114,6 +115,7 @@ where
             // 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,
diff --git a/rclrs/src/wait.rs b/rclrs/src/wait.rs
index 56a2c52bf..26b1dc385 100644
--- a/rclrs/src/wait.rs
+++ b/rclrs/src/wait.rs
@@ -96,6 +96,7 @@ impl WaitSet {
         let rcl_wait_set = unsafe {
             // SAFETY: Getting a zero-initialized value is always safe
             let mut rcl_wait_set = rcl_get_zero_initialized_wait_set();
+            let mut rcl_context = context.handle.rcl_context.lock().unwrap();
             // SAFETY: We're passing in a zero-initialized wait set and a valid context.
             // There are no other preconditions.
             rcl_wait_set_init(
@@ -106,7 +107,7 @@ impl WaitSet {
                 number_of_clients,
                 number_of_services,
                 number_of_events,
-                &mut *context.handle.rcl_context.lock().unwrap(),
+                &mut *rcl_context,
                 rcutils_get_default_allocator(),
             )
             .ok()?;
diff --git a/rclrs/src/wait/guard_condition.rs b/rclrs/src/wait/guard_condition.rs
index b3c7001a7..e91c17eeb 100644
--- a/rclrs/src/wait/guard_condition.rs
+++ b/rclrs/src/wait/guard_condition.rs
@@ -110,9 +110,9 @@ impl GuardCondition {
         callback: Option<Box<dyn Fn() + Send + Sync>>,
     ) -> Self {
         let rcl_guard_condition = {
-            let mut rcl_context = context_handle.rcl_context.lock().unwrap();
             // SAFETY: Getting a zero initialized value is always safe
             let mut guard_condition = unsafe { rcl_get_zero_initialized_guard_condition() };
+            let mut rcl_context = context_handle.rcl_context.lock().unwrap();
             unsafe {
                 // SAFETY: The context must be valid, and the guard condition must be zero-initialized
                 rcl_guard_condition_init(

From 4dc88b1c9ffac1c7d4876aa0d7945d8512e9f405 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Thu, 4 Apr 2024 17:05:43 +0000
Subject: [PATCH 07/19] Run rustfmt

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/client.rs               |  2 +-
 rclrs/src/context.rs              |  6 +++---
 rclrs/src/executor.rs             |  4 +++-
 rclrs/src/node.rs                 | 13 +++++--------
 rclrs/src/node/builder.rs         |  4 ++--
 rclrs/src/node/graph.rs           | 20 ++++----------------
 rclrs/src/publisher.rs            |  7 ++-----
 rclrs/src/subscription.rs         |  2 +-
 rclrs/src/wait.rs                 |  2 +-
 rclrs/src/wait/guard_condition.rs |  3 ++-
 10 files changed, 24 insertions(+), 39 deletions(-)

diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs
index 86cefb03a..0b4dba493 100644
--- a/rclrs/src/client.rs
+++ b/rclrs/src/client.rs
@@ -9,7 +9,7 @@ use rosidl_runtime_rs::Message;
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::MessageCow;
-use crate::{rcl_bindings::*, RclrsError, NodeHandle, ENTITY_LIFECYCLE_MUTEX};
+use crate::{rcl_bindings::*, NodeHandle, RclrsError, ENTITY_LIFECYCLE_MUTEX};
 
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
 // they are running in. Therefore, this type can be safely sent to another thread.
diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index 012db600d..f4dd0a27d 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -90,7 +90,7 @@ impl Context {
     /// assert_eq!(context.domain_id(), 5);
     /// ````
     pub fn new_with_options(
-        args: impl IntoIterator<Item=String>,
+        args: impl IntoIterator<Item = String>,
         options: InitOptions,
     ) -> Result<Self, RclrsError> {
         // SAFETY: Getting a zero-initialized value is always safe
@@ -135,7 +135,7 @@ impl Context {
         Ok(Self {
             handle: Arc::new(ContextHandle {
                 rcl_context: Mutex::new(rcl_context),
-            })
+            }),
         })
     }
 
@@ -150,7 +150,7 @@ impl Context {
         let ret = unsafe {
             rcl_context_get_domain_id(
                 &mut *self.handle.rcl_context.lock().unwrap(),
-                &mut domain_id
+                &mut domain_id,
             )
         };
 
diff --git a/rclrs/src/executor.rs b/rclrs/src/executor.rs
index 79171e329..2f0b6c4d9 100644
--- a/rclrs/src/executor.rs
+++ b/rclrs/src/executor.rs
@@ -42,7 +42,9 @@ impl SingleThreadedExecutor {
         for node in { self.nodes_mtx.lock().unwrap() }
             .iter()
             .filter_map(Weak::upgrade)
-            .filter(|node| unsafe { rcl_context_is_valid(&*node.handle.context_handle.rcl_context.lock().unwrap()) })
+            .filter(|node| unsafe {
+                rcl_context_is_valid(&*node.handle.context_handle.rcl_context.lock().unwrap())
+            })
         {
             let wait_set = WaitSet::new_for_node(&node)?;
             let ready_entities = wait_set.wait(timeout)?;
diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index 37c6ec695..857694966 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -13,9 +13,10 @@ pub use self::builder::*;
 pub use self::graph::*;
 use crate::rcl_bindings::*;
 use crate::{
-    Client, ClientBase, Clock, Context, ENTITY_LIFECYCLE_MUTEX, GuardCondition, ParameterBuilder, ParameterInterface,
-    ParameterVariant, Parameters, Publisher, QoSProfile, RclrsError, Service, ServiceBase,
-    Subscription, SubscriptionBase, SubscriptionCallback, TimeSource, ContextHandle,
+    Client, ClientBase, Clock, Context, ContextHandle, GuardCondition, ParameterBuilder,
+    ParameterInterface, ParameterVariant, Parameters, Publisher, QoSProfile, RclrsError, Service,
+    ServiceBase, Subscription, SubscriptionBase, SubscriptionCallback, TimeSource,
+    ENTITY_LIFECYCLE_MUTEX,
 };
 
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
@@ -257,11 +258,7 @@ impl Node {
     where
         T: Message,
     {
-        let publisher = Arc::new(Publisher::<T>::new(
-            Arc::clone(&self.handle),
-            topic,
-            qos,
-        )?);
+        let publisher = Arc::new(Publisher::<T>::new(Arc::clone(&self.handle), topic, qos)?);
         Ok(publisher)
     }
 
diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 6a16fd0a9..28c75fc12 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -3,8 +3,8 @@ use std::sync::{Arc, Mutex};
 
 use crate::rcl_bindings::*;
 use crate::{
-    ClockType, Context, ContextHandle, ENTITY_LIFECYCLE_MUTEX, Node, NodeHandle, ParameterInterface, QoSProfile, RclrsError, TimeSource, ToResult,
-    QOS_PROFILE_CLOCK,
+    ClockType, Context, ContextHandle, Node, NodeHandle, ParameterInterface, QoSProfile,
+    RclrsError, TimeSource, ToResult, ENTITY_LIFECYCLE_MUTEX, QOS_PROFILE_CLOCK,
 };
 
 /// A builder for creating a [`Node`][1].
diff --git a/rclrs/src/node/graph.rs b/rclrs/src/node/graph.rs
index 3190c1926..48e725790 100644
--- a/rclrs/src/node/graph.rs
+++ b/rclrs/src/node/graph.rs
@@ -266,12 +266,7 @@ impl Node {
         // SAFETY: The topic_name string was correctly allocated previously
         unsafe {
             let rcl_node = self.handle.rcl_node.lock().unwrap();
-            rcl_count_publishers(
-                &*rcl_node,
-                topic_name.as_ptr(),
-                &mut count,
-            )
-            .ok()?
+            rcl_count_publishers(&*rcl_node, topic_name.as_ptr(), &mut count).ok()?
         };
         Ok(count)
     }
@@ -287,12 +282,7 @@ impl Node {
         // SAFETY: The topic_name string was correctly allocated previously
         unsafe {
             let rcl_node = self.handle.rcl_node.lock().unwrap();
-            rcl_count_subscribers(
-                &*rcl_node,
-                topic_name.as_ptr(),
-                &mut count,
-            )
-            .ok()?
+            rcl_count_subscribers(&*rcl_node, topic_name.as_ptr(), &mut count).ok()?
         };
         Ok(count)
     }
@@ -469,10 +459,8 @@ mod tests {
 
     #[test]
     fn test_graph_empty() {
-        let context = Context::new_with_options(
-            [],
-            InitOptions::new().with_domain_id(Some(99))
-        ).unwrap();
+        let context =
+            Context::new_with_options([], InitOptions::new().with_domain_id(Some(99))).unwrap();
         let node_name = "test_publisher_names_and_types";
         let node = Node::new(&context, node_name).unwrap();
         // Test that the graph has no publishers
diff --git a/rclrs/src/publisher.rs b/rclrs/src/publisher.rs
index ce5774ee6..cc0407e14 100644
--- a/rclrs/src/publisher.rs
+++ b/rclrs/src/publisher.rs
@@ -29,10 +29,7 @@ impl Drop for PublisherHandle {
             // 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,
-            );
+            rcl_publisher_fini(self.rcl_publisher.get_mut().unwrap(), &mut *rcl_node);
         }
     }
 }
@@ -116,7 +113,7 @@ where
             handle: PublisherHandle {
                 rcl_publisher: Mutex::new(rcl_publisher),
                 node_handle,
-            }
+            },
         })
     }
 
diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs
index d72f25f84..1bf45fded 100644
--- a/rclrs/src/subscription.rs
+++ b/rclrs/src/subscription.rs
@@ -8,7 +8,7 @@ use rosidl_runtime_rs::{Message, RmwMessage};
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::qos::QoSProfile;
-use crate::{rcl_bindings::*, RclrsError, NodeHandle, ENTITY_LIFECYCLE_MUTEX};
+use crate::{rcl_bindings::*, NodeHandle, RclrsError, ENTITY_LIFECYCLE_MUTEX};
 
 mod callback;
 mod message_info;
diff --git a/rclrs/src/wait.rs b/rclrs/src/wait.rs
index 26b1dc385..959689679 100644
--- a/rclrs/src/wait.rs
+++ b/rclrs/src/wait.rs
@@ -21,7 +21,7 @@ use std::vec::Vec;
 
 use crate::error::{to_rclrs_result, RclReturnCode, RclrsError, ToResult};
 use crate::rcl_bindings::*;
-use crate::{ClientBase, Context, Node, ServiceBase, SubscriptionBase, ContextHandle};
+use crate::{ClientBase, Context, ContextHandle, Node, ServiceBase, SubscriptionBase};
 
 mod exclusivity_guard;
 mod guard_condition;
diff --git a/rclrs/src/wait/guard_condition.rs b/rclrs/src/wait/guard_condition.rs
index e91c17eeb..497ddb817 100644
--- a/rclrs/src/wait/guard_condition.rs
+++ b/rclrs/src/wait/guard_condition.rs
@@ -139,7 +139,8 @@ impl GuardCondition {
     pub fn trigger(&self) -> Result<(), RclrsError> {
         unsafe {
             // SAFETY: The rcl_guard_condition_t is valid.
-            rcl_trigger_guard_condition(&mut *self.handle.rcl_guard_condition.lock().unwrap()).ok()?;
+            rcl_trigger_guard_condition(&mut *self.handle.rcl_guard_condition.lock().unwrap())
+                .ok()?;
         }
         if let Some(callback) = &self.callback {
             callback();

From 1b6eeb48af7a85f860cf195f3827fc41baf578dc Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Thu, 4 Apr 2024 17:22:35 +0000
Subject: [PATCH 08/19] Run clippy

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/client.rs       | 4 ++--
 rclrs/src/context.rs      | 2 +-
 rclrs/src/node/builder.rs | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs
index 0b4dba493..3e7aa172c 100644
--- a/rclrs/src/client.rs
+++ b/rclrs/src/client.rs
@@ -99,11 +99,11 @@ where
             // The topic name and the options are copied by this function, so they can be dropped
             // afterwards.
             {
-                let mut rcl_node = node_handle.rcl_node.lock().unwrap();
+                let rcl_node = node_handle.rcl_node.lock().unwrap();
                 let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
                 rcl_client_init(
                     &mut rcl_client,
-                    &mut *rcl_node,
+                    &*rcl_node,
                     type_support,
                     topic_c_string.as_ptr(),
                     &client_options,
diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index f4dd0a27d..ab55b11fe 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -219,7 +219,7 @@ impl InitOptions {
             if let Some(domain_id) = self.domain_id {
                 rcl_init_options_set_domain_id(&mut rcl_init_options, domain_id as usize);
             }
-            return Ok(rcl_init_options);
+            Ok(rcl_init_options)
         }
     }
 }
diff --git a/rclrs/src/node/builder.rs b/rclrs/src/node/builder.rs
index 28c75fc12..27aacd7bb 100644
--- a/rclrs/src/node/builder.rs
+++ b/rclrs/src/node/builder.rs
@@ -288,7 +288,7 @@ impl NodeBuilder {
         let parameter = {
             let rcl_node = handle.rcl_node.lock().unwrap();
             ParameterInterface::new(
-                &*rcl_node,
+                &rcl_node,
                 &rcl_node_options.arguments,
                 &rcl_context.global_arguments,
             )?

From f2ca13ee5756c8fe2fdd1e6518e6db8523916714 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Thu, 4 Apr 2024 17:34:45 +0000
Subject: [PATCH 09/19] Ensure that test_graph_empty works even if the system
 has ROS_DOMAIN_ID set to 99

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/node/graph.rs | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/rclrs/src/node/graph.rs b/rclrs/src/node/graph.rs
index 48e725790..01843ecb5 100644
--- a/rclrs/src/node/graph.rs
+++ b/rclrs/src/node/graph.rs
@@ -459,8 +459,24 @@ mod tests {
 
     #[test]
     fn test_graph_empty() {
+        let domain_id: usize = std::env::var("ROS_DOMAIN_ID")
+            .ok()
+            .and_then(|value| value.parse().ok())
+            .map(|value| {
+                if value == 99 {
+                    // The default domain ID for this application is 99, which
+                    // conflicts with our arbitrarily chosen default for the
+                    // empty graph test. Therefore we will set the empty graph
+                    // test domain ID to 98 instead.
+                    98
+                } else {
+                    99
+                }
+            })
+            .unwrap_or(99);
+
         let context =
-            Context::new_with_options([], InitOptions::new().with_domain_id(Some(99))).unwrap();
+            Context::new_with_options([], InitOptions::new().with_domain_id(Some(domain_id))).unwrap();
         let node_name = "test_publisher_names_and_types";
         let node = Node::new(&context, node_name).unwrap();
         // Test that the graph has no publishers

From 44d6166ececa988a3440ace1b86439fe588ed389 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Thu, 4 Apr 2024 17:38:34 +0000
Subject: [PATCH 10/19] Run rustfmt

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/node/graph.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/rclrs/src/node/graph.rs b/rclrs/src/node/graph.rs
index 01843ecb5..750266709 100644
--- a/rclrs/src/node/graph.rs
+++ b/rclrs/src/node/graph.rs
@@ -476,7 +476,8 @@ mod tests {
             .unwrap_or(99);
 
         let context =
-            Context::new_with_options([], InitOptions::new().with_domain_id(Some(domain_id))).unwrap();
+            Context::new_with_options([], InitOptions::new().with_domain_id(Some(domain_id)))
+                .unwrap();
         let node_name = "test_publisher_names_and_types";
         let node = Node::new(&context, node_name).unwrap();
         // Test that the graph has no publishers

From 8b6e82522c5880d13bca75409b9a1f9e479b2b7c Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Thu, 4 Apr 2024 17:43:20 +0000
Subject: [PATCH 11/19] Use usize instead of u8 for domain id

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/context.rs    | 12 ++++++------
 rclrs/src/node/graph.rs |  5 +++--
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index ab55b11fe..4bd9dffd3 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -145,7 +145,7 @@ impl Context {
     /// It can be set through the `ROS_DOMAIN_ID` environment variable.
     ///
     /// [1]: https://docs.ros.org/en/rolling/Concepts/About-Domain-ID.html
-    pub fn domain_id(&self) -> u8 {
+    pub fn domain_id(&self) -> usize {
         let mut domain_id: usize = 0;
         let ret = unsafe {
             rcl_context_get_domain_id(
@@ -155,7 +155,7 @@ impl Context {
         };
 
         debug_assert_eq!(ret, 0);
-        domain_id as u8
+        domain_id
     }
 
     /// Checks if the context is still valid.
@@ -179,7 +179,7 @@ pub struct InitOptions {
     /// [ROS_DOMAIN_ID][1] environment variable.
     ///
     /// [1]: https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Domain-ID.html#the-ros-domain-id
-    domain_id: Option<u8>,
+    domain_id: Option<usize>,
 }
 
 impl InitOptions {
@@ -189,19 +189,19 @@ impl InitOptions {
     }
 
     /// Transform an InitOptions into a new one with a certain domain_id
-    pub fn with_domain_id(mut self, domain_id: Option<u8>) -> InitOptions {
+    pub fn with_domain_id(mut self, domain_id: Option<usize>) -> InitOptions {
         self.domain_id = domain_id;
         self
     }
 
     /// Set the domain_id of an InitOptions, or reset it to the default behavior
     /// (determined by environment variables) by providing None.
-    pub fn set_domain_id(&mut self, domain_id: Option<u8>) {
+    pub fn set_domain_id(&mut self, domain_id: Option<usize>) {
         self.domain_id = domain_id;
     }
 
     /// Get the domain_id that will be provided by these InitOptions.
-    pub fn domain_id(&self) -> Option<u8> {
+    pub fn domain_id(&self) -> Option<usize> {
         self.domain_id
     }
 
diff --git a/rclrs/src/node/graph.rs b/rclrs/src/node/graph.rs
index 01843ecb5..95b04cc42 100644
--- a/rclrs/src/node/graph.rs
+++ b/rclrs/src/node/graph.rs
@@ -462,7 +462,7 @@ mod tests {
         let domain_id: usize = std::env::var("ROS_DOMAIN_ID")
             .ok()
             .and_then(|value| value.parse().ok())
-            .map(|value| {
+            .map(|value: usize| {
                 if value == 99 {
                     // The default domain ID for this application is 99, which
                     // conflicts with our arbitrarily chosen default for the
@@ -476,7 +476,8 @@ mod tests {
             .unwrap_or(99);
 
         let context =
-            Context::new_with_options([], InitOptions::new().with_domain_id(Some(domain_id))).unwrap();
+            Context::new_with_options([], InitOptions::new().with_domain_id(Some(domain_id)))
+                .unwrap();
         let node_name = "test_publisher_names_and_types";
         let node = Node::new(&context, node_name).unwrap();
         // Test that the graph has no publishers

From 6d9043164979da57668bc17e7a5eba1cc8a56bec Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Thu, 4 Apr 2024 18:43:24 +0000
Subject: [PATCH 12/19] Satisfy clippy

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/context.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index 4bd9dffd3..ad82c680e 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -217,7 +217,7 @@ impl InitOptions {
             // other than None. When the user asks for None, that is equivalent
             // to the default value in rcl_init_options.
             if let Some(domain_id) = self.domain_id {
-                rcl_init_options_set_domain_id(&mut rcl_init_options, domain_id as usize);
+                rcl_init_options_set_domain_id(&mut rcl_init_options, domain_id);
             }
             Ok(rcl_init_options)
         }

From 4caa2081e32f7b0651aab035585bfcb97ce695da Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Fri, 5 Apr 2024 04:17:14 +0000
Subject: [PATCH 13/19] Remove the need for lazy_static

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/Cargo.toml     |  3 ---
 rclrs/src/context.rs | 14 ++++++--------
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/rclrs/Cargo.toml b/rclrs/Cargo.toml
index 14a8e9e91..8ea67974b 100644
--- a/rclrs/Cargo.toml
+++ b/rclrs/Cargo.toml
@@ -23,9 +23,6 @@ cfg-if = "1.0.0"
 # Needed for clients
 futures = "0.3"
 
-# Needed to create a global mutex for managing the lifecycles of middleware entities
-lazy_static = "1.4"
-
 # Needed for dynamic messages
 libloading = { version = "0.8", optional = true }
 
diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index ad82c680e..7e69d7d09 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -7,14 +7,12 @@ use std::vec::Vec;
 use crate::rcl_bindings::*;
 use crate::{RclrsError, ToResult};
 
-lazy_static::lazy_static! {
-    /// This is locked whenever initializing or dropping any middleware entity
-    /// because we have found issues in RCL and some RMW implementations that
-    /// make it unsafe to simultaneously initialize and/or drop various types of
-    /// entities. It seems these C and C++ based libraries will regularly use
-    /// unprotected global variables in their object initialization and cleanup.
-    pub(crate) static ref ENTITY_LIFECYCLE_MUTEX: Mutex<()> = Mutex::new(());
-}
+/// This is locked whenever initializing or dropping any middleware entity
+/// because we have found issues in RCL and some RMW implementations that
+/// make it unsafe to simultaneously initialize and/or drop various types of
+/// entities. It seems these C and C++ based libraries will regularly use
+/// unprotected global variables in their object initialization and cleanup.
+pub(crate) static ENTITY_LIFECYCLE_MUTEX: Mutex<()> = Mutex::new(());
 
 impl Drop for rcl_context_t {
     fn drop(&mut self) {

From 5abbb3419a42b1e590d9e9714a88d6b51319ee70 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Fri, 5 Apr 2024 16:16:21 +0000
Subject: [PATCH 14/19] Update documentation and safety info on rcl entity
 lifecycles

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/client.rs               | 29 +++++++++++++-------
 rclrs/src/context.rs              | 17 +++++++-----
 rclrs/src/node.rs                 |  9 +++++--
 rclrs/src/node/builder.rs         | 10 ++++---
 rclrs/src/publisher.rs            | 44 +++++++++++++++++++------------
 rclrs/src/service.rs              | 40 +++++++++++++++++-----------
 rclrs/src/subscription.rs         | 41 +++++++++++++++++-----------
 rclrs/src/wait.rs                 |  5 ++++
 rclrs/src/wait/guard_condition.rs |  5 ++++
 9 files changed, 130 insertions(+), 70 deletions(-)

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

From cf0d434814a7c02331478268f7685b664aa3d69d Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Fri, 5 Apr 2024 16:27:41 +0000
Subject: [PATCH 15/19] Rename to avoid confusion with Handle pattern

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/node.rs      | 4 ++--
 rclrs/src/parameter.rs | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index 943bd395d..a09644f8c 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -194,7 +194,7 @@ impl Node {
         getter: unsafe extern "C" fn(*const rcl_node_t) -> *const c_char,
     ) -> String {
         let rcl_node = self.handle.rcl_node.lock().unwrap();
-        unsafe { call_string_getter_with_handle(&rcl_node, getter) }
+        unsafe { call_string_getter_with_rcl_node(&rcl_node, getter) }
     }
 
     /// Creates a [`Client`][1].
@@ -446,7 +446,7 @@ impl Node {
 // function, which is why it's not merged into Node::call_string_getter().
 // This function is unsafe since it's possible to pass in an rcl_node_t with dangling
 // pointers etc.
-pub(crate) unsafe fn call_string_getter_with_handle(
+pub(crate) unsafe fn call_string_getter_with_rcl_node(
     rcl_node: &rcl_node_t,
     getter: unsafe extern "C" fn(*const rcl_node_t) -> *const c_char,
 ) -> String {
diff --git a/rclrs/src/parameter.rs b/rclrs/src/parameter.rs
index cbd9c8f3c..db3bb3948 100644
--- a/rclrs/src/parameter.rs
+++ b/rclrs/src/parameter.rs
@@ -5,7 +5,7 @@ pub(crate) use override_map::*;
 pub use value::*;
 
 use crate::rcl_bindings::*;
-use crate::{call_string_getter_with_handle, RclrsError};
+use crate::{call_string_getter_with_rcl_node, RclrsError};
 use std::collections::{btree_map::Entry, BTreeMap};
 use std::fmt::Debug;
 use std::marker::PhantomData;
@@ -781,7 +781,7 @@ impl ParameterInterface {
         global_arguments: &rcl_arguments_t,
     ) -> Result<Self, RclrsError> {
         let override_map = unsafe {
-            let fqn = call_string_getter_with_handle(rcl_node, rcl_node_get_fully_qualified_name);
+            let fqn = call_string_getter_with_rcl_node(rcl_node, rcl_node_get_fully_qualified_name);
             resolve_parameter_overrides(&fqn, node_arguments, global_arguments)?
         };
 

From 21d3b35533ed1839a3d701074ab54672af53ca61 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Fri, 5 Apr 2024 16:36:34 +0000
Subject: [PATCH 16/19] Improve the documentation for the domain ID situation
 of test_graph_empty

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/node/graph.rs | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/rclrs/src/node/graph.rs b/rclrs/src/node/graph.rs
index 95b04cc42..01ad8fd3d 100644
--- a/rclrs/src/node/graph.rs
+++ b/rclrs/src/node/graph.rs
@@ -459,20 +459,28 @@ mod tests {
 
     #[test]
     fn test_graph_empty() {
+        // cargo test by default will run all test functions in parallel using
+        // as many threads as the underlying system allows. However, the test
+        // expectations of test_graph_empty will fail if its detects any other middleware
+        // activity while it's running.
+        //
+        // If we ensure that the Context of test_graph_empty is given a different domain ID
+        // from the rest of the tests, then we can ensure that it will not observe any other
+        // middleware activity, and its expectations can pass (as long as the user is not
+        // running any other ROS executables on their system).
+        //
+        // By default we will assign 99 to the domain ID of test_graph_empty's Context.
+        // However, if the ROS_DOMAIN_ID environment variable was set to 99 by the user,
+        // then the rest of the tests will be using that value. So here we are detecting
+        // that situation and setting the domain ID of test_graph_empty's Context to 98
+        // in that situation.
+        //
+        // 99 and 98 are just chosen as arbitrary valid domain ID values. There is
+        // otherwise nothing special about either value.
         let domain_id: usize = std::env::var("ROS_DOMAIN_ID")
             .ok()
             .and_then(|value| value.parse().ok())
-            .map(|value: usize| {
-                if value == 99 {
-                    // The default domain ID for this application is 99, which
-                    // conflicts with our arbitrarily chosen default for the
-                    // empty graph test. Therefore we will set the empty graph
-                    // test domain ID to 98 instead.
-                    98
-                } else {
-                    99
-                }
-            })
+            .map(|value: usize| if value != 99 { 99 } else { 98 })
             .unwrap_or(99);
 
         let context =

From 0efa8315d78baa1141551d2e433a58ff1a6717fb Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Fri, 5 Apr 2024 16:43:55 +0000
Subject: [PATCH 17/19] Update documentation on ENTITY_LIFECYCLE_MUTEX

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/context.rs | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index 17305b0b5..660d81a86 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -9,9 +9,16 @@ use crate::{RclrsError, ToResult};
 
 /// This is locked whenever initializing or dropping any middleware entity
 /// because we have found issues in RCL and some RMW implementations that
-/// make it unsafe to simultaneously initialize and/or drop various types of
-/// entities. It seems these C and C++ based libraries will regularly use
+/// make it unsafe to simultaneously initialize and/or drop middleware
+/// entities such as [`rcl_context_t`] and [`rcl_node_t`] as well middleware
+/// primitives such as [`rcl_publisher_t`], [`rcl_subscription_t`], etc.
+/// It seems these C and C++ based libraries will regularly use
 /// unprotected global variables in their object initialization and cleanup.
+///
+/// Further discussion with the RCL team may help to improve the RCL
+/// documentation to specifically call out where these risks are present. For
+/// now we lock this mutex for any RCL function that carries reasonable suspicion
+/// of a risk.
 pub(crate) static ENTITY_LIFECYCLE_MUTEX: Mutex<()> = Mutex::new(());
 
 impl Drop for rcl_context_t {

From 8a16367a394993d588e5132744347b6c47c80cdc Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Fri, 5 Apr 2024 16:53:40 +0000
Subject: [PATCH 18/19] Get rid of doc links to private structs

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/client.rs               | 6 +++---
 rclrs/src/context.rs              | 8 ++++----
 rclrs/src/node.rs                 | 6 +++---
 rclrs/src/publisher.rs            | 6 +++---
 rclrs/src/service.rs              | 6 +++---
 rclrs/src/subscription.rs         | 6 +++---
 rclrs/src/wait.rs                 | 6 +++---
 rclrs/src/wait/guard_condition.rs | 6 +++---
 8 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs
index 63eec4a32..6fd44ac09 100644
--- a/rclrs/src/client.rs
+++ b/rclrs/src/client.rs
@@ -15,9 +15,9 @@ 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 {}
 
-/// 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`].
+/// 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 {
diff --git a/rclrs/src/context.rs b/rclrs/src/context.rs
index 660d81a86..6d566433c 100644
--- a/rclrs/src/context.rs
+++ b/rclrs/src/context.rs
@@ -10,8 +10,8 @@ use crate::{RclrsError, ToResult};
 /// This is locked whenever initializing or dropping any middleware entity
 /// because we have found issues in RCL and some RMW implementations that
 /// make it unsafe to simultaneously initialize and/or drop middleware
-/// entities such as [`rcl_context_t`] and [`rcl_node_t`] as well middleware
-/// primitives such as [`rcl_publisher_t`], [`rcl_subscription_t`], etc.
+/// entities such as `rcl_context_t` and `rcl_node_t` as well middleware
+/// primitives such as `rcl_publisher_t`, `rcl_subscription_t`, etc.
 /// It seems these C and C++ based libraries will regularly use
 /// unprotected global variables in their object initialization and cleanup.
 ///
@@ -59,10 +59,10 @@ pub struct Context {
     pub(crate) handle: Arc<ContextHandle>,
 }
 
-/// This struct manages the lifetime and access to the [`rcl_context_t`]. 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 {
diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index a09644f8c..e0abd098d 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -68,9 +68,9 @@ pub struct Node {
     pub(crate) handle: Arc<NodeHandle>,
 }
 
-/// 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`].
+/// 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 {
diff --git a/rclrs/src/publisher.rs b/rclrs/src/publisher.rs
index 2178ab0e8..d4c664a5c 100644
--- a/rclrs/src/publisher.rs
+++ b/rclrs/src/publisher.rs
@@ -18,9 +18,9 @@ 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`].
+/// 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 {
diff --git a/rclrs/src/service.rs b/rclrs/src/service.rs
index a2c964f6e..7352e3614 100644
--- a/rclrs/src/service.rs
+++ b/rclrs/src/service.rs
@@ -13,9 +13,9 @@ 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 {}
 
-/// 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`].
+/// 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 {
diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs
index 2e792a657..1c075d344 100644
--- a/rclrs/src/subscription.rs
+++ b/rclrs/src/subscription.rs
@@ -21,9 +21,9 @@ 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 {}
 
-/// 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`].
+/// 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 {
diff --git a/rclrs/src/wait.rs b/rclrs/src/wait.rs
index 0b6ec7fbc..1bdcd8c51 100644
--- a/rclrs/src/wait.rs
+++ b/rclrs/src/wait.rs
@@ -28,9 +28,9 @@ 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`].
+/// 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 {
diff --git a/rclrs/src/wait/guard_condition.rs b/rclrs/src/wait/guard_condition.rs
index eb3fc3daa..138dbb1dd 100644
--- a/rclrs/src/wait/guard_condition.rs
+++ b/rclrs/src/wait/guard_condition.rs
@@ -52,9 +52,9 @@ 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`].
+/// 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 {

From 58b2c663df468335038eb37e9d0b030c10b3bea6 Mon Sep 17 00:00:00 2001
From: "Michael X. Grey" <mxgrey@intrinsic.ai>
Date: Fri, 5 Apr 2024 16:56:53 +0000
Subject: [PATCH 19/19] Fix link formatting

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
---
 rclrs/src/client.rs               | 2 +-
 rclrs/src/node.rs                 | 2 +-
 rclrs/src/publisher.rs            | 2 +-
 rclrs/src/service.rs              | 2 +-
 rclrs/src/subscription.rs         | 2 +-
 rclrs/src/wait.rs                 | 2 +-
 rclrs/src/wait/guard_condition.rs | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/rclrs/src/client.rs b/rclrs/src/client.rs
index 6fd44ac09..e7b75cba0 100644
--- a/rclrs/src/client.rs
+++ b/rclrs/src/client.rs
@@ -19,7 +19,7 @@ unsafe impl Send for rcl_client_t {}
 /// 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
+/// [1]: <https://doc.rust-lang.org/reference/destructors.html>
 pub struct ClientHandle {
     rcl_client: Mutex<rcl_client_t>,
     node_handle: Arc<NodeHandle>,
diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs
index e0abd098d..9410e3321 100644
--- a/rclrs/src/node.rs
+++ b/rclrs/src/node.rs
@@ -72,7 +72,7 @@ pub struct Node {
 /// 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
+/// [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>,
diff --git a/rclrs/src/publisher.rs b/rclrs/src/publisher.rs
index d4c664a5c..82034f9ae 100644
--- a/rclrs/src/publisher.rs
+++ b/rclrs/src/publisher.rs
@@ -22,7 +22,7 @@ unsafe impl Send for rcl_publisher_t {}
 /// 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
+/// [1]: <https://doc.rust-lang.org/reference/destructors.html>
 struct PublisherHandle {
     rcl_publisher: Mutex<rcl_publisher_t>,
     node_handle: Arc<NodeHandle>,
diff --git a/rclrs/src/service.rs b/rclrs/src/service.rs
index 7352e3614..5dfa8f511 100644
--- a/rclrs/src/service.rs
+++ b/rclrs/src/service.rs
@@ -17,7 +17,7 @@ unsafe impl Send for rcl_service_t {}
 /// 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
+/// [1]: <https://doc.rust-lang.org/reference/destructors.html>
 pub struct ServiceHandle {
     rcl_service: Mutex<rcl_service_t>,
     node_handle: Arc<NodeHandle>,
diff --git a/rclrs/src/subscription.rs b/rclrs/src/subscription.rs
index 1c075d344..cbc72bc1b 100644
--- a/rclrs/src/subscription.rs
+++ b/rclrs/src/subscription.rs
@@ -25,7 +25,7 @@ unsafe impl Send for rcl_subscription_t {}
 /// 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
+/// [1]: <https://doc.rust-lang.org/reference/destructors.html>
 pub struct SubscriptionHandle {
     rcl_subscription: Mutex<rcl_subscription_t>,
     node_handle: Arc<NodeHandle>,
diff --git a/rclrs/src/wait.rs b/rclrs/src/wait.rs
index 1bdcd8c51..cd7c51f93 100644
--- a/rclrs/src/wait.rs
+++ b/rclrs/src/wait.rs
@@ -32,7 +32,7 @@ pub use guard_condition::*;
 /// 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
+/// [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 138dbb1dd..e9c4f1148 100644
--- a/rclrs/src/wait/guard_condition.rs
+++ b/rclrs/src/wait/guard_condition.rs
@@ -56,7 +56,7 @@ pub struct GuardCondition {
 /// 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
+/// [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