Review Request 118988: AsyncMusicBrainzLookup: fix a crash and a memory leak

[Frank Schaefer]

> On June 30, 2014, 9:28 a.m., Harald Sitter wrote:
> > Random thought of the day: have you considered using QtConcurrent::run? it appears to me that the task at hand seems pretty much like the thing it is supposed to help with optimizing. 
> > 
> > http://qt-project.org/doc/qt-4.8/qtconcurrentrun.html

I can't see any benefits of using QtConcurrent::run().


> On June 30, 2014, 9:28 a.m., Harald Sitter wrote:
> > libkcddb/musicbrainz/asyncmusicbrainzlookup.h, lines 24-25
> > <https://git.reviewboard.kde.org/r/118988/diff/1/?file=285067#file285067line24>
> >
> >     move to cpp

... (see below)


> On June 30, 2014, 9:28 a.m., Harald Sitter wrote:
> > libkcddb/musicbrainz/asyncmusicbrainzlookup.h, lines 30-53
> > <https://git.reviewboard.kde.org/r/118988/diff/1/?file=285067#file285067line30>
> >
> >     move to cpp

May I ask you why ?
IMHO, classes like this should be defined in the header file.

Anyway... with the class definition put into the cpp file and building from a clean tree:
...
Linking CXX shared library ../lib/libkcddb.so
CMakeFiles/kcddb.dir/musicbrainz/asyncmusicbrainzlookup.o: In function `KCDDB::AsyncMusicBrainzLookup::~AsyncMusicBrainzLookup()':
/home/frank/libkcddb/libkcddb/musicbrainz/asyncmusicbrainzlookup.cpp:62: undefined reference to `vtable for KCDDB::AsyncMusicBrainzLookup'
CMakeFiles/kcddb.dir/musicbrainz/asyncmusicbrainzlookup.o: In function `~AsyncMusicBrainzLookup':
/home/frank/libkcddb/libkcddb/musicbrainz/asyncmusicbrainzlookup.cpp:62: undefined reference to `vtable for KCDDB::AsyncMusicBrainzLookup'
CMakeFiles/kcddb.dir/musicbrainz/asyncmusicbrainzlookup.o: In function `KCDDB::AsyncMusicBrainzLookup::AsyncMusicBrainzLookup()':
/home/frank/libkcddb/libkcddb/musicbrainz/asyncmusicbrainzlookup.cpp:55: undefined reference to `vtable for KCDDB::AsyncMusicBrainzLookup'
CMakeFiles/kcddb.dir/musicbrainz/asyncmusicbrainzlookup.o: In function `KCDDB::AsyncMusicBrainzLookup::processLookupResult(KCDDB::Result, QList<KCDDB::CDInfo>)':
/home/frank/libkcddb/libkcddb/musicbrainz/asyncmusicbrainzlookup.cpp:91: undefined reference to `KCDDB::AsyncMusicBrainzLookup::finished(KCDDB::Result)'
collect2: error: ld returned 1 exit status
make[2]: *** [lib/libkcddb.so.4.13.2] Fehler 1
make[1]: *** [libkcddb/CMakeFiles/kcddb.dir/all] Fehler 2
make: *** [all] Fehler 2

Please note that this happens even though "asyncmusicbrainzlookup.moc" is included at the bottom.

Suggestions ? :-)


- Frank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118988/#review61259
-----------------------------------------------------------


On June 28, 2014, 10:01 a.m., Frank Schaefer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118988/
> -----------------------------------------------------------
> 
> (Updated June 28, 2014, 10:01 a.m.)
> 
> 
> Review request for KDE Multimedia.
> 
> 
> Bugs: 336678
>     http://bugs.kde.org/show_bug.cgi?id=336678
> 
> 
> Repository: libkcddb
> 
> 
> Description
> -------
> 
> 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.
> This causes crashes with the Audex CDDA extraction tool and likely also other users of libkcddb.
> 
> In addition to that, we are leaking the memory of a previously intstanciated lookup thread object if
> the lookup() method is called multiple times.
> 
> Solve both issues by making the lookup thread object "self-destructive".
> This way we can maintain the current behavior of the destructor (non-blocking) and
> the lookup() method (always start a new lookup without canceling pending lookups).
> 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
> 
> 
> Diffs
> -----
> 
>   libkcddb/musicbrainz/asyncmusicbrainzlookup.h 373fb16 
>   libkcddb/musicbrainz/asyncmusicbrainzlookup.cpp c13c94f 
> 
> Diff: https://git.reviewboard.kde.org/r/118988/diff/
> 
> 
> Testing
> -------
> 
> yes
> 
> 
> Thanks,
> 
> Frank Schaefer
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20140630/42cfefa5/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia