FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(Executor): Fix segfault if callback group is deleted during rmw_wait by jmachowinski · Pull Request #2683 · ros2/rclcpp · GitHub

/ rclcpp Public

fix(Executor): Fix segfault if callback group is deleted during rmw_wait - #2683

Merged
fujitatomoya merged 1 commit into
ros2:rollingfrom
cellumation:seg_fault_rolling
Nov 30, 2024
ConversationCommits (1)ChecksFiles changed
Merged

fix(Executor): Fix segfault if callback group is deleted during rmw_wait#2683
fujitatomoya merged 1 commit into
ros2:rollingfrom
cellumation:seg_fault_rolling

Conversation

jmachowinski commented Nov 22, 2024

Copy link
Copy Markdown
Collaborator

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

mjcarroll commented Nov 22, 2024

Copy link
Copy Markdown
Member

Not ideal, but does the job. Thanks.

christophebedard reviewed Nov 22, 2024
View reviewed changes
Comment thread rclcpp/src/rclcpp/executor.cpp Outdated

wjwwood commented Nov 22, 2024

Copy link
Copy Markdown
Member

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 approved these changes Nov 22, 2024
View reviewed changes

fujitatomoya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm with green CI, unit test would be ideal.

Comment thread rclcpp/src/rclcpp/executor.cpp
Comment on lines +732 to +734
// 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

fujitatomoya Nov 22, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

This was referenced Nov 22, 2024
Merged
Closed

jmachowinski commented Nov 25, 2024

Copy link
Copy Markdown
Collaborator Author

Added unit test and fixed the same problem in the StaticSingleThreadedExecutor.

fix(Executor): Fix segfault if callback group is deleted during rmw_wait
86d8313 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>

ahcorde commented Nov 27, 2024 •
edited by christophebedard
Loading

Copy link
Copy Markdown
Contributor

Pulls: #2683
Gist: https://gist.githubusercontent.com/ahcorde/fbbd8e165d63b1398346b46e40e02a57/raw/1e348604284103bcceb6dc009bd30720eb3e7ecc/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14874

  • Linux
  • Linux-aarch64
  • Linux-rhel
  • Windows

jmachowinski commented Nov 28, 2024

Copy link
Copy Markdown
Collaborator Author

@ahcorde @alsora can we merge ?

fujitatomoya commented Nov 30, 2024

Copy link
Copy Markdown
Collaborator

https://ci.ros2.org/job/ci_windows/22936/msbuild/new/ are unrelated.

fujitatomoya merged commit e9b1004 into ros2:rolling Nov 30, 2024
This was referenced Dec 3, 2024
Merged
Merged
This was referenced Dec 3, 2024
Merged
Merged
HarunTeper pushed a commit to HarunTeper/rclcpp that referenced this pull request Dec 9, 2024
fix(Executor): Fix segfault if callback group is deleted during rmw_w…
21208ee 
…ait (ros2#2683)

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: HarunTeper <harun.teper@tu-dortmund.de>
HarunTeper pushed a commit to HarunTeper/rclcpp that referenced this pull request Dec 9, 2024
fix(Executor): Fix segfault if callback group is deleted during rmw_w…
4f00311 
…ait (ros2#2683)

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: HarunTeper <harun.teper@tu-dortmund.de>
HarunTeper pushed a commit to HarunTeper/rclcpp that referenced this pull request Dec 9, 2024
fix(Executor): Fix segfault if callback group is deleted during rmw_w…
d810987 
…ait (ros2#2683)

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
ahcorde pushed a commit that referenced this pull request Feb 19, 2025
Cleanup of #2683 (#2714)
34cc960 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
bartalor pushed a commit to bartalor/rclcpp that referenced this pull request Jul 15, 2026
fix(Executor): Fix segfault if callback group is deleted during rmw_w…
05a5f77 
…ait (ros2#2683)

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
bartalor pushed a commit to bartalor/rclcpp that referenced this pull request Jul 15, 2026
Cleanup of ros2#2683 (ros2#2714)
17d7505 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

christophebedard christophebedard left review comments
mjcarroll mjcarroll approved these changes
alsora alsora approved these changes
fujitatomoya fujitatomoya approved these changes
ivanpauno Awaiting requested review from ivanpauno
hidmic Awaiting requested review from hidmic
wjwwood Awaiting requested review from wjwwood

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL