Have a single signature for create_service by bundling arguments

docs/building.md

@@ -19,8 +19,6 @@ mkdir src
 git clone https://github.com/ros2-rust/ros2_rust.git src/ros2_rust
 ```
 
-*Note: Once `rclrs` is published on crates.io, it's not technically needed anymore to clone the `ros2_rust` repo, and this section will be modified to reflect that.*
-
 
 ## Environment setup
 
@@ -151,47 +149,7 @@ The plugin to build `cargo` projects with `colcon` currently has the issue that
 Rust packages for ROS 2 can also be built with pure `cargo`, and still integrate with ROS 2 tools like `ros2 run`.
 
 
-### Learning by doing
-
-*Note: The following needs to be adapted once we publish `rclrs` on crates.io.*
-
-If you `cd` into e.g. `rclrs` before ever having built it with `colcon` and try to `cargo build` it, you'll see an error like
-
-```
-    Updating crates.io index
-error: no matching package named `rosidl_runtime_rs` found
-location searched: registry `crates-io`
-required by package `rclrs v0.2.0 (/workspace/ros2_rust/rclrs)`
-```
-
-Why is that? It's because `rclrs/Cargo.toml` contains `rosidl_runtime_rs = "*"` instead of `rosidl_runtime_rs = { path = "../rosidl_runtime_rs" }`, even though the package at that path is the one that is meant. If that's confusing, please read on. The reason is that it is a principle of ROS 2 and `colcon` for packages to reference their dependencies only by their _name_, and not by their path. This allows packages to be moved around freely in a workspace, or replaced by versions installed through `apt`, with no changes to the source code.
-
-Unfortunately, referring to a package only by its name makes `cargo` assume that it should download that package from `crates.io`. `colcon-ros-cargo` works around this with a little hack: At build-time, it resolves these names to concrete paths to the local package, which are then written into `.cargo/config.toml`. The entries in that file override the original locations on `crates.io`, and therefore the _local_ packages will be used instead.
-
-So, the problem above is fixed by doing one initial build of the package, or the whole workspace, with `colcon`. This creates the `.cargo/config.toml` file, and `cargo` will now use it to find `rosidl_runtime_rs`. Of course, if you don't want to install/use `colcon` for some reason, you can also create that file yourself, or replace the name-only dependencies in `rclrs/Cargo.toml` with paths.
-
-Running `cargo build` in `rclrs` will now work, as well as `cargo doc`, `cargo test` and all the other `cargo` commands. Unfortunately, `cargo` may sometimes print messages saying
-
-> warning: Patch `rclrs v0.1.0 (/workspace/install/rclrs/share/rclrs/rust)` was not used in the crate graph.
-
-This can be ignored.
-
-To summarize: 
-
-```shell
-# Initial build of the package with colcon
-# The --lookup-in-workspace flag is recommended for a cargo-based workflow
-# Compare .cargo/config.toml with and without it to see its effect
-colcon build --packages-up-to examples_rclrs_minimal_pub_sub --lookup-in-workspace
-cd src/ros2_rust/examples/minimal_pub_sub
-# Run cargo build, or cargo check, cargo doc, etc.
-cargo build
-```
-
-If you'd like to not have any of that "overriding dependencies through  `.cargo/config.toml`", you can do that too of course. You just need to use concrete paths for any dependency that isn't published on `crates.io`, such as message packages. For instance, you might have to write `std_msgs = {path = '/workspace/install/std_msgs/share/std_msgs/rust'}`.
-
-
-### Integration with ROS 2 tools
+## Integration with ROS 2 tools
 
 How can a binary created in Rust be made available to `ros2 run`, `ros2 launch` etc.? And how can other ROS 2 packages use a `cdylib` created in Rust? For that to work, the correct files must be placed at the correct location in the install directory, see [REP 122](https://www.ros.org/reps/rep-0122.html).
 

examples/minimal_client_service/src/minimal_service.rs

@@ -2,8 +2,10 @@ use std::env;
 
 use anyhow::{Error, Result};
 
+use rclrs::Req;
+
 fn handle_service(
-    request: example_interfaces::srv::AddTwoInts_Request,
+    Req{ request, .. }: Req<example_interfaces::srv::AddTwoInts_Request>,
 ) -> example_interfaces::srv::AddTwoInts_Response {
     println!("request: {} + {}", request.a, request.b);
     example_interfaces::srv::AddTwoInts_Response {

rclrs/Cargo.toml

@@ -14,24 +14,26 @@ 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 for dynamic messages
 libloading = { version = "0.8", optional = true }
+
 # Needed for the Message trait, among others
 rosidl_runtime_rs = "0.4"
 
-[dependencies.builtin_interfaces]
-version = "*"
-
 [dev-dependencies]
 # Needed for e.g. writing yaml files in tests
 tempfile = "3.3.0"
+# Needed for publisher and subscriber tests
+test_msgs = {version = "*"}
 
 [build-dependencies]
 # Needed for FFI

rclrs/package.xml

@@ -19,6 +19,8 @@
   <depend>builtin_interfaces</depend>
   <depend>rcl_interfaces</depend>
   <depend>rosgraph_msgs</depend>
+
+  <test_depend>test_msgs</test_depend>
 
   <export>
     <build_type>ament_cargo</build_type>

rclrs/src/client.rs

@@ -9,32 +9,38 @@ use rosidl_runtime_rs::Message;
 
 use crate::error::{RclReturnCode, ToResult};
 use crate::MessageCow;
-use crate::{rcl_bindings::*, RclrsError};
+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.
 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_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();
-        // SAFETY: No preconditions for this function
+        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: 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, rcl_node_mtx);
+            rcl_client_fini(rcl_client, &mut *rcl_node);
         }
     }
 }
@@ -74,7 +80,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
@@ -92,24 +98,32 @@ 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.
-            rcl_client_init(
-                &mut rcl_client,
-                &*rcl_node_mtx.lock().unwrap(),
-                type_support,
-                topic_c_string.as_ptr(),
-                &client_options,
-            )
-            .ok()?;
+        {
+            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,
+                    type_support,
+                    topic_c_string.as_ptr(),
+                    &client_options,
+                )
+                .ok()?;
+            }
         }
 
         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 +259,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
@@ -289,3 +303,38 @@ where
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::test_helpers::*;
+    use test_msgs::srv;
+
+    #[test]
+    fn traits() {
+        assert_send::<Client<srv::Arrays>>();
+        assert_sync::<Client<srv::Arrays>>();
+    }
+
+    #[test]
+    fn test_clients() -> Result<(), RclrsError> {
+        let namespace = "/test_clients_graph";
+        let graph = construct_test_graph(namespace)?;
+        let _node_2_empty_client = graph
+            .node2
+            .create_client::<srv::Empty>("graph_test_topic_4")?;
+
+        std::thread::sleep(std::time::Duration::from_millis(200));
+
+        let client_names_and_types = graph
+            .node2
+            .get_client_names_and_types_by_node(&graph.node2.name(), &graph.node2.namespace())?;
+        let types = client_names_and_types
+            .get("/test_clients_graph/graph_test_topic_4")
+            .unwrap();
+
+        assert!(types.contains(&"test_msgs/srv/Empty".to_string()));
+
+        Ok(())
+    }
+}

rclrs/src/clock.rs

@@ -195,11 +195,10 @@ unsafe impl Send for rcl_clock_t {}
 mod tests {
     use super::*;
 
-    fn assert_send<T: Send>() {}
-    fn assert_sync<T: Sync>() {}
-
     #[test]
-    fn clock_is_send_and_sync() {
+    fn traits() {
+        use crate::test_helpers::*;
+
         assert_send::<Clock>();
         assert_sync::<Clock>();
     }