-
Notifications
You must be signed in to change notification settings - Fork 344
Use filelock in spawner to avoid concurrent operations #2202
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
Use filelock in spawner to avoid concurrent operations #2202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this PR on a robotic platform with 15 controllers spawning. The master version failed 20% of the times. With this PR there was 50/50 success rate.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2202 +/- ##
==========================================
- Coverage 89.14% 89.09% -0.05%
==========================================
Files 139 139
Lines 16124 16142 +18
Branches 1389 1390 +1
==========================================
+ Hits 14373 14381 +8
- Misses 1222 1229 +7
- Partials 529 532 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-authored-by: Sai Kishor Kothakota <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @saikishor and also @doterkuile for the notes on testing!
Use filelock to avoid concurrent operations when launching more spawners at system startup and this should make it more robust.
I've tested spawning 30+ controllers with ros2_control_demos branch
test/use/filelock/spawner
, with this fix it works 10 out of 10 times and without this, it didn't even work onceNeeds ros/rosdistro#45306