Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions rclrs/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl Drop for ClientHandle {
// 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);
rcl_client_fini(rcl_client, &mut **rcl_node);
}
}
}
Expand Down Expand Up @@ -115,7 +115,7 @@ where
unsafe {
rcl_client_init(
&mut rcl_client,
&*rcl_node,
&**rcl_node,
type_support,
topic_c_string.as_ptr(),
&client_options,
Expand Down Expand Up @@ -263,7 +263,7 @@ where
pub fn service_is_ready(&self) -> Result<bool, RclrsError> {
let mut is_ready = false;
let client = &mut *self.handle.rcl_client.lock().unwrap();
let node = &mut *self.handle.node_handle.rcl_node.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
Expand Down
26 changes: 26 additions & 0 deletions rclrs/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ impl Drop for rcl_context_t {
// 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);

// SAFETY: No preconditions for rcl_logging_fini
rcl_logging_fini();

rcl_context_fini(self);
}
}
Expand All @@ -56,6 +60,10 @@ unsafe impl Send for rcl_context_t {}
/// - middleware-specific data, e.g. the domain participant in DDS
/// - the allocator used (left as the default by `rclrs`)
///
/// The context also configures the rcl_logging_* layer to allow publication to /rosout
/// (as well as the terminal). TODO: This behaviour should be configurable using an
/// "auto logging initialise" flag as per rclcpp and rclpy.
///
pub struct Context {
pub(crate) handle: Arc<ContextHandle>,
}
Expand Down Expand Up @@ -142,6 +150,24 @@ impl Context {
rcl_init_options_fini(&mut rcl_init_options).ok()?;
// Move the check after the last fini()
ret?;

// TODO: "Auto set-up logging" is forced but should be configurable as per rclcpp and rclpy
// SAFETY:
// * Lock the mutex as we cannot guarantee that rcl_* functions are protecting their global variables
// * Context is validated before we reach this point
// * No other preconditions for calling this function
let ret = {
let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
// Note: shadowing the allocator above. The rcutils_get_default_allocator provides us with mechanisms for allocating and freeing
// memory, e.g. calloc/free. As these are function pointers and will be the same each time we call the method, it is safe to
// perform this shadowing
// Alternate is to call rcl_init_options_get_allocator however, we've freed the allocator above and restructuring the call
// sequence is unnecessarily complex
let allocator = rcutils_get_default_allocator();

rcl_logging_configure(&rcl_context.global_arguments, &allocator).ok()
Copy link
Collaborator

@mxgrey mxgrey Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will get called every time a Context is created, but it's meant to be called only once. It's possible for users to create multiple Contexts in one application (we do this a lot in tests), so we'd be making redundant calls.

Similar to ros2/rclcpp#998 we should guard this from being called multiple times. We also need to make sure that rcl_logging_fini is only called after all Contexts are destructed.

This is a tricky problem to resolve in a sound way. I might suggest something like this:

struct LoggingConfiguration {
    lifecycle: Mutex<Weak<LoggingLifecycle>>
}

impl LoggingConfiguration {
    fn configure(args: &rcl_arguments_t) -> Result<Arc<LoggingLifecycle>, RclError> {
        static CONFIGURATION: LazyLock<Self> = LazyLock::new(|| Self { lifecycle: Mutex::new(Weak::new()) });
        let mut lifecycle = CONFIGURATION.lifecycle.lock().unwrap();
        if let Some(arc_lifecycle) = lifecycle.upgrade() {
            return Ok(arc_lifecycle);
        }
        let arc_lifecycle = Arc::new(LoggingLifecycle::new(args)?);
        *lifecycle = arc_lifecycle.downgrade();
        Ok(arc_lifecycle);
    }
}

struct LoggingLifecycle;
impl LoggingLifecycle {
    fn new(args: &rcl_arguments_t) -> Result<Self, RclError> {
        let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
        let allocator = rcutils_get_default_allocator();
        rcl_logging_configure(args, &allocator).ok()?;
        Self
    }
}

impl Drop for LoggingLifecycle {
    fn drop(&mut self) {
        let _lifecycle_lock = ENTITY_LIFECYCLE_MUTEX.lock().unwrap();
        rcl_logging_fini();
    }
}

Then we would add an Arc<LoggingLifecycle> to the ContextHandle. Once all ContextHandles are gone, the LoggingLifecycle will drop and call rcl_logging_fini. If a new context is made after that, rcl_logging_configure will be called again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good pick-up and suggestion @mxgrey - thank you. Will update the code to make use of your suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the liberty of opening a PR for this: geoff-imdex#6

It's almost exactly what I recommended above.

};
ret?
}
Ok(Self {
handle: Arc::new(ContextHandle {
Expand Down
2 changes: 2 additions & 0 deletions rclrs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ mod clock;
mod context;
mod error;
mod executor;
mod logging;
mod node;
mod parameter;
mod publisher;
Expand Down Expand Up @@ -38,6 +39,7 @@ pub use clock::*;
pub use context::*;
pub use error::*;
pub use executor::*;
pub use logging::*;
pub use node::*;
pub use parameter::*;
pub use publisher::*;
Expand Down
Loading
Loading