Skip to content

Fix crash in ~Executor for executors using the minimal constructor - #3350

Merged
ahcorde merged 2 commits into
rollingfrom
ahcorde/rolling/fix_executor_dtor_null_notify_waitable
Oct 20, 2026
Merged

ahcorde merged 2 commits into
rollingfrom
ahcorde/rolling/fix_executor_dtor_null_notify_waitable

Conversation

@ahcorde

@ahcorde ahcorde commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Description

Executor(const std::shared_ptr<Context>&) lets derived executors skip the base implementation. It leaves notify_waitable_ null and registers no shutdown callback, but ~Executor dereferenced notify_waitable_ unconditionally, so destroying such an executor segfaulted.

This PR only removes the guard conditions and the shutdown callback when notify_waitable_ was set, and adds a test that constructs and destroys an executor through that constructor.

Did you use Generative AI?

Claude Opus 5.5

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
@ahcorde
ahcorde requested a review from fujitatomoya October 15, 2026 14:21
@ahcorde ahcorde self-assigned this Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

ABI Compliance Check

✅ Verdict: compatible

Library Verdict Summary
libcomponent_manager.so ✅ compatible No ABI changes detected.
librclcpp.so ✅ compatible No ABI changes detected.
librclcpp_action.so ✅ compatible No ABI changes detected.
librclcpp_lifecycle.so ✅ compatible No ABI changes detected.
✅ libcomponent_manager.so — full abidiff report

Compared:

  • Base: lib-base/libcomponent_manager.so
  • Head: lib-pr/libcomponent_manager.so @ f4def90
(empty report — no differences printed by abidiff)
✅ librclcpp.so — full abidiff report

Compared:

  • Base: lib-base/librclcpp.so
  • Head: lib-pr/librclcpp.so @ f4def90
Functions changes summary: 0 Removed (41 filtered out), 0 Changed (79 filtered out), 0 Added (19 filtered out) functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable


✅ librclcpp_action.so — full abidiff report

Compared:

  • Base: lib-base/librclcpp_action.so
  • Head: lib-pr/librclcpp_action.so @ f4def90
(empty report — no differences printed by abidiff)
✅ librclcpp_lifecycle.so — full abidiff report

Compared:

  • Base: lib-base/librclcpp_lifecycle.so
  • Head: lib-pr/librclcpp_lifecycle.so @ f4def90
(empty report — no differences printed by abidiff)

Updated for commit f4def90 · check run · suppressions: /home/runner/work/_temp/ros2-abi-suppressions.txt

@ahcorde

ahcorde commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor Author

Pulls: #3350
Gist: https://gist.githubusercontent.com/ahcorde/64aae22178febd4e43823e795a876f90/raw/9bef69dc03eecc2de46ba1211829ee2fc72acf8a/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/20773

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Signed-off-by: Alejandro Hernandez Cordero <ahcorde@gmail.com>
@ahcorde
ahcorde merged commit 5f89e5f into rolling Oct 20, 2026
11 checks passed
@ahcorde
ahcorde deleted the ahcorde/rolling/fix_executor_dtor_null_notify_waitable branch October 20, 2026 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants