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

fix: Fixed possible memory violation after remove_node by jmachowinski · Pull Request #2767 · ros2/rclcpp · GitHub

/ rclcpp Public

fix: Fixed possible memory violation after remove_node - #2767

Draft
jmachowinski wants to merge 1 commit into
ros2:rollingfrom
cellumation:remove_node_blocking
ConversationCommits (1)ChecksFiles changed
Draft

fix: Fixed possible memory violation after remove_node#2767
jmachowinski wants to merge 1 commit into
ros2:rollingfrom
cellumation:remove_node_blocking

Conversation

jmachowinski commented Mar 14, 2025

Copy link
Copy Markdown
Collaborator

Up until this change, after the call to remove node, entities of a node might still be in the use by the executor. Therefore direct deletion of the node after the call might result in a memory violation if an entity would do a callback into the node itself. From now on, by default the remove_node will be blocking until all entities are released by the executor, which should be the expected behavior for most users.

This commit fixes #2749

fujitatomoya self-requested a review March 14, 2025 16:12
fujitatomoya approved these changes Mar 18, 2025
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 one comment.

Comment thread rclcpp/test/rclcpp/test_executor.cpp

fujitatomoya commented Mar 18, 2025

Copy link
Copy Markdown
Collaborator

@jmachowinski do we want to backport the similar fix to jazzy and humble without changing function signature?

jmachowinski commented Mar 18, 2025

Copy link
Copy Markdown
Collaborator Author

@jmachowinski do we want to backport the similar fix to jazzy and humble without changing function signature?

This would be a behavior change, so I think we can't backport

jmachowinski commented Mar 18, 2025

Copy link
Copy Markdown
Collaborator Author

Pulls: #2767
Gist: https://gist.githubusercontent.com/jmachowinski/4d53ebb774c3ac9d2a8237d958ac3bbd/raw/6a7d039e89a00abdaecdcc0ca187e49fdda5b0e7/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15412

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

ahcorde commented Apr 8, 2025

Copy link
Copy Markdown
Contributor
  • Linux
  • Linux-aarch64
  • Linux-rhel
  • Windows

jmachowinski commented Apr 8, 2025

Copy link
Copy Markdown
Collaborator Author

@ahcorde I think rerunning the CI won't change the result. I need to investigate whats going on, but did not find time yet...

fix: Fixed possible memory violation after remove_node
9a481a1 
Up until this change, after the call to remove node, entities of a
node might still be in the use by the executor. Therefore direct deletion
of the node after the call might result in a memory violation if an entity
would do a callback into the node itself. From now on, by default the
remove_node will be blocking until all entities are released by the executor,
which should be the expected behavior for most users.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
jmachowinski force-pushed the remove_node_blocking branch from 1e40f42 to 9a481a1 Compare July 8, 2025 15:21

jmachowinski commented Jul 8, 2025

Copy link
Copy Markdown
Collaborator Author

Pulls: #2767
Gist: https://gist.githubusercontent.com/jmachowinski/1cd47cc4d2317b54aabfe35687774fdf/raw/0539ed4e823926d2c893726f0a91f9a1d507f213/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16431

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

jmachowinski commented Jul 8, 2025

Copy link
Copy Markdown
Collaborator Author

rerunning CI to get the failing tests, old logs seems to have been deleted

ahcorde requested changes Oct 1, 2025
View reviewed changes

ahcorde left a comment

Copy link
Copy Markdown
Contributor

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

Do you mind to merge with rolling ?

jmachowinski marked this pull request as draft October 2, 2025 18:02

jmachowinski commented Oct 2, 2025

Copy link
Copy Markdown
Collaborator Author

This patch causes unwanted side effects, and I did not find time to rework it yet

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

fujitatomoya fujitatomoya approved these changes
ahcorde ahcorde requested changes

Assignees

jmachowinski

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

🧑‍🌾 Windows test_executors regression: SEH Exception

3 participants

Back | FazBrowse Home | New Git URL