Skip to content

[Sofa.Simulation.Core] MappingGraph: fix memory leak in from parent/child shared_ptr cycle - #6259

Open
fredroy wants to merge 1 commit into
sofa-framework:masterfrom
fredroy:fix_leaks_mappinggraph
Open

[Sofa.Simulation.Core] MappingGraph: fix memory leak in from parent/child shared_ptr cycle#6259
fredroy wants to merge 1 commit into
sofa-framework:masterfrom
fredroy:fix_leaks_mappinggraph

Conversation

@fredroy

@fredroy fredroy commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

From Claude (sonnet 😅):

BaseMappingGraphNode stored both m_parents and m_children as owning shared_ptrs. MappingGraph::addEdge() linked every connected pair in both directions, creating a reference cycle between each parent and child node.
MappingGraph::clear() (called at the start of every build()) only drops the graph's own m_allNodes vector; it never breaks these cycles. Since EulerExplicitSolver::solve() and EulerImplicitSolver::solve() call
m_mappingGraph.build() unconditionally on every timestep, the entire previous graph leaked on every single animation step for as long as the solver was active.
Fix: store m_parents as std::weak_ptr instead of shared_ptr. Node ownership already belongs to MappingGraph::m_allNodes (and to each node's owning parent via m_children), so the back-reference doesn't need to be owning.

Solver step Live nodes (before fix) Live nodes (after fix)
1 0 0
101 1200 24
1001 12000 24
1901 22800 24

Basically the shared_ptr retained a ref so the parents/childs were owning each other, so the shared_ptr/gc mechanism were effectively dismissed.

Fix #6230

[with-all-tests]


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@fredroy fredroy added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request pr: AI-aided Label notifying the reviewers that part or all of the PR has been generated with the help of an AI labels Aug 25, 2026
@fredroy

fredroy commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

for windows
[ci-build][with-all-tests]

@alxbilger alxbilger 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.

Should the children be weak pointers too ? They are supposed to be in m_allNodes too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: AI-aided Label notifying the reviewers that part or all of the PR has been generated with the help of an AI pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory leak

2 participants