Switch to shared state for action client/server

This matches the pattern used by the other endpoint structs.

Author: nwn

rclrs/src/action.rs

@@ -5,8 +5,8 @@ mod server_goal_handle;
 use crate::rcl_bindings::RCL_ACTION_UUID_SIZE;
 use std::fmt;
 
-pub use client::{ActionClient, ActionClientBase};
-pub use server::{ActionServer, ActionServerBase};
+pub use client::{ActionClient, ActionClientBase, ActionClientState};
+pub use server::{ActionServer, ActionServerBase, ActionServerState};
 pub use server_goal_handle::ServerGoalHandle;
 
 /// A unique identifier for a goal request.

rclrs/src/action/client.rs

@@ -1,5 +1,5 @@
 use crate::{
-    error::ToResult, rcl_bindings::*, wait::WaitableNumEntities, Node, RclrsError,
+    error::ToResult, rcl_bindings::*, wait::WaitableNumEntities, Node, NodeHandle, RclrsError,
     ENTITY_LIFECYCLE_MUTEX,
 };
 use std::{
@@ -19,7 +19,7 @@ unsafe impl Send for rcl_action_client_t {}
 /// [1]: <https://doc.rust-lang.org/reference/destructors.html>
 pub struct ActionClientHandle {
     rcl_action_client: Mutex<rcl_action_client_t>,
-    node: Node,
+    node_handle: Arc<NodeHandle>,
     pub(crate) in_use_by_wait_set: Arc<AtomicBool>,
 }
 
@@ -32,7 +32,7 @@ impl ActionClientHandle {
 impl Drop for ActionClientHandle {
     fn drop(&mut self) {
         let rcl_action_client = self.rcl_action_client.get_mut().unwrap();
-        let mut rcl_node = self.node.handle.rcl_node.lock().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.
@@ -62,16 +62,41 @@ pub(crate) enum ReadyMode {
     ResultResponse,
 }
 
-pub struct ActionClient<ActionT>
+///
+/// Main class responsible for sending goals to a ROS action server.
+///
+/// Create a client using [`Node::create_action_client`][1].
+///
+/// Receiving feedback and results requires the node's executor to [spin][2].
+///
+/// [1]: crate::NodeState::create_action_client
+/// [2]: crate::spin
+pub type ActionClient<ActionT> = Arc<ActionClientState<ActionT>>;
+
+/// The inner state of an [`ActionClient`].
+///
+/// This is public so that you can choose to create a [`Weak`][1] reference to it
+/// if you want to be able to refer to an [`ActionClient`] in a non-owning way. It is
+/// generally recommended to manage the `ActionClientState` inside of an [`Arc`],
+/// and [`ActionClient`] is provided as a convenience alias for that.
+///
+/// The public API of the [`ActionClient`] type is implemented via `ActionClientState`.
+///
+/// [1]: std::sync::Weak
+pub struct ActionClientState<ActionT>
 where
     ActionT: rosidl_runtime_rs::Action,
 {
     _marker: PhantomData<fn() -> ActionT>,
     pub(crate) handle: Arc<ActionClientHandle>,
     num_entities: WaitableNumEntities,
+    /// Ensure the parent node remains alive as long as the subscription is held.
+    /// This implementation will change in the future.
+    #[allow(unused)]
+    node: Node,
 }
 
-impl<T> ActionClient<T>
+impl<T> ActionClientState<T>
 where
     T: rosidl_runtime_rs::Action,
 {
@@ -97,7 +122,7 @@ where
 
             // SAFETY:
             // * The rcl_action_client was zero-initialized as expected by this function.
-            // * The rcl_node is kept alive by the Node because it is a dependency of the action client.
+            // * The rcl_node is kept alive by the NodeHandle because it is a dependency of the action 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
@@ -116,7 +141,7 @@ where
 
         let handle = Arc::new(ActionClientHandle {
             rcl_action_client: Mutex::new(rcl_action_client),
-            node: node.clone(),
+            node_handle: Arc::clone(&node.handle),
             in_use_by_wait_set: Arc::new(AtomicBool::new(false)),
         });
 
@@ -137,6 +162,7 @@ where
             _marker: Default::default(),
             handle,
             num_entities,
+            node: Arc::clone(node),
         })
     }
 
@@ -161,7 +187,7 @@ where
     }
 }
 
-impl<T> ActionClientBase for ActionClient<T>
+impl<T> ActionClientBase for ActionClientState<T>
 where
     T: rosidl_runtime_rs::Action,
 {

rclrs/src/action/server.rs

@@ -3,7 +3,7 @@ use crate::{
     error::{RclReturnCode, ToResult},
     rcl_bindings::*,
     wait::WaitableNumEntities,
-    Clock, DropGuard, Node, RclrsError, ENTITY_LIFECYCLE_MUTEX,
+    Clock, DropGuard, Node, NodeHandle, RclrsError, ENTITY_LIFECYCLE_MUTEX,
 };
 use rosidl_runtime_rs::{Action, ActionImpl, Message, Service};
 use std::{
@@ -23,7 +23,7 @@ unsafe impl Send for rcl_action_server_t {}
 /// [1]: <https://doc.rust-lang.org/reference/destructors.html>
 pub struct ActionServerHandle {
     rcl_action_server: Mutex<rcl_action_server_t>,
-    node: Node,
+    node_handle: Arc<NodeHandle>,
     pub(crate) in_use_by_wait_set: Arc<AtomicBool>,
 }
 
@@ -36,7 +36,7 @@ impl ActionServerHandle {
 impl Drop for ActionServerHandle {
     fn drop(&mut self) {
         let rcl_action_server = self.rcl_action_server.get_mut().unwrap();
-        let mut rcl_node = self.node.handle.rcl_node.lock().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.
@@ -69,7 +69,33 @@ pub type GoalCallback<ActionT> = dyn Fn(GoalUuid, <ActionT as rosidl_runtime_rs:
 pub type CancelCallback<ActionT> = dyn Fn(Arc<ServerGoalHandle<ActionT>>) -> CancelResponse + 'static + Send + Sync;
 pub type AcceptedCallback<ActionT> = dyn Fn(Arc<ServerGoalHandle<ActionT>>) + 'static + Send + Sync;
 
-pub struct ActionServer<ActionT>
+/// An action server that can respond to requests sent by ROS action clients.
+///
+/// Create an action server using [`Node::create_action_server`][1].
+///
+/// ROS only supports having one server for any given fully-qualified
+/// action name. "Fully-qualified" means the namespace is also taken into account
+/// for uniqueness. A clone of an `ActionServer` will refer to the same server
+/// instance as the original. The underlying instance is tied to [`ActionServerState`]
+/// which implements the [`ActionServer`] API.
+///
+/// Responding to requests requires the node's executor to [spin][2].
+///
+/// [1]: crate::NodeState::create_action_server
+/// [2]: crate::spin
+pub type ActionServer<ActionT> = Arc<ActionServerState<ActionT>>;
+
+/// The inner state of an [`ActionServer`].
+///
+/// This is public so that you can choose to create a [`Weak`][1] reference to it
+/// if you want to be able to refer to a [`ActionServer`] in a non-owning way. It is
+/// generally recommended to manage the `ActionServerState` inside of an [`Arc`],
+/// and [`ActionServer`] is provided as a convenience alias for that.
+///
+/// The public API of the [`ActionServer`] type is implemented via `ActionServerState`.
+///
+/// [1]: std::sync::Weak
+pub struct ActionServerState<ActionT>
 where
     ActionT: rosidl_runtime_rs::Action + rosidl_runtime_rs::ActionImpl,
 {
@@ -83,16 +109,19 @@ where
     goal_handles: Mutex<HashMap<GoalUuid, Arc<ServerGoalHandle<ActionT>>>>,
     goal_results: Mutex<HashMap<GoalUuid, <<ActionT::GetResultService as Service>::Response as Message>::RmwMsg>>,
     result_requests: Mutex<HashMap<GoalUuid, Vec<rmw_request_id_t>>>,
+    /// Ensure the parent node remains alive as long as the subscription is held.
+    /// This implementation will change in the future.
+    #[allow(unused)]
+    node: Node,
 }
 
-impl<T> ActionServer<T>
+impl<T> ActionServerState<T>
 where
     T: rosidl_runtime_rs::Action + rosidl_runtime_rs::ActionImpl,
 {
     /// Creates a new action server.
     pub(crate) fn new(
         node: &Node,
-        clock: Clock,
         topic: &str,
         goal_callback: impl Fn(GoalUuid, T::Goal) -> GoalResponse + 'static + Send + Sync,
         cancel_callback: impl Fn(Arc<ServerGoalHandle<T>>) -> CancelResponse + 'static + Send + Sync,
@@ -114,13 +143,14 @@ where
 
         {
             let mut rcl_node = node.handle.rcl_node.lock().unwrap();
+            let clock = node.get_clock();
             let rcl_clock = clock.rcl_clock();
             let mut rcl_clock = rcl_clock.lock().unwrap();
             let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
 
             // SAFETY:
             // * The rcl_action_server is zero-initialized as mandated by this function.
-            // * The rcl_node is kept alive by the Node because it is a dependency of the action server.
+            // * The rcl_node is kept alive by the NodeHandle because it is a dependency of the action server.
             // * 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
@@ -140,7 +170,7 @@ where
 
         let handle = Arc::new(ActionServerHandle {
             rcl_action_server: Mutex::new(rcl_action_server),
-            node: node.clone(),
+            node_handle: Arc::clone(&node.handle),
             in_use_by_wait_set: Arc::new(AtomicBool::new(false)),
         });
 
@@ -166,6 +196,7 @@ where
             goal_handles: Mutex::new(HashMap::new()),
             goal_results: Mutex::new(HashMap::new()),
             result_requests: Mutex::new(HashMap::new()),
+            node: node.clone(),
         })
     }
 
@@ -684,7 +715,7 @@ where
     }
 }
 
-impl<T> ActionServerBase for ActionServer<T>
+impl<T> ActionServerBase for ActionServerState<T>
 where
     T: rosidl_runtime_rs::Action + rosidl_runtime_rs::ActionImpl,
 {

rclrs/src/action/server_goal_handle.rs

@@ -1,4 +1,4 @@
-use crate::{action::ActionServer, rcl_bindings::*, GoalUuid, RclrsError, ToResult};
+use crate::{action::ActionServerState, rcl_bindings::*, GoalUuid, RclrsError, ToResult};
 use std::sync::{Arc, Mutex, Weak};
 
 // Values defined by `action_msgs/msg/GoalStatus`
@@ -25,7 +25,7 @@ where
     ActionT: rosidl_runtime_rs::Action + rosidl_runtime_rs::ActionImpl,
 {
     rcl_handle: Mutex<*mut rcl_action_goal_handle_t>,
-    action_server: Weak<ActionServer<ActionT>>,
+    action_server: Weak<ActionServerState<ActionT>>,
     goal_request: Arc<ActionT::Goal>,
     uuid: GoalUuid,
 }
@@ -44,7 +44,7 @@ where
 {
     pub(crate) fn new(
         rcl_handle: *mut rcl_action_goal_handle_t,
-        action_server: Weak<ActionServer<ActionT>>,
+        action_server: Weak<ActionServerState<ActionT>>,
         goal_request: Arc<ActionT::Goal>,
         uuid: GoalUuid,
     ) -> Self {

rclrs/src/node.rs

@@ -19,13 +19,13 @@ use std::{
 use rosidl_runtime_rs::Message;
 
 use crate::{
-    rcl_bindings::*, ActionClient, ActionClientBase, ActionServer, ActionServerBase,
-    CancelResponse, Client, ClientBase, ClientOptions, ClientState, Clock, ContextHandle,
-    GoalResponse, GoalUuid, GuardCondition, LogParams, Logger, ParameterBuilder,
-    ParameterInterface, ParameterVariant, Parameters, Publisher, PublisherOptions, PublisherState,
-    RclrsError, ServerGoalHandle, Service, ServiceBase, ServiceOptions, ServiceState, Subscription,
-    SubscriptionBase, SubscriptionCallback, SubscriptionOptions, SubscriptionState, TimeSource,
-    ToLogParams, ENTITY_LIFECYCLE_MUTEX,
+    rcl_bindings::*, ActionClient, ActionClientBase, ActionClientState, ActionServer,
+    ActionServerBase, ActionServerState, CancelResponse, Client, ClientBase, ClientOptions,
+    ClientState, Clock, ContextHandle, GoalResponse, GoalUuid, GuardCondition, LogParams, Logger,
+    ParameterBuilder, ParameterInterface, ParameterVariant, Parameters, Publisher,
+    PublisherOptions, PublisherState, RclrsError, ServerGoalHandle, Service, ServiceBase,
+    ServiceOptions, ServiceState, Subscription, SubscriptionBase, SubscriptionCallback,
+    SubscriptionOptions, SubscriptionState, TimeSource, ToLogParams, ENTITY_LIFECYCLE_MUTEX,
 };
 
 // SAFETY: The functions accessing this type, including drop(), shouldn't care about the thread
@@ -299,11 +299,11 @@ impl NodeState {
     pub fn create_action_client<T>(
         self: &Arc<Self>,
         topic: &str,
-    ) -> Result<Arc<ActionClient<T>>, RclrsError>
+    ) -> Result<ActionClient<T>, RclrsError>
     where
         T: rosidl_runtime_rs::Action,
     {
-        let action_client = Arc::new(ActionClient::<T>::new(self, topic)?);
+        let action_client = Arc::new(ActionClientState::<T>::new(self, topic)?);
         self.action_clients_mtx
             .lock()
             .unwrap()
@@ -321,16 +321,15 @@ impl NodeState {
         handle_goal: GoalCallback,
         handle_cancel: CancelCallback,
         handle_accepted: AcceptedCallback,
-    ) -> Result<Arc<ActionServer<ActionT>>, RclrsError>
+    ) -> Result<ActionServer<ActionT>, RclrsError>
     where
         ActionT: rosidl_runtime_rs::Action + rosidl_runtime_rs::ActionImpl,
         GoalCallback: Fn(GoalUuid, <ActionT as rosidl_runtime_rs::Action>::Goal) -> GoalResponse + 'static + Send + Sync,
         CancelCallback: Fn(Arc<ServerGoalHandle<ActionT>>) -> CancelResponse + 'static + Send + Sync,
         AcceptedCallback: Fn(Arc<ServerGoalHandle<ActionT>>) + 'static + Send + Sync,
     {
-        let action_server = Arc::new(ActionServer::<ActionT>::new(
+        let action_server = Arc::new(ActionServerState::<ActionT>::new(
             self,
-            self.get_clock(),
             topic,
             handle_goal,
             handle_cancel,