-
Notifications
You must be signed in to change notification settings - Fork 455
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
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
There was a problem hiding this 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>
165183b
to
3967b72
Compare
|
6fdf8e5
to
27dfcd2
Compare
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
27dfcd2
to
cc18cde
Compare
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>
5f02599
to
26a8a9b
Compare
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
@@ -0,0 +1,42 @@ | |||
// Copyright 2025 Open Source Robotics Foundation, Inc. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent is off?
// 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()); |
There was a problem hiding this comment.
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?
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)); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
// 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
Signed-off-by: Adam Aposhian <adam.aposhian@fireflyautomatix.com>
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include <system_error>
There was a problem hiding this comment.
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>
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