Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Log missing pairs only once #397

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Log missing pairs only once #397

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 22 additions & 14 deletions src/dynamic_bridge.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,17 @@ void update_bridge(
bridge.ros1_type_name, topic_name, 10,
bridge.ros2_type_name, topic_name, ros2_publisher_qos);
} catch (std::runtime_error & e) {
fprintf(
stderr,
"failed to create 1to2 bridge for topic '%s' "
"with ROS 1 type '%s' and ROS 2 type '%s': %s\n",
topic_name.c_str(), bridge.ros1_type_name.c_str(), bridge.ros2_type_name.c_str(), e.what());
if (std::string(e.what()).find("No template specialization") != std::string::npos) {
fprintf(stderr, "check the list of supported pairs with the `--print-pairs` option\n");
static std::set<std::string> logged_topic_errors;
Copy link
Collaborator

Choose a reason for hiding this comment

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

having this static global variable is safe since both ros1 and ros2 uses single thread to poll the process.

auto entry = logged_topic_errors.emplace(topic_name);
if (entry.second) { // topic name was not already in the set, so log once
Comment on lines +196 to +197
Copy link
Collaborator

Choose a reason for hiding this comment

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

if create -> remove -> create case on the same topic name, this will never print the error on 2nd trial since the element will never be pulled out when the topic becomes unavailable.
besides, this just increase the element in this global space as long as this process is running, could be memory leak problem.

Copy link
Contributor Author

@Timple Timple Apr 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if create -> remove -> create case on the same topic name, this will never print the error on 2nd trial since the element will never be pulled out when the topic becomes unavailable.

This was not meant to be supported. Since publishers can be created on demand (actually are a lot in our case). Topics become available and unavailable more often.

could be memory leak problem.

Yes, technically. But the memory will be limited by the amount of unique topics which fail. Which I assume have a limit.

I tried keeping the solution locally to the logging statement. Same kind of implementation that ROS_INFO_ONCE uses with the static variable.

Let me know if you would like a solution which is kept in sync with the publishers and subscribers. In which case the administration will probably passed up and down through functions.

fprintf(
stderr,
"failed to create 1to2 bridge for topic '%s' "
"with ROS 1 type '%s' and ROS 2 type '%s': %s\n",
topic_name.c_str(), bridge.ros1_type_name.c_str(), bridge.ros2_type_name.c_str(), e.what());
if (std::string(e.what()).find("No template specialization") != std::string::npos) {
fprintf(stderr, "check the list of supported pairs with the `--print-pairs` option\n");
}
}
continue;
}
Expand Down Expand Up @@ -258,13 +262,17 @@ void update_bridge(
bridge.ros2_type_name, topic_name, 10,
bridge.ros1_type_name, topic_name, 10);
} catch (std::runtime_error & e) {
fprintf(
stderr,
"failed to create 2to1 bridge for topic '%s' "
"with ROS 2 type '%s' and ROS 1 type '%s': %s\n",
topic_name.c_str(), bridge.ros2_type_name.c_str(), bridge.ros1_type_name.c_str(), e.what());
if (std::string(e.what()).find("No template specialization") != std::string::npos) {
fprintf(stderr, "check the list of supported pairs with the `--print-pairs` option\n");
static std::set<std::string> logged_topic_errors;
auto entry = logged_topic_errors.emplace(topic_name);
if (entry.second) { // topic name was not already in the set, so log once
fprintf(
stderr,
"failed to create 2to1 bridge for topic '%s' "
"with ROS 2 type '%s' and ROS 1 type '%s': %s\n",
topic_name.c_str(), bridge.ros2_type_name.c_str(), bridge.ros1_type_name.c_str(), e.what());
if (std::string(e.what()).find("No template specialization") != std::string::npos) {
fprintf(stderr, "check the list of supported pairs with the `--print-pairs` option\n");
}
}
continue;
}
Expand Down