Fix invalid timer handle exception

[z80020100]

• This PR try to fix invalid timer handle exception caused by removing handle repeatedly.
• This issue is found when using Nav2 and set a small transform_tolerance for local_costmap and global_costmap.
• Specific error messages are as follows:

  [server-9] [planner_server-4] terminate called after throwing an instance of 'tf2_ros::InvalidTimerHandleException'
  [server-9] [planner_server-4]   what():  Invalid timer handle in remove()
  

• We found this exception is thrown from

  geometry2/tf2_ros/src/create_timer_ros.cpp

  Line 93 in 16562ce

  throw InvalidTimerHandleException("Invalid timer handle in remove()");

• If adding more messages for debug, we found that there is a high probability that handle will be removed repeatedly.

  [server-9] [planner_server-4] [INFO] [1636089969.121297887] [tf2_buffer]: [cliff] remove@217, it->first=286, this->timer_interface_=0x55eee89ff140
  [server-9] [planner_server-4] [INFO] [1636089969.126648738] [tf2_buffer]: [cliff] remove@286, timer_handle=286, timer_interface_=0x55eee89ff140
  [server-9] [planner_server-4] terminate called after throwing an instance of 'tf2_ros::InvalidTimerHandleException'
  [server-9] [planner_server-4]   what():  Invalid timer handle in remove()
  

• Therefore, we have made a change to ensure that timer_handle only can be removed when it can be found in timer_to_request_map_

[aprotyas]

Looks reasonable to me. Is it possible to add a test case that demonstrates this?

[z80020100]

How should I add a test case?

[aprotyas]

How should I add a test case?

Can you write a code snippet that uses the tf2_ros interface in a way that throws this timer handle exception without the patch (but not with it)? If so, that snippet should may live as a test case in tf2_ros/test/test_buffer.cpp.

[z80020100]

Thank you for the feedback. I will try to write a test case and push it later.

[aprotyas]

@z80020100 there are linter errors in the Rpr job, can you address them please?
https://build.ros2.org/job/Rpr__geometry2__ubuntu_focal_amd64/372/testReport/

[z80020100]

@aprotyas Sure, I will fix them.

[cmraaron]

status: works for me 👍

[swarajpeppermint]

Will this be merged into humble? @aprotyas I'm sure other people will also face this issue (we did and we independently identified the same bug). If this is good, then we should merge this into the code base or at least mark it as an issue so that others can find this.

[aprotyas]

@swarajpeppermint thanks for flagging this. Since I’m not a maintainer I can’t actually merge the code, but I’m tagging @clalancette who can hopefully get this PR across the finish line

[clalancette]

I'm sorry for the very long delay here. In short, after looking at this more closely, I agree with this change. Thank you so much for adding the test case as well, that is very helpful.

I'm going to approve, rebase it onto the latest, and go ahead and run CI on it.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows

[cmraaron]

fantastic!, can we backport to iron and humble?

[ahcorde]

https://github.com/Mergifyio backport iron humble

[mergify]

backport iron humble

✅ Backports have been created

• #613 Fix invalid timer handle exception (backport #474) has been created for branch iron
• #614 Fix invalid timer handle exception (backport #474) has been created for branch humble

[tonynajjar]

This issue was independently reported to Nav2, ROS answers and Robotics Stack Exchange and I couldn't find a fix for a long time. Thanks for this fix, it was really a frequent problem for our production robots.

at least mark it as an issue

In retrospect, this would have probably been a good idea to help find this issue/PR more easily

[anath93]

@z80020100 @swarajpeppermint @clalancette Can this be backported to Galactic or Foxy ?

[ahcorde]

@anath93 Galactic and Foxy are EOL we are not merging more PRs in these branches.

[anath93]

@ahcorde these are the only LTS version on 20.04, there is still a lot of community and would be helpful for the community. Can this be considered still ?

[clalancette]

Can this be considered still ?

No, sorry. We have no way to deliver fixes for those distributions anymore.