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 all commits
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
102 changes: 86 additions & 16 deletions src/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,54 @@ 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* pCoreApplicationInstance = NULL;
QApplication* pApplicationInstance = NULL;

CRpcServer* pRpcServer = NULL;
#ifndef SERVER_ONLY
CClientRpc* pClientRpc = NULL;
#endif
CServerRpc* pServerRpc = NULL;

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

#ifndef SERVER_ONLY
if ( pClientRpc )
{
delete pClientRpc;
pClientRpc = NULL;
}
#endif

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

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

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

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

Expand All @@ -63,6 +111,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 +185,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 +614,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 +636,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 +651,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 +676,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 @@ -761,16 +816,24 @@ int main ( int argc, char** argv )
// Application/GUI setup ---------------------------------------------------
// Application object
#ifdef HEADLESS
QCoreApplication* pApp = new QCoreApplication ( argc, argv );
QCoreApplication* pApp = pCoreApplicationInstance = 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 );
QApplication* pApp = pApplicationInstance = new QApplication ( argc, argv );
# else
QCoreApplication* pApp = bUseGUI ? new QApplication ( argc, argv ) : new QCoreApplication ( argc, argv );
QCoreApplication* pApp;
if ( bUseGUI )
{
pApp = pApplicationInstance = new QApplication ( argc, argv );
}
else
{
pApp = pCoreApplicationInstance = new QCoreApplication ( argc, argv );
}
# endif
#endif

Expand Down Expand Up @@ -818,20 +881,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 +904,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 +915,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 +949,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 +968,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 +1007,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 +1032,7 @@ int main ( int argc, char** argv )
ServerDlg.show();
}

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

pApp->exec();
exit_code = pApp->exec();
}
}
}
Expand All @@ -999,15 +1064,17 @@ int main ( int argc, char** argv )
#endif
{
qCritical() << qUtf8Printable ( QString ( "%1: %2" ).arg ( APP_NAME ).arg ( generr.GetErrorText() ) );
exit ( 1 );
}

if ( exit_code == 0 )
{
exit_code = 1;
}
}

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

return 0;
return exit_code;
}

/******************************************************************************\
Expand Down Expand Up @@ -1102,6 +1169,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 +1198,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 +1207,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