Fix tabulated bond memory leaks

[jngrad]

Fixes #3906

Description of changes:

• fix dangling pointers in tabulated bonds
• reduce code duplication in callbacks

[KaiSzuttor]

haven't looked into the code, but can't we use smart pointers?

[jngrad]

I don't think so: we overwrite the memory block of the bond object on worker nodes with a bytestring of the same object from the head node, so the counter from a shared_ptr would be overwritten. As for unique_ptr, in the line you highlighted, we do an assignment, so the smart pointer goes out of scope and gets garbage-collected... but it contains a random memory address (because of the bytestring deserialization a few lines above), which is a recipe for segmentation faults.

[KaiSzuttor]

i'm not sure i'm following... but it sounds like this is a place for larger scale refactoring which is out of scope of this pull request?

[jngrad]

the correct way would be to write boost serialization methods, which can handle pointer members out of the box, e.g.

struct Tabulated_bond_parameters {
  int type;
  TabulatedPotential *pot;

private:
  friend boost::serialization::access;
  template <typename Archive>
  void serialize(Archive &ar, long int /* version */) {
    ar &type;
    ar &pot;
  }
};

but there is no guarantee that we are overwriting a bond type with the same bond type, so we would have to add destructors too... Not sure if this is worth the effort, if our long-term goal is to replace this interaction communication infrastructure with a python setter.

[jngrad]

Also, a much more promising refactoring goal is breaking the dependency on MpiCallbacks.hpp from consumers of MPI callbacks (fix-3906...fix-3907-p1), which as a side effect is removing global variables and removing prototypes of MPI worker functions from header files.

[KaiSzuttor]

Also, a much more promising refactoring goal is breaking the dependency on MpiCallbacks.hpp from consumers of MPI callbacks (fix-3906...fix-3907-p1), which as a side effect is removing global variables and removing prototypes of MPI worker functions from header files.

Isn't that totally orthogonal to the memory leak issue?

[KaiSzuttor]

the correct way would be to write boost serialization methods, which can handle pointer members out of the box, e.g.

struct Tabulated_bond_parameters {
  int type;
  TabulatedPotential *pot;

private:
  friend boost::serialization::access;
  template <typename Archive>
  void serialize(Archive &ar, long int /* version */) {
    ar &type;
    ar &pot;
  }
};

but there is no guarantee that we are overwriting a bond type with the same bond type, so we would have to add destructors too... Not sure if this is worth the effort, if our long-term goal is to replace this interaction communication infrastructure with a python setter.

sorry, if that has been discussed and i don't remember it anymore... Do we have some design plans for the interaction stuff already?

[RudolfWeeber]

sorry, if that has been discussed and i don't remember it anymore... Do we have some design plans for the interaction stuff already?

My understanding is that * Actors, bonded and non-bonded interactions should be used via the script-interface, which handles the broadcasting of parameters, so the comm infrastructure for it in the core goes away * The BondedInteractionParams type should be a variant rather than a union * Since the communication is handled by the script interface, bonds are set in the core on the node level by assignment. As long as the strucs holding the parameters for the individual bodns either don’t contain manual memory management or provide a destructor, it should not be possible to loose memory anymore.

[KaiSzuttor]

so think we should now merge this but also stop refactoring the existing infrastructure if the plan is to move most of the parts to the sciptinterface.

[KaiSzuttor]

cant't you just check for nullptr?

[KaiSzuttor]

if (bonded_ia_params[bond_type].p.tab.pot) delete bonded_ia_params[bond_type].p.tab.pot;

[jngrad]

Actually this would raise a Clang-Tidy warning because you can simplify it to

delete bonded_ia_params[bond_type].p.tab.pot;

which is safe on a nullptr.
However, this would also trigger a segfault, because you can create a tabulated bond on top of an existing FENE bond, in which case .p.tab.pot contains a random address. You can overwrite an bond existing bond in ESPResSo, this is used for example in testsuite/python/interactions_bonded_interface.py.

[fweik]

IMHO, #3019 should be fixed before this. This correct way to solve this is to have the tabulated bond manage it's own memory (via unique ptr or similar), but this is not possible as long as the bond parameters is a union, because you can only have trivial types in a union. This is why I opened 3019 in the first place. There is no way to fix the memory leak which is not a hack in the sense that it mixes unrelated concerns.

[RudolfWeeber]

Let's merge this, also due to the comment fixes contained in the PR.
The "fix" for the mem leak is, of course, a hack.
It will go away, once we convert the bond parameters to a Variant and let the script interface do the parameter broadcasting.