jamulussoftware
Fixing memory leaks using exceptions
Short description of changes
Fix some memory leaks. Alternative for PR #2618 using exceptions to cleanly exit the application. (#2618 won't solve the problem when still using exit() or on other unhandled exceptions.)
CHANGELOG: Fixed some memory leaks in main.
Context: Fixes an issue?
Fixes #2614
Does this change need documentation? What needs to be documented and how?
No documentation changes
Status of this Pull Request
Bugfix
What is missing until this pull request can be merged? Review
Checklist
- [x] I've verified that this Pull Request follows the general code principles
- [x] I tested my code and it does what I want
- [x] My code follows the style guide
- [-] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
- [x] I've filled all the content above
Can I suggest a simpler solution that combines ideas from both of your approaches?
The only uses of exit outside of main are in the option-parsing functions, which are only used before any of the dynamic allocations happen anyway. So turning the exit calls in main into straightforward returns will guarantee that stack-allocated objects in main are destroyed properly.
You can then convert the pointer variables into smart pointers, adding new smart pointers for the RPC classes that didn't have them already as you've done above.
I've put a quick attempt at this on the atsampson:scopedexit branch. I haven't tested this with all the combinations of configuration options but it should give you an idea of what I mean...
@atsampson
Can I suggest a simpler solution that combines ideas from both of your approaches?
Thanks for the suggestion !
But...
For the current code that would be almost right. Though exit(1) is also used in the commandline parsing functions, and return can't be used there. Also there a several PR's pending that would also need an exception solution, that's why I changed to using exceptions.
EDIT: BTW Where are the with new allocated instances deleted ???, that was the original memory leak problem....
For the current code that would be almost right. Though exit(1) is also used in the commandline parsing functions, and return can't be used there.
That's fine, because the option-parsing functions are only used before any of the dynamic allocation happens. There's a comment noting this in my changes.
BTW Where are the with new allocated instances deleted ???
QScopedPointer is a smart pointer class; it's roughly Qt's equivalent of std::unique_ptr. It deletes the pointer it's holding automatically when it goes out of scope, so the object will be destroyed correctly when the function exits (either via return or through an exception). If you haven't encountered smart pointers before, I'd definitely recommend reading up on them - there's rarely a good reason to use delete in modern C++.
QScopedPointeris a smart pointer class; it's roughly Qt's equivalent ofstd::unique_ptr. It deletes the pointer it's holding automatically when it goes out of scope, so the object will be destroyed correctly when the function exits (either viareturnor through an exception)
Yes I know smart pointers, I even considered using them. But they have some disadvantages too. 1: They only work when there is a well defined scope. And the rpc classes don't have a well defined scope in the current implementation. 2: No garantee in wich order they are deleted if they are in the same scope (often same order as creation). And since the pointers are cross referenced we need to delete them in a specific order (reverse order of creation).
So I still think the exception solution is the simplest and most reliable, since it works under all circumstances...
No garantee in wich order they are deleted if they are in the same scope (often same order as creation).
No, that's not correct. In C++, variables with automatic storage duration (i.e. locals) are guaranteed to be destroyed in reverse order of construction - see the language standard or the ISO C++ FAQ. If that wasn't the case, then lots of RAII idioms wouldn't work.
And the rpc classes don't have a well defined scope in the current implementation.
... which means they do have a well-defined lifetime if you convert them to smart pointers - they will be deleted in reverse order of the declarations when main exits. You want to ensure that the RPC objects are deleted before the objects they're controlling, which matches the declaration order in the current code.
There's absolutely nothing wrong with using exceptions to cause a clean exit from elsewhere in the code - one of the reasons smart pointers were added to the standard library was to simplify memory management when exceptions are being used, and if you're adding more use of exceptions then you may well find that there are other places in the code where you can replace manual memory management with smart pointers to avoid complex cleanup code.
@atsampson Thanks for the information. If this is true (though I encountered several occasions where it didn't work this way) we could use smart pointers here to do the cleanup (and so it also should be done on a lot of other places in the code), but see the note:
Note [2] However, the program can be terminated (by calling std::exit() or std::abort() for example) without destroying objects with automatic storage duration — end note]
So we still should use exceptions instead of exit. I don't like to rely on the fact that no pointers or other objects are assigned before using exit. This might (and will) change in the future, so it would be safer to state "NEVER use exit but use the CErrorExit exception.
@atsampson @pgScorpio what's the agreement on the other discussed solution I think @pgScorpio wanted to use exceptions?
@atsampson @pgScorpio what's the agreement on the other discussed solution I think @pgScorpio wanted to use exceptions?
Correct. Since the exception solution will also work where normally exit() would be used elsewhere, while the other method only works when exiting from main()
@ann0see The only thing I'm really uncomfortable about here is the manual cleanup code using delete - it really should be using smart pointers in modern C++. I'm fine with adding an exception specifically to handle the option parsing errors - there's no advantage in terms of the original fault, but it does simplify the code in the parsing functions. If the exception rework goes beyond that, then I think it would be better to review it later when it's needed.
The only thing I'm really uncomfortable about here is the manual cleanup code using
delete- it really should be using smart pointers in modern C++.
I agree that using smart pointers would be a better solution, but AFAIK, besides in CSignalHandler, it's nowhere else used in the code, but though not specifically the solution for the original problem, it could be implemented here as well, though I think that should better be a separate PR, replacing all pointers with smart pointers...
I'm fine with adding an exception specifically to handle the option parsing errors - there's no advantage in terms of the original fault, but it does simplify the code in the parsing functions.
If the exception rework goes beyond that, then I think it would be better to review it later when it's needed.
And as you say yourself, it's already used in the option parsing functions... And this will be even more neccesay if we implement the commandline options class(es).
Ok. So @atsampson @pgScorpio could you please open an issue on that? What's the state of this PR? Can @atsampson do a review so that we are a bit closer to a merge?
@atsampson Thanks for the review ;-). I think it's quite close to the KISS principle and "hard core Jamulus like".
@ann0see
I think it's quite close to the KISS principle and "hard core Jamulus like".
May I note that the KISS principle refers to the implementation of (unnecessary) functionality, NOT the way the code is implemented. "Simple and Stupid" code implementation will always lead to more and more problems and bugs.... That's the whole idea behind my 'improvement' PR's, improving the framework to make maintaining the code easier and less prone to bugs...
Yes. Probably. However the history of Jamulus was quite radical with code changes. It doesn't need to be like that - at least it's probably not up to me, but "the community" to decide. The goal should be simple, extendable, unbloated code which doesn't do more than it needs to do. I think the refactoring of the Autobuild was a good example on simplification.
Not sure if that's a good idea, but I think for your PRs (especially the sound related redesign, I think it would be worth having npostavs and corrados as reviewers). I know corrados is officially no longer part of the Jamulus Team, however he has the most insight probably.
@jamulussoftware/maindevelopers how do we proceed here? I think the review by @atsampson has valid input, but I'm not fully sure how to continue (especially since - as discussed internally - some team members don't quite see the improvements).
I'd like someone like corrados to review sound redesign related changes (maybe not this one) since he's probably most familiar with the sound code.
Moving back to Triage until a way forward is proposed (and removing milestone).