Open juan-g-bonilla opened 1 year ago
Tagging @joaogvcarneiro @patkenneally @schaubh
Issue #295 is another example of memory management issues related to raw pointers. In that case, a ReadFunctor
had a pointer to the payload of a Message
declared in Python that goes out of scope too early, which causes the Message
(and payload) to be destroyed. Making the payload a shared_ptr
would allow readers to refer to them even when the Message
has been destroyed. However, C modules present a problem.
Describe your use case Currently, the codebase relies heavily on raw pointers and occasionally C-style arrays. There are many reasons why using these is a bad idea (summarized from "Effective Modern C++" by Scott Meyers):
delete
,delete[]
, or even some other custom destruction mechanism.Smart pointers and
std::array
stand to solve these problems:operator[]
is disallowed forstd::shared_ptr
andstd::unique_ptr
.std::shared_ptr
andstd::unique_ptr
handle the destruction of the object.std::weak_ptr
supports dangling checks and will throw nice exceptions when trying to manipulate them.Morever, the fact that we use SWIG makes raw pointers even more brittle. Because of how memory is handled by SWIG, an object is either owned by the Python layer or the C++ layer (by default, Python owns the object). This means that, as soon as Python objects go out of scope in Python, the C++ object is removed, and any pointer left around is dangling. This means that, for the duration of the simulation, all Python objects must be kept somehow alive.
Currently, modules and tasks are kept alive by adding them to lists in
SimBaseClass
andTaskBaseClass
. On the other hand, when planets are created ingravBodyFactory
through methods likecreateEarth
, these methods explicitely return ownership of the memory to the C++ layer throughearth.this.disown()
. This allows the PythonGravBodyData
to go out of scope in Python, but it may be causing memory leaks (I haven't found any place where these pointers are deleted).Integrators, and possibly many other objects, have no built-in mechanism to keep them alive. This means that the following code will fail:
Because as soon as the function returns, the integrator object will be deleted. This can be an enormous source of confusion for our users, especially those not familiar with memory management. Moreover, these errors can be hard to debug, as dangling pointers are undefined behaviour.
Smart pointers are not a perfect solution to the complex memory management of SWIG, but they do provide some advantages. By using
shared_ptr
, memory ownership is shared by the C++ and Python layers. This means that an object handled by asmart_ptr
can go out of scope in Python without the C++ object disappearing, thus allowing the following code:Note that the use of
shared_ptr
requires use of some SWIG features that imply widespread changes to our SWIG files and will very likely break user code that is implementing their own C++ code.See branch 282-smart-pointer-dynamic-object for an example of how one could move from raw pointers to
shared_ptr
. Note that this is just a prototyping branch, there might be other ways to use smart pointers with SWIG that are better than what I came up with in this implementation.Describe alternatives solutions you've considered We could stay with raw pointers and develop a strict protocol for how to handle these objects. Special attention should be given to those pointers that interface with SWIG to prevent dangling pointers in C++ but also memory leaks.
Additional context The branch showcased before makes use of the
%import
SWIG feature. Currently, by always including every header file in our SWIG interface files, where are forcing SWIG to generate Python wrappers for every single module it generates. By using%import
, we can link modules together, which removes code duplication. This lowers the memory requirements of Basilisk and prevents some unintuitive behaviour:Most importantly, the use of
%import
allows us to perform all SWIG changes to a a class in a single file (such as the use ofshared_ptr
), and then have these changes apply to all other SWIG interfaces.Systems that use raw pointers (to be expanded)
SimModel::processList
andSimThreadExecution::processList
ModelScheduleEntry::TaskPtr
(used inSysProcess::processTasks
)ModelPriorityPair::ModelPtr
(used inSysModelTask::TaskModels
std::vector<Message<SomeMsgPayload>*> moreOutMsgs
(this is especially prone to leaks, as users could forget to free this memory on the destructor of externalSysModel
)SimModel::threadList
andSimThreadExecution::threadContext
DynamicObject::integrator
andDynamicObject::setIntegrator
DynamicObject::syncDynamicsIntegration
andStateVecIntegrator::dynPtrs
GravityEffector::gravBodies
DynParamManager
(registerState
,getStateObject
,createProperty
, andgetPropertyReference
return raw pointers; models store the results as raw pointers too)THROperation::opThrustForce_B
andTHROperation::opThrustForce_B
OrbElemConvert::r_N
,OrbElemConvert::v_N
FuelTank::fuelSloshParticles
,FuelTank::dynEffectors
,FuelTank::stateEffectors
,FuelTank::fuelTankModel
(+ related methods)VSCMGStateEffector::AddVSCMG
(this method is also copying the passedNewVSCMG
, so changes toNewVSCMG
after the method is called will not be reflected in the actual object used byVSCMGStateEffector
)DentonFluxModel
uses several C-style arraysaddSpacecraftToModel
, present in:FormationBarycenter
,MsmForceTorque
,AtmosphereBase
,MagneticFieldBase
,Eclipse
,GroundLocation
,SpacecraftLocation
SpiceInterface::loadSpiceKernel
,SpiceInterface::unloadSpiceKernel
,SpiceInterface::pullSpiceData
(some raw pointers are inevitable in the.cpp
, as they interact with CSPICE)AtmosphereBase::updateRelativePos
andAtmosphereBase::evaluateAtmosphereModel
(and subclasses)MagneticFieldBase::updateRelativePos
andMagneticFieldBase::evaluateMagneticFieldModel
(and subclasses)