Ensure that mutex guards are not being dropped prematurely

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

Author: mxgrey

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 {

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);

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![]),

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, "")

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,

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 _,

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)),
         });