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

[Frank Schaefer]

> On Juni 30, 2014, 9:28 vorm., 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
> 
> Frank Schaefer wrote:
>     I can't see any benefits of using QtConcurrent::run().
> 
> Harald Sitter wrote:
>     thread resuage, no manual qthread definition, it does *exactly* what the qthread would do except without all the cruft

I have very well understood how QtConcurrent::run() works.
But I'm not going to rewrite almost the whole thing just to save the deleterLater() call.
Sorry, -ENOTIME.


> On Juni 30, 2014, 9:28 vorm., 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
> 
> Frank Schaefer wrote:
>     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 ? :-)
> 
> Harald Sitter wrote:
>     Pointless IO for headers including this header is why it should not be in the header. Generally I will argue that exposing insides to the outside is not good (regardless of whether a class is public or not). There quite simply is no benefit to be had from defining the thread in the .h
>     
>     As for the moc issue, best completely random guess is that the cpp isn't actually moc'd which entirely depends on what exactly the cmakelists does, but I really cannot be bothered to look because... QtConcurrent::run() would avoid this question entirely as using ::run there would be no need for a class that needed to access the vtable ;)

I don't think so. Have you really understood the problems of the code ?

If think you can do it better, feel free to send your own patches.

I've spent more than enough time on this issue and even the two other simple patches are bitrotting since 6 weeks now.
Good luck !


- Frank


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


On Juni 28, 2014, 10:01 vorm., Frank Schaefer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118988/
> -----------------------------------------------------------
> 
> (Updated Juni 28, 2014, 10:01 vorm.)
> 
> 
> 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/20140808/3fef214e/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia