fix(Executor): Fix segfault if callback group is deleted during rmw_wait #2683
Conversation
jmachowinski
requested review from
hidmic,
ivanpauno and
wjwwood
as code owners
|
Not ideal, but does the job. Thanks. |
jmachowinski
force-pushed
the
seg_fault_rolling
branch
from
c42898b to
c58e8e7
Compare
|
We talked about a potentially better fix for this in rolling (versus Jazzy) at the client library working group, but I would be ok with a temporary fix in rolling as well. I also mentioned on the jazzy fix that it would be nice to have a regression test of some sort, but that's more important for rolling than jazzy imo. |
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI, unit test would be ideal.
| // we need to make sure that callback groups don't get out of scope | ||
| // during the wait. As in jazzy, they are not covered by the DynamicStorage, | ||
| // we explicitly hold them here as a bugfix |
There was a problem hiding this comment.
i would add TODO if we already have a plan to proper fix in the future not to miss, this does not block the PR.
jmachowinski
force-pushed
the
seg_fault_rolling
branch
from
c58e8e7 to
37d214f
Compare
|
Added unit test and fixed the same problem in the StaticSingleThreadedExecutor. |
Signed-off-by: Janosch Machowinski <[email protected]>
jmachowinski
force-pushed
the
seg_fault_rolling
branch
from
37d214f to
86d8313
Compare
|
Pulls: #2683
|
|
https://ci.ros2.org/job/ci_windows/22936/msbuild/new/ are unrelated. |
…ait (ros2#2683) Signed-off-by: Janosch Machowinski <[email protected]> Signed-off-by: HarunTeper <[email protected]>
…ait (ros2#2683) Signed-off-by: Janosch Machowinski <[email protected]> Signed-off-by: HarunTeper <[email protected]>
…ait (ros2#2683) Signed-off-by: Janosch Machowinski <[email protected]>
Signed-off-by: Janosch Machowinski <[email protected]>
7 participants
Fix for #2664 in rolling
Note, this is not my preferred fix for this. But I would like to merge this now,
to get a fix into jazzy ASAP.
@alsora @mjcarroll fyi