Faulty memory management in adapt

[Anders Logg (Chalmers)]

The following code crashes:

from dolfin import *

mesh = UnitSquareMesh(16, 16)
subdomains = MeshFunction("size_t", mesh, 2, 0)

data = []
for i in range(5):

    # Works if this line is not commented out
    #data.append(subdomains)

    markers = CellFunction("bool", mesh, True)
    mesh = adapt(mesh, markers)
    subdomains = adapt(subdomains, mesh)

The code runs if the MeshFunctions are stored in a list during refinement.

The cause is likely the set_parent_child function in adapt.cpp which uses reference_to_no_delete_pointer. What needs to be done is to improve the interface by adding shared pointer versions of all functions and making sure that these are used from Python (not the reference versions).

[Anders Logg (Chalmers)]

The new work Chris is doing on refinement hierarchies will change the way memory and adapted objects are handled, so a new issue will need to be opened in the unlikely event that the code by @chris_richardson is not perfect.

[Jan Blechta]

Issue #563 was marked as a duplicate of this issue.

[Jan Blechta]

Example workaround for mesh function

adapt_wrapper_code = """
#include <dolfin/adaptivity/adapt.h>

namespace dolfin {

std::shared_ptr<MeshFunction<std::size_t>> adapt_wrapper(
  const MeshFunction<std::size_t>& mesh_function,
  std::shared_ptr<const Mesh> adapted_mesh)
{
  std::shared_ptr<MeshFunction<std::size_t>> mf;
  mf = std::make_shared<MeshFunction<std::size_t>>(adapt(mesh_function, adapted_mesh));
  return mf;
}
}
"""

adapt = compile_extension_module(adapt_wrapper_code).adapt_wrapper;

EDIT: This workaround is incorrect. Correct one is to never use returned value of any adapt function, see below.

[Martin Sandve Alnæs]

Removing milestone: 1.6 (automated comment)

[Jan Blechta]

Few comments to this:

1. The nodeleter in set_parent_child _parent.set_child(child); child->set_parent(reference_to_no_delete_pointer(_parent));is there to avoid circular pointers parent->child->parent->child-> .... Correct solution is having std::weak_ptr<T> Hierarchical::_parent rather than shared_ptr with no deleter.

2. Much more serious flaw is that many adapt functions allocate returned object by std::shared_ptr<T> child(new T());, store child using set_parent_child and return *child by value! They should rather return child, i.e. the shared ptr which takes care of object lifetime.

Workaround for users:

1. Don't ever use returned object of any adapt functions!
2. Access the result by child() method.

Example:

# Don't do this!
mesh = adapt(V.mesh(), markers)
u = adapt(u, mesh)

# Do rather this
adapt(V.mesh(), markers)
mesh = V.mesh().child()
adapt(u, mesh)
u = u.child()

[Jan Blechta]

Fix of 2. in jan/fix-issue-319. Sketch of 1. in jan/weak-parent.

[Jan Blechta]

2. fixed in master.

[Jan Blechta]

Plan to resolve the rest in #642 and #643.

[Nicklas Karlsson]

Problem is still there. Code below work but using value returned from this function result in segmentation fault. I would guess pointer is set to local variable somewhere within code.

In general I consider declaring local variable as the first and best choice for memory magement since also make possible use of stack like memory allocation scheme with or without stack atlhough I am not certain compiler is able to handle the case then memory allocated by object is to large to handle by the stack but as with any memory magament care must be taken so that there is no pointer/reference left then object cease to exist.

void refiner(std::shared_ptr<Mesh> refined_mesh, std::shared_ptr<const Mesh> mesh, double r){ auto refineMarkers = MeshFunction<bool>(mesh, mesh->topology().dim()); refineMarkers.set_all(false); refineArea(r).mark(refineMarkers, true);

refined_mesh = std::make_shared<Mesh>(adapt(mesh.get(), refineMarkers));

VTKFile meshFile("results/meshDebug1.pvd", "ascii"); meshFile.write(*refined_mesh); }