Skip to content

set thread names by node in component container isolated #2871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: rolling
Choose a base branch
from

Conversation

Aposhian
Copy link
Contributor

Description

Set the thread name of each node's thread in component_container_isolated. This should support POSIX platforms, Apple, and Windows. I tested on Linux only so far.

Fixes #2818

Is this user-facing behavior change?

User's may see an additional warning if the thread name couldn't set for whatever reason. If users are looking at custom thread names, then they will see a difference.

Did you use Generative AI?

Yes with Cursor autocomplete.

Additional Information

Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

is there any way to add a test?

Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
@Aposhian Aposhian force-pushed the 2818-custom-thread-names branch from 165183b to 3967b72 Compare June 11, 2025 16:11
@Aposhian
Copy link
Contributor Author

Aposhian commented Jun 11, 2025

  • Add test

@Aposhian Aposhian force-pushed the 2818-custom-thread-names branch from 6fdf8e5 to 27dfcd2 Compare June 11, 2025 17:19
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
@Aposhian Aposhian force-pushed the 2818-custom-thread-names branch from 27dfcd2 to cc18cde Compare June 11, 2025 17:19
@Aposhian
Copy link
Contributor Author

Ok, I added a get_thread_name method, as well as tests for each platform. I have only tested the Linux test so far.

Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
@Aposhian Aposhian force-pushed the 2818-custom-thread-names branch from 5f02599 to 26a8a9b Compare June 11, 2025 18:19
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
@@ -0,0 +1,42 @@
// Copyright 2025 Open Source Robotics Foundation, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

1st of all, i would highly recommend to move this APIs to rcpputils that is where those utilities are maintained in c++ language. so that the user application can use those utilities without rclcpp dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

  • move os_thread to rcpputils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will resolve the requested changes here first, and then move it over.

rc = pthread_setname_np(pthread_self(), truncated_name.c_str());
#endif
if (rc != 0) {
// Don't throw since this is not critical
Copy link
Collaborator

Choose a reason for hiding this comment

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

indent is off?

Suggested change
// Don't throw since this is not critical
// Don't throw since this is not critical

#endif
if (rc != 0) {
// Don't throw since this is not critical
RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set thread name: %s", name.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about calling strerror(errno) to print more information from the system?

Suggested change
RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set thread name: %s", name.c_str());
RCLCPP_WARN(rclcpp::get_logger("rclcpp"), "Failed to set thread name(%s): %s", name.c_str(), strerror(errno));

Copy link
Contributor Author

@Aposhian Aposhian Jun 16, 2025

Choose a reason for hiding this comment

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

  • print OS error information

@@ -77,8 +77,11 @@ class ComponentManagerIsolated : public rclcpp_components::ComponentManager
DedicatedExecutorWrapper & wrapper = result.first->second;
wrapper.executor = exec;
auto & thread_initialized = wrapper.thread_initialized;
auto name = node_wrappers_[node_id].get_node_base_interface()->get_name();
Copy link
Collaborator

Choose a reason for hiding this comment

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

we would want to add the docstring about this behavior including truncating the node name into 16bytes depending on the platform? it also can mention that if using MultiThreadedExecutor, all the threads in that will be named same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean to the docstring of set_thread_name or elsewhere?

  • Document truncation behavior

Comment on lines 49 to 52
// Truncate name to maximum length supported by pthread_setname_np
// leaving one character for the null terminator
// otherwise pthread_setname_np will return an ERANGE error
std::string truncated_name = name.substr(0, MAXTHREADNAMESIZE - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be still better to add the warning if truncation happens on this call? so that user application can know the actual thread name to be set to the Executor.

Copy link
Contributor Author

@Aposhian Aposhian Jun 16, 2025

Choose a reason for hiding this comment

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

That's a good idea, to log the actual name set if it was truncated.

  • log truncated name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up removing the log of the truncated name since I wasn't sure how to go about that inside of the rcpputils project. Any suggestions?

Aposhian added 2 commits June 16, 2025 08:04
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
@Aposhian
Copy link
Contributor Author

I copied the thread name utilities and test over to rcpputils: ros2/rcpputils#213.

Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
[exec, &thread_initialized, name]() {
try {
rcpputils::set_thread_name(name);
} catch (const std::system_error & e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

include <system_error>

Copy link
Contributor Author

@Aposhian Aposhian Jun 17, 2025

Choose a reason for hiding this comment

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

  • include system error

Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding custom thread names to component_container_isolated
3 participants