Fix memory leak in radTInteraction::Setup#5
Conversation
The memory allocated for IdentTransPtr was not reclaimed when the method failed, causing the constructor to throw and skip the destructor. Changing the code to manage the memory with unique_ptr causes the object to be automatically reclaimed regardless of whether the constructor throws or not.
|
|
||
| NewMagnArray = NULL; | ||
| NewFieldArray = NULL; | ||
| IdentTransPtr = NULL; |
There was a problem hiding this comment.
There is probably a ton of places in the code that may lead to some memory leak(s) when a method fails. In our previous / current interfaces (Mathematica. Python) this is not so important, because entire simulation is usually re-started if some method fails. But all such places could indeed de analyzed more thoroughly, and eliminated.
In this particular case, instead of a pointer to a radIdentTrans object, we can keep the object itself (since it does not require a lot of memory) as a member variable, in that or in the base class.
I am working now on parallelizing (via MPI) this and other functions, so will postpone this eventual change until the corresponding update.
There was a problem hiding this comment.
Yes, code with manual memory management tends to not do the right thing around errors, it's just very very hard to get it right. This is where things like unique_ptr, vector and other higher level constructs really shine. In fact, there is a wide consensus that since C++11 there is essentially never a need to use new or delete at all in application level code*, and it's considered best practice to avoid them.
Cool that you are working on parallelizing this code! Let me warmly recommend ThreadSanitizer. It detects and pinpoints data races and other threading issues, which can be nearly impossible to fix or even find. If you write code that has any mutable state that is shared across threads you need this tool. It is one of the closest things to magic I have seen in the world of software.
*: The one exception to this is when creating unique_ptr objects, where you still need new. C++14 introduces std::make_unique to fix this little wart.
The memory allocated for IdentTransPtr was not reclaimed when the method failed, causing the constructor to throw and skip the destructor.
Changing the code to manage the memory with
unique_ptrcauses the object to be automatically reclaimed regardless of whether the constructor throws or not. The lines that initialize the pointer toNULLare removed becauseunique_ptrinitializes to null automatically by default.I found this with Address Sanitizer. The change removes the warning and seems to work. The change is tested on Linux/Clang only.