Fixing memory leaks. #2618

pgScorpio · 2022-04-26T19:54:22Z

Short description of changes
Moved pointers to with new assigned objects outside main and added a
cleanup function that is called before every exit to delete those objects again.

CHANGELOG:
Fixed some memory leaks.

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?
Ready to merge.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
[-] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

Moved pointers to with new assigned objects outside main and added a cleanup function that is called before every exit to delete those objects again.

ann0see · 2022-04-26T20:07:05Z

@dtinth could you please Review?

ann0see · 2022-04-26T20:07:45Z

@pgScorpio thank you again ;-)

ann0see

Not sure if that's the standard C++ clean up way? It looks a bit strange to me (but I have a Java and PHP background)

ann0see · 2022-04-26T20:10:32Z

src/main.cpp

@@ -999,15 +1044,21 @@ int main ( int argc, char** argv )
 #endif
        {
            qCritical() << qUtf8Printable ( QString ( "%1: %2" ).arg ( APP_NAME ).arg ( generr.GetErrorText() ) );
-            exit ( 1 );
+
+            if ( exit_code == 0 )


Looks a bit odd in my eyes...

What looks strange to you ?

This is the "normal" way of exception handling in main.
catch the exception, set the exit code, and exit main normally with that exit code....

Though what looks strange to me is that the try block covers only part of the main() code. and that there are so many exit() calls. The "normal" way to exit from main would be to throw an exception. In that case the cleanup can be done just before exiting main. And that would be the better solution, but that would also have a lot more code changed....
Of course I can do that implementation too if you want that ;=)

The whole handling here - as you said. Mostly the "if exit_code == 0 then set exit code to 1" thing. I'd really prefer the exit ( ) function to be extended with cleanup() but I assume exit() is not a method of a class, so you'd write a function calling exit instead.

Correct. exit() is a C/C++ library function, and is not the best way to end the program when using exceptions. In that case throwing special exceptions to exit would be the better way.

pgScorpio · 2022-04-26T20:34:45Z

Not sure if that's the standard C++ clean up way? It looks a bit strange to me (but I have a Java and PHP background)

Well.... what's the standard C++ cleanup way ?
The only "standard" is that every function should delete all allocated data before exiting. This is best achieved by using class instances, where the destructor cleans up everything when the instance goes out of scope. Unfortunately, because of the way things are implemented, there is no clear scope here.

hoffie · 2022-04-26T20:50:15Z

src/main.cpp

@@ -768,9 +811,9 @@ int main ( int argc, char** argv )
    bIsClient      = true; // Client only - TODO: maybe a switch in interface to change to server?

    // bUseMultithreading = true;
-    QApplication* pApp = new QApplication ( argc, argv );
+    pApp = new QApplication ( argc, argv );


So, for iOS apps this used to be of type QApplication instead of QCoreApplication. Can you give a quick explanation why this is safe to do?

It's probably hard or artificial to put that into an individual commit, but at least the commit description should mention that this was intentional and why it is considered safe (I guess because iOS doesn't use any methods from the broader interface either?).

???

I didn't change the type, I just moved the pointer declaration outside main() to be able to implement the cleanup() function. For IOS it is still QApplication:

#ifdef HEADLESS QCoreApplication* pApp = new QCoreApplication ( argc, argv ); #else # if defined( Q_OS_IOS ) bUseGUI = true; bIsClient = true; // Client only - TODO: maybe a switch in interface to change to server? // bUseMultithreading = true; pApp = new QApplication ( argc, argv ); # else pApp = bUseGUI ? new QApplication ( argc, argv ) : new QCoreApplication ( argc, argv ); # endif #endif

EDIT: Hmmmm, but now I see I also missed something in headless mode ;=) So thanks !

Yeah, I understand the reason for moving. I just wanted to hint at the side effect (maybe it's a noop, but I'm not sure):
pApp was previously either of type QApplication or QCoreApplication based on platform and/or GUI/non-GUI.
With the new code, it's always a QCoreApplication which is probably the lowest common denominator. It might be fine, but it's still a change for iOS, isn't it?

I'm still wondering why you think anything changed for IOS, AFAIK It was, and still is, always QApplication for IOS.

Current version:

#ifdef HEADLESS QCoreApplication* pApp = new QCoreApplication ( argc, argv ); #else # if defined( Q_OS_IOS ) bUseGUI = true; bIsClient = true; // Client only - TODO: maybe a switch in interface to change to server? // bUseMultithreading = true; QApplication* pApp = new QApplication ( argc, argv ); # else QCoreApplication* pApp = bUseGUI ? new QApplication ( argc, argv ) : new QCoreApplication ( argc, argv ); # endif #endif

New version of this PR:

#ifdef HEADLESS pApp = new QCoreApplication ( argc, argv ); #else # if defined( Q_OS_IOS ) bUseGUI = true; bIsClient = true; // Client only - TODO: maybe a switch in interface to change to server? // bUseMultithreading = true; pApp = new QApplication ( argc, argv ); # else pApp = bUseGUI ? new QApplication ( argc, argv ) : new QCoreApplication ( argc, argv ); # endif #endif

It was, and still is, always QApplication for IOS.

As I read it, the old code for iOS was essentially this:

QApplication* pApp = new QApplication ( argc, argv );

The new code is this:

QCoreApplication* pApp = NULL; pApp = new QApplication ( argc, argv );

I belive (and this is where I might be wrong!) that QCoreApplication is the narrower base class and QApplication is the richer GUI class. Therefore, I suspected that this assignment would limit the interface of pApp to QCoreApplication here (what had been QApplication, i.e. the richter interface, before).
I suspect that it does not make a relevant difference, otherwise I would have expected build failures (e.g. when accessing methods which are only part of QApplication and not QCoreApplication), but I don't claim to have full understanding here so I was asking about whether this was intentional and/or known to be side-effect free.

Thinking some more about it, it's likely to be OK as the non-iOS code is written in a similar way (the type is always QCoreApplication, even when the instance is QApplication). It still makes sense to think explicitly about it as it is more than a syntax change or code moving.

The inheritance chain as I read it is:

QCoreApplication (most basic type, sufficient for CLI usage)

QGuiApplication

QApplication (richer type, used for GUI usage)

OK, Now I see your point. Thanks for noting!

QCoreApplication is the base type of all QApplications, that's why I can use that type to store the pointer.
But I expected the basetype to have virtual functions, but now I checked it and that isn't the case...
Though all QApplications will call QCoreApplication::exec() eventually there might be a difference in behavior, so I will have to find a better solution for this.

EDIT: But how about this line in the current code ???

QCoreApplication* pApp = bUseGUI ? new QApplication ( argc, argv ) : new QCoreApplication ( argc, argv );

Shouldn't there be the same problem ??

src/main.cpp

hoffie · 2022-04-26T20:53:09Z

src/main.cpp

@@ -55,6 +55,42 @@ extern void qt_set_sequence_auto_mnemonic ( bool bEnable );

 // Implementation **************************************************************

+// NOTE: To prevent memory leaks any pointers assigned with new
+//       should be declared here and checked and deleted in reverse order of creation in cleanup().
+//       cleanup() should be called before any exit() !


I'm wondering if it was harder to forget calling cleanup if we had a generic CleanExit(int) function?

I'm wondering if it was harder to forget calling cleanup if we had a generic CleanExit(int) function?

Yes I thought about that too, but then I could not use cleanup() for the normal exit. (And also one could still call exit() instead of CleanExit(), the same kind of mistake as forgetting to call cleanup().)

I would prefer using an exception instead of using exit() even more...
(Note that calling exit() instead of normally returning from main with a return code may skip some cleanup in windows itself, and maybe it's the same on other OS's, so this increases the chance on memory leaks.)

hoffie · 2022-04-26T20:56:58Z

src/main.cpp

@@ -63,6 +99,7 @@ int main ( int argc, char** argv )
    // Qt will not show these with underline characters in the GUI on MacOS. (#1873)
    qt_set_sequence_auto_mnemonic ( true );
 #endif
+    int exit_code = 0;


This is about improving return code correctness, right? At least I don't see first-hand how it is related to the memory leak fix.

I guess it's halfway OK to do it in this PR, but it should be an individual commit.

Correct.
I added exit_code to be able to do the cleanup and a normal return on an exception.

You are correct I now also use the pApp->exec() result so when exiting via QApplication also the correct exit code is returned by main, and that is indeed not directly related to the problem.

Forgot to use the right pApp pointer in headless mode.

pgScorpio · 2022-04-26T23:58:10Z

I opened a second PR #2620 on the same issue.
That one is the solution using exceptions.
Please pick your solution of choice.
As said I prefer the exception solution...

hoffie · 2022-04-27T07:15:27Z

src/main.cpp

+//       should be declared here and checked and deleted in reverse order of creation in cleanup().
+//       cleanup() should be called before any exit() !
+
+QCoreApplication* pApp       = NULL;


Let's keep the conversation in the other place, but this is the place I'm referring to wrt. QCoreApplication vs. QApplication on iOS.

hoffie · 2022-04-27T07:23:26Z

src/main.cpp

@@ -768,9 +811,9 @@ int main ( int argc, char** argv )
    bIsClient      = true; // Client only - TODO: maybe a switch in interface to change to server?

    // bUseMultithreading = true;
-    QApplication* pApp = new QApplication ( argc, argv );
+    pApp = new QApplication ( argc, argv );


It was, and still is, always QApplication for IOS.

As I read it, the old code for iOS was essentially this:

QApplication* pApp = new QApplication ( argc, argv );

The new code is this:

QCoreApplication* pApp = NULL; pApp = new QApplication ( argc, argv );

I belive (and this is where I might be wrong!) that QCoreApplication is the narrower base class and QApplication is the richer GUI class. Therefore, I suspected that this assignment would limit the interface of pApp to QCoreApplication here (what had been QApplication, i.e. the richter interface, before).
I suspect that it does not make a relevant difference, otherwise I would have expected build failures (e.g. when accessing methods which are only part of QApplication and not QCoreApplication), but I don't claim to have full understanding here so I was asking about whether this was intentional and/or known to be side-effect free.

Thinking some more about it, it's likely to be OK as the non-iOS code is written in a similar way (the type is always QCoreApplication, even when the instance is QApplication). It still makes sense to think explicitly about it as it is more than a syntax change or code moving.

The inheritance chain as I read it is:

QCoreApplication (most basic type, sufficient for CLI usage)

QGuiApplication

QApplication (richer type, used for GUI usage)

pgScorpio · 2022-04-28T17:46:53Z

I'm closing this PR in favor of #2620

Too many problems with this solution and it also won't work when exit(1) is used in other places, like in PR #2599
I'm also working on a CCommandlineParameters class and new settings handling which all need exception handling to solve this problem.

Fixing memory leaks.

2ecdb7f

Moved pointers to with new assigned objects outside main and added a cleanup function that is called before every exit to delete those objects again.

pgScorpio mentioned this pull request Apr 26, 2022

Memory leak when using Rpc server #2614

Open

hoffie added this to the Release 3.9.0 milestone Apr 26, 2022

ann0see reviewed Apr 26, 2022

View reviewed changes

hoffie reviewed Apr 26, 2022

View reviewed changes

pgScorpio added 2 commits April 26, 2022 23:02

Bugfix pClientRpc does not exist in server-only mode

affd49d

Bugfix headless mode

e090359

Forgot to use the right pApp pointer in headless mode.

pgScorpio mentioned this pull request Apr 26, 2022

Fixing memory leaks using exceptions #2620

Closed

4 tasks

hoffie reviewed Apr 27, 2022

View reviewed changes

Bugfixes pApp pointer type and exit_code

e8c1a21

pgScorpio closed this Apr 28, 2022

pljones removed this from the Release 3.9.0 milestone May 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing memory leaks. #2618

Fixing memory leaks. #2618

pgScorpio commented Apr 26, 2022

ann0see commented Apr 26, 2022

ann0see commented Apr 26, 2022

ann0see left a comment

ann0see Apr 26, 2022

pgScorpio Apr 26, 2022 •

edited

Loading

ann0see Apr 26, 2022

pgScorpio Apr 26, 2022

pgScorpio commented Apr 26, 2022

hoffie Apr 26, 2022

pgScorpio Apr 26, 2022 •

edited

Loading

hoffie Apr 26, 2022

pgScorpio Apr 26, 2022 •

edited

Loading

hoffie Apr 27, 2022

pgScorpio Apr 27, 2022 •

edited

Loading

hoffie Apr 26, 2022

pgScorpio Apr 26, 2022 •

edited

Loading

hoffie Apr 26, 2022

pgScorpio Apr 26, 2022

pgScorpio commented Apr 26, 2022

hoffie Apr 27, 2022

hoffie Apr 27, 2022

pgScorpio commented Apr 28, 2022

Fixing memory leaks. #2618

Fixing memory leaks. #2618

Conversation

pgScorpio commented Apr 26, 2022

Checklist

ann0see commented Apr 26, 2022

ann0see commented Apr 26, 2022

ann0see left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio commented Apr 26, 2022

Choose a reason for hiding this comment

pgScorpio Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio commented Apr 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgScorpio commented Apr 28, 2022

pgScorpio Apr 26, 2022 •

edited

Loading

pgScorpio Apr 26, 2022 •

edited

Loading

pgScorpio Apr 26, 2022 •

edited

Loading

pgScorpio Apr 27, 2022 •

edited

Loading

pgScorpio Apr 26, 2022 •

edited

Loading