Skip to content

Fixing memory leaks. #2618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pgScorpio wants to merge 4 commits into from
Closed

Fixing memory leaks. #2618

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ecdb7f
Fixing memory leaks.
pgScorpio Apr 26, 2022
affd49d
Bugfix pClientRpc does not exist in server-only mode
pgScorpio Apr 26, 2022
e090359
Bugfix headless mode
pgScorpio Apr 26, 2022
e8c1a21
Bugfixes pApp pointer type and exit_code
pgScorpio Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 66 additions & 12 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() !
Copy link
Member

@hoffie hoffie Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


QCoreApplication* pApp = NULL;
Copy link
Member

@hoffie hoffie Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CRpcServer* pRpcServer = NULL;
CClientRpc* pClientRpc = NULL;
CServerRpc* pServerRpc = NULL;

void cleanup()
{
if ( pServerRpc )
{
delete pServerRpc;
pServerRpc = NULL;
}

if ( pClientRpc )
{
delete pClientRpc;
pClientRpc = NULL;
}

if ( pRpcServer )
{
delete pRpcServer;
pRpcServer = NULL;
}

if ( pApp )
{
delete pApp;
pApp = NULL;
}
}

int main ( int argc, char** argv )
{

Expand All @@ -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;
Copy link
Member

@hoffie hoffie Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


QString strArgument;
double rDbleArgument;
Expand Down Expand Up @@ -136,13 +173,15 @@ int main ( int argc, char** argv )
{
const QString strHelp = UsageArguments ( argv );
std::cout << qUtf8Printable ( strHelp );
cleanup();
exit ( 0 );
}

// Version number ------------------------------------------------------
if ( ( !strcmp ( argv[i], "--version" ) ) || ( !strcmp ( argv[i], "-v" ) ) )
{
std::cout << qUtf8Printable ( GetVersionAndNameStr ( false ) );
cleanup();
exit ( 0 );
}

Expand Down Expand Up @@ -563,6 +602,7 @@ int main ( int argc, char** argv )
// clicking on the Mac application bundle, the actual application
// is called with weird command line args -> do not exit on these
#if !( defined( Q_OS_MACX ) )
cleanup();
exit ( 1 );
#endif
}
Expand All @@ -584,6 +624,7 @@ int main ( int argc, char** argv )
if ( bIsClient )
{
qCritical() << "Only --server mode is supported in this build.";
cleanup();
exit ( 1 );
}
#endif
Expand All @@ -598,6 +639,7 @@ int main ( int argc, char** argv )
qCritical() << qUtf8Printable ( QString ( "%1: Server only option(s) '%2' used. Did you omit '--server'?" )
.arg ( argv[0] )
.arg ( ServerOnlyOptions.join ( ", " ) ) );
cleanup();
exit ( 1 );
}

Expand All @@ -622,6 +664,7 @@ int main ( int argc, char** argv )
{
qCritical() << qUtf8Printable (
QString ( "%1: Client only option(s) '%2' used. See '--help' for help" ).arg ( argv[0] ).arg ( ClientOnlyOptions.join ( ", " ) ) );
cleanup();
exit ( 1 );
}

Expand Down Expand Up @@ -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 );
Copy link
Member

@hoffie hoffie Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

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 !

Copy link
Member

@hoffie hoffie Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@hoffie hoffie Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ??

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

Expand Down Expand Up @@ -818,20 +861,20 @@ int main ( int argc, char** argv )
// clang-format on
#endif

CRpcServer* pRpcServer = nullptr;

if ( iJsonRpcPortNumber != INVALID_PORT )
{
if ( strJsonRpcSecretFileName.isEmpty() )
{
qCritical() << qUtf8Printable ( QString ( "- JSON-RPC: --jsonrpcsecretfile is required. Exiting." ) );
cleanup();
exit ( 1 );
}

QFile qfJsonRpcSecretFile ( strJsonRpcSecretFileName );
if ( !qfJsonRpcSecretFile.open ( QFile::OpenModeFlag::ReadOnly ) )
{
qCritical() << qUtf8Printable ( QString ( "- JSON-RPC: Unable to open secret file %1. Exiting." ).arg ( strJsonRpcSecretFileName ) );
cleanup();
exit ( 1 );
}
QTextStream qtsJsonRpcSecretStream ( &qfJsonRpcSecretFile );
Expand All @@ -841,6 +884,7 @@ int main ( int argc, char** argv )
qCritical() << qUtf8Printable ( QString ( "JSON-RPC: Refusing to run with secret of length %1 (required: %2). Exiting." )
.arg ( strJsonRpcSecret.length() )
.arg ( JSON_RPC_MINIMUM_SECRET_LENGTH ) );
cleanup();
exit ( 1 );
}

Expand All @@ -851,6 +895,7 @@ int main ( int argc, char** argv )
if ( !pRpcServer->Start() )
{
qCritical() << qUtf8Printable ( QString ( "- JSON-RPC: Server failed to start. Exiting." ) );
cleanup();
exit ( 1 );
}
}
Expand Down Expand Up @@ -884,7 +929,7 @@ int main ( int argc, char** argv )

if ( pRpcServer )
{
new CClientRpc ( &Client, pRpcServer, pRpcServer );
pClientRpc = new CClientRpc ( &Client, pRpcServer, pRpcServer );
}

# ifndef HEADLESS
Expand All @@ -903,15 +948,15 @@ int main ( int argc, char** argv )

// show dialog
ClientDlg.show();
pApp->exec();
exit_code = pApp->exec();
}
else
# endif
{
// only start application without using the GUI
qInfo() << qUtf8Printable ( GetVersionAndNameStr ( false ) );

pApp->exec();
exit_code = pApp->exec();
}
}
else
Expand Down Expand Up @@ -942,7 +987,7 @@ int main ( int argc, char** argv )

if ( pRpcServer )
{
new CServerRpc ( &Server, pRpcServer, pRpcServer );
pServerRpc = new CServerRpc ( &Server, pRpcServer, pRpcServer );
}

#ifndef HEADLESS
Expand All @@ -967,7 +1012,7 @@ int main ( int argc, char** argv )
ServerDlg.show();
}

pApp->exec();
exit_code = pApp->exec();
}
else
#endif
Expand All @@ -982,7 +1027,7 @@ int main ( int argc, char** argv )
Server.SetDirectoryType ( AT_CUSTOM );
}

pApp->exec();
exit_code = pApp->exec();
}
}
}
Expand All @@ -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 )
Copy link
Member

@ann0see ann0see Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks a bit odd in my eyes...

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;=)

Copy link
Member

@ann0see ann0see Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@pgScorpio pgScorpio Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
exit_code = 1;
}
}
}

#if defined( Q_OS_MACX )
activity.EndActivity();
#endif

return 0;
cleanup();

return exit_code;
}

/******************************************************************************\
Expand Down Expand Up @@ -1102,6 +1153,7 @@ bool GetStringArgument ( int argc, char** argv, int& i, QString strShortOpt, QSt
if ( ++i >= argc )
{
qCritical() << qUtf8Printable ( QString ( "%1: '%2' needs a string argument." ).arg ( argv[0] ).arg ( argv[i - 1] ) );
cleanup();
exit ( 1 );
}

Expand Down Expand Up @@ -1130,6 +1182,7 @@ bool GetNumericArgument ( int argc,
if ( ++i >= argc )
{
qCritical() << qUtf8Printable ( errmsg.arg ( argv[0] ).arg ( argv[i - 1] ).arg ( rRangeStart ).arg ( rRangeStop ) );
cleanup();
exit ( 1 );
}

Expand All @@ -1138,6 +1191,7 @@ bool GetNumericArgument ( int argc,
if ( *p || ( rValue < rRangeStart ) || ( rValue > rRangeStop ) )
{
qCritical() << qUtf8Printable ( errmsg.arg ( argv[0] ).arg ( argv[i - 1] ).arg ( rRangeStart ).arg ( rRangeStop ) );
cleanup();
exit ( 1 );
}

Expand Down