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.

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

Log missing pairs only once #397

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

Conversation

Timple
Copy link
Contributor

@Timple Timple commented Mar 3, 2023

Most topics will keep existing. So getting this log message every second is not helpful.

@Timple
Copy link
Contributor Author

Timple commented Mar 3, 2023

Error: The process '/usr/bin/sudo' failed with exit code 100

Build failure seems unrelated?

Signed-off-by: Tim Clephas <[email protected]>
@Timple
Copy link
Contributor Author

Timple commented Apr 3, 2023

apt-get update was flaky. Can someone with the correct rights re-trigger CI?

@clalancette
Copy link
Contributor

@ros-pull-request-builder retest this please

@Timple
Copy link
Contributor Author

Timple commented Apr 3, 2023

Thank you, fails on the same error. It seems like the main branch is also failing for half a year already.

Probably because the github actions use ubuntu-latest which perhaps changed to jammy?

@LucasHaug
Copy link

LucasHaug commented Apr 3, 2023

Thank you, fails on the same error. It seems like the main branch is also failing for half a year already.

Probably because the github actions use ubuntu-latest which perhaps changed to jammy?

I opened the PR #398 to fix it, just waiting for review. But yeah, I think the problem is using ubuntu-latest.

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.

Comment on lines +196 to +197
auto entry = logged_topic_errors.emplace(topic_name);
if (entry.second) { // topic name was not already in the set, so log once
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

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.

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.

4 participants