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

Fix race condition when declaring parameters #525

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

JafarAbdi
Copy link
Contributor

Description

Fix: #474

Not an optimal solution at all, but at least it works :D

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@tylerjw
Copy link
Member

tylerjw commented Jun 30, 2021

Why would a node have already declared these parameters?

@JafarAbdi
Copy link
Contributor Author

Why would a node have already declared these parameters?

Not 100% sure, but I think this mainly happens for RViz since each display will load its own robot model and they all share the same node causing multiple declarations

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #525 (0924589) into main (656180d) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 0924589 differs from pull request most recent head da2f124. Consider uploading reports for the commit da2f124 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #525      +/-   ##
==========================================
- Coverage   54.24%   54.18%   -0.06%     
==========================================
  Files         192      192              
  Lines       20224    20187      -37     
==========================================
- Hits        10969    10936      -33     
+ Misses       9255     9251       -4     
Impacted Files Coverage Δ
...eit_core/robot_trajectory/src/robot_trajectory.cpp 15.75% <0.00%> (-7.38%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 74.72% <0.00%> (-1.13%) ⬇️
...bot_state/include/moveit/robot_state/robot_state.h 88.89% <0.00%> (-1.05%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 45.49% <0.00%> (-0.32%) ⬇️
...anning_scene_monitor/src/current_state_monitor.cpp 74.18% <0.00%> (-0.22%) ⬇️
...it/planning_scene_monitor/planning_scene_monitor.h 33.34% <0.00%> (ø)
...on_distance_field/collision_distance_field_types.h 61.06% <0.00%> (ø)
moveit_ros/moveit_servo/src/servo_node.cpp
moveit_ros/moveit_servo/src/servo_server.cpp 78.95% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 656180d...da2f124. Read the comment docs.

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

So... RViz is initializing the plugins in separate threads with the same node instance? Well.. this is definitely another requirement for our WIP parameter API

@v4hn
Copy link
Contributor

v4hn commented Jun 30, 2021

We actually have interfaces for sharing a RobotModel(Loader) in static memory.
Wouldn't it make sense to migrate the plugins to use moveit::planning_interface::getSharedRobotModel() instead of fixing the racing condition via catching exceptions?

@JafarAbdi
Copy link
Contributor Author

I ended up fixing it by adding a shared robot model loaders, hopefully, this's a better solution than the previous one

@JafarAbdi JafarAbdi force-pushed the pr-fix_race_condition branch 2 times, most recently from 5414a12 to ef61ac3 Compare September 20, 2021 15:33
@henningkayser henningkayser moved this from In Progress to Review in progress in Galactic/Rolling - 2.3.0 - September 10 Sep 21, 2021
Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Solution looks good to me. Please verify if we should or should not load IK solvers for the shared robot model

@JafarAbdi JafarAbdi merged commit 792ce15 into moveit:main Oct 7, 2021
Galactic/Rolling - 2.3.0 - September 10 automation moved this from Review in progress to Done Oct 7, 2021
JafarAbdi added a commit to JafarAbdi/moveit2 that referenced this pull request Nov 4, 2021
JafarAbdi added a commit that referenced this pull request Nov 8, 2021
* Add getSharedRobotModelLoader to fix race condition when having multiple displays for the same node (#525)

* common_objects: getSharedRobotModelLoader fix deadlock (#734)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

MotionPlanningDisplay not loading initially, requires reset
4 participants