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(start/goal_planner): fix multi thread memory crash #6322

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

kosuke55
Copy link
Contributor

@kosuke55 kosuke55 commented Feb 5, 2024

Description

Wait for the multithreaded callback to finish in the destructor for
#5154

Tests performed

psim

evaluator_description: fix/start_goal_multithread
2024/02/06 https://evaluation.tier4.jp/evaluation/reports/a099d691-034b-5484-a751-547fe6c34756/?project_id=prd_jt

evaluator_description: fix/start_goal_multithread
2024/02/06 https://evaluation.tier4.jp/evaluation/reports/18277ac1-dd2a-5069-bced-148402a874bf/?project_id=prd_jt

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added the component:planning Route planning, decision-making, and navigation. (auto-assigned) label Feb 5, 2024
@kosuke55 kosuke55 added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Feb 5, 2024
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (bd4b5ca) 14.36% compared to head (1ab5eb2) 14.35%.
Report is 15 commits behind head on main.

Files Patch % Lines
...r_path_goal_planner_module/goal_planner_module.hpp 0.00% 16 Missing ⚠️
...path_start_planner_module/start_planner_module.hpp 0.00% 9 Missing ⚠️
..._start_planner_module/src/start_planner_module.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6322      +/-   ##
==========================================
- Coverage   14.36%   14.35%   -0.02%     
==========================================
  Files        1907     1907              
  Lines      130136   130272     +136     
  Branches    37640    37640              
==========================================
  Hits        18697    18697              
- Misses      90430    90566     +136     
  Partials    21009    21009              
Flag Coverage Δ *Carryforward flag
differential 0.00% <0.00%> (?)
total 14.36% <ø> (+<0.01%) ⬆️ Carriedforward from bd4b5ca

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VRichardJP
Copy link
Contributor

Feels a bit over engineered, no? If the goal is to make sure 2 callbacks are not executed at the same time, then why not simply use the same callback queue for both?

Signed-off-by: kosuke55 <kosuke.tnp@gmail.com>
@kosuke55
Copy link
Contributor Author

kosuke55 commented Feb 6, 2024

@VRichardJP
thanks for checking the PR.

Since path candidates generation in this module is time consuming, it is done in a separate thread from the main thread called by run from manager.

Concurrency is desirable because we want to generate paths without stopping the main thread.
However, it has not been verified whether anything other than the main thread is running when manager deletes an planner instance.
This PR ensures that when deleting the instance, the other threads processes are finished.

https://autowarefoundation.github.io/autoware.universe/main/planning/behavior_path_goal_planner_module/#pull-over
image

Copy link
Contributor

@kyoichi-sugahara kyoichi-sugahara left a comment

Choose a reason for hiding this comment

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

LGTM!!!
Super thanks.

@kosuke55 kosuke55 merged commit c897b3d into autowarefoundation:main Feb 6, 2024
22 of 25 checks passed
@kosuke55 kosuke55 deleted the fix/start_goal_multithread branch February 6, 2024 12:34
kosuke55 added a commit to tier4/autoware.universe that referenced this pull request Feb 6, 2024
anhnv3991 pushed a commit to anhnv3991/autoware.universe that referenced this pull request Feb 13, 2024
StepTurtle pushed a commit to StepTurtle/autoware.universe that referenced this pull request Feb 28, 2024
karishma1911 pushed a commit to Interplai/autoware.universe that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:planning Route planning, decision-making, and navigation. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants