Commit dab0a7a6 authored by Frank Schaefer's avatar Frank Schaefer
Browse files

AsyncMusicBrainzLookup: fix a crash and a memory leak

From the documentation of QThread::~QThread():
"... Note that deleting a QThread object will not stop the execution of the
 thread it manages. Deleting a running QThread (i.e. isFinished() returns false)
 will probably result in a program crash.
 Wait for the finished() signal before deleting the QThread."

The destructor of class AsyncMusicBrainzLookup currently just deletes the lookup
thread object and does _not_ wait for the running thread to finish.
If it is called while the lookup thread is still running, a crash occurs.

In addition to that, we are leaking the memory of a previously instanciated
lookup thread object if the lookup() method is called multiple times.

To maintain the current behavior of the destructor (non-blocking) and the
lookup() method (always start a new lookup without canceling pending lookups),
solve both issues by making the lookup thread object self-destructive.
As part of the solution the lookup thread is modifed to deliver the lookup results
directly with the "lookupFinished" signal. This has two benefits:
1.) the queued signal-slot connection avoids thread synchronization/locking issues
2.) some class members become obsolete
parent 53d598a9
......@@ -15,51 +15,67 @@ namespace KCDDB
{
class LookupThread : public QThread
{
Q_OBJECT
public:
void run() override
{
Result result;
CDInfoList lookupResponse;
MusicBrainzLookup lookup;
m_result = lookup.lookup(QString(), 0, m_offsetList);
result = lookup.lookup(QString(), 0, m_offsetList);
if (m_result == Success)
m_lookupResponse = lookup.lookupResponse();
if (result == Success)
lookupResponse = lookup.lookupResponse();
Q_EMIT lookupFinished(result, lookupResponse);
}
TrackOffsetList m_offsetList;
Result m_result;
CDInfoList m_lookupResponse;
Q_SIGNALS:
void lookupFinished( KCDDB::Result, KCDDB::CDInfoList );
} ;
AsyncMusicBrainzLookup::AsyncMusicBrainzLookup()
{
// Register custom data types for the signal-slot connection with the lookup thread:
qRegisterMetaType<KCDDB::Result>("KCDDB::Result");
qRegisterMetaType<KCDDB::CDInfoList>("KCDDB::CDInfoList");
}
AsyncMusicBrainzLookup::~AsyncMusicBrainzLookup()
{
delete m_lookupThread;
}
Result AsyncMusicBrainzLookup::lookup( const QString &, uint, const TrackOffsetList & trackOffsetList )
{
m_lookupThread = new LookupThread();
m_lookupThread->m_offsetList = trackOffsetList;
connect(m_lookupThread, &QThread::finished, this, &AsyncMusicBrainzLookup::lookupFinished);
LookupThread* lookupThread = new LookupThread();
lookupThread->m_offsetList = trackOffsetList;
connect(lookupThread, &LookupThread::lookupFinished, this, &AsyncMusicBrainzLookup::processLookupResult); // queued connection
m_lookupThread->start();
// Make the thread object "self-destructive"; allows us to keep the destructor non-blocking
connect(lookupThread, &LookupThread::finished, lookupThread, &LookupThread::deleteLater);
// NOTE: the memory automatically gets cleared after the thread has finished
lookupThread->start();
return Success;
}
void AsyncMusicBrainzLookup::lookupFinished()
void AsyncMusicBrainzLookup::processLookupResult( KCDDB::Result result, KCDDB::CDInfoList lookupResponse )
{
qDebug() ;
cdInfoList_ = m_lookupThread->m_lookupResponse;
cdInfoList_ = lookupResponse;
Q_EMIT finished(m_lookupThread->m_result);
Q_EMIT finished(result);
}
}
#include "asyncmusicbrainzlookup.moc"
// vim:tabstop=2:shiftwidth=2:expandtab:cinoptions=(s,U1,m1
......@@ -32,10 +32,7 @@ namespace KCDDB
void finished( KCDDB::Result );
protected Q_SLOTS:
void lookupFinished( );
private:
LookupThread* m_lookupThread;
void processLookupResult( KCDDB::Result result, KCDDB::CDInfoList lookupResponse );
};
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment