Skip to content

Fix race condition when declaring parameters#525

Merged
JafarAbdi merged 1 commit intomoveit:mainfrom
JafarAbdi:pr-fix_race_condition
Oct 7, 2021
Merged

Fix race condition when declaring parameters#525
JafarAbdi merged 1 commit intomoveit:mainfrom
JafarAbdi:pr-fix_race_condition

Conversation

@JafarAbdi
Copy link
Copy Markdown
Member

@JafarAbdi JafarAbdi commented Jun 30, 2021

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
Copy Markdown
Member

tylerjw commented Jun 30, 2021

Why would a node have already declared these parameters?

@JafarAbdi
Copy link
Copy Markdown
Member Author

JafarAbdi commented Jun 30, 2021

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
Copy Markdown

codecov bot commented Jun 30, 2021 •
edited
Loading

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.

henningkayser
henningkayser reviewed Jun 30, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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 JafarAbdi mentioned this pull request Aug 13, 2021
Closed
@tylerjw tylerjw mentioned this pull request Aug 17, 2021
Open
@JafarAbdi JafarAbdi force-pushed the pr-fix_race_condition branch from 1237961 to a768e9e Compare September 20, 2021 15:06
@JafarAbdi
Copy link
Copy Markdown
Member Author

JafarAbdi commented Sep 20, 2021

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 reviewed Sep 29, 2021
View reviewed changes
Copy link
Copy Markdown
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 force-pushed the pr-fix_race_condition branch from 0924589 to da2f124 Compare October 7, 2021 13:36
@JafarAbdi JafarAbdi merged commit 792ce15 into moveit:main Oct 7, 2021
@JafarAbdi JafarAbdi mentioned this pull request Oct 11, 2021
Merged
6 tasks
@JafarAbdi JafarAbdi mentioned this pull request Nov 4, 2021
Closed
JafarAbdi added a commit to JafarAbdi/moveit2 that referenced this pull request Nov 4, 2021
@JafarAbdi
Add getSharedRobotModelLoader to fix race condition when having multi…
b800a75 
…ple displays for the same node (moveit#525)
JafarAbdi added a commit that referenced this pull request Nov 8, 2021
@JafarAbdi
[foxy]: Backport fixes to race-condition causing exceptions (#782)
fd9a562 
* 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

Reviewers

@henningkayser henningkayser henningkayser approved these changes

@v4hn v4hn Awaiting requested review from v4hn

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MotionPlanningDisplay not loading initially, requires reset

4 participants

@JafarAbdi @tylerjw @v4hn @henningkayser