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

[Harald Sitter]

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


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


libkcddb/musicbrainz/asyncmusicbrainzlookup.h
<https://git.reviewboard.kde.org/r/118988/#comment42661>

    move to cpp



libkcddb/musicbrainz/asyncmusicbrainzlookup.h
<https://git.reviewboard.kde.org/r/118988/#comment42662>

    move to cpp


- Harald Sitter


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/04124cb1/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia