Review Request: Avoid terminating a QThread in kio/kio/hostinfo.cpp

[Dawit Alemayehu]

> On Aug. 18, 2011, 8:39 p.m., Albert Astals Cid wrote:
> > kio/kio/hostinfo.cpp, line 406
> > <http://git.reviewboard.kde.org/r/102238/diff/5/?file=32029#file32029line406>
> >
> >     You are connecting thread finished to lookupFinished to try to reuse the thread and then in the constructor you are connecting thread finished to deleteLater. How is this going to work?

That is a mistake. The connection in the constructor was supposed to be gone since the thread is now reused. The deleteLater slot of the thread should instead be connected to HostInfoAgentPrivate's destroyed signal. Thanks for the catch, I will update the patch.


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102238/#review5799
-----------------------------------------------------------


On Aug. 17, 2011, 5:40 a.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102238/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2011, 5:40 a.m.)
> 
> 
> Review request for kdelibs, David Faure and Thiago Macieira.
> 
> 
> Summary
> -------
> 
> The attached patch is an alternate approach to address the issue of crashes that arise from terminating an active thread than the one proposed at https://git.reviewboard.kde.org/r/102179/. With this patch the function "QHostInfo::lookupHost(QString, int)" avoids the use of QThread::terminate with the following fairly simple changes:
> 
> - Connect its finished signal to its parent deleteLater slot in the ctor so that the thread is automatically deleted later.
> - Store the looked up DNS info in  the global cache to avoid unnecessary queries for the same request.
> - Check for cached DNS information and avoid doing reverse look ups before resorting to performing DNS queries in a separate thread.
> 
> 
> Diffs
> -----
> 
>   kio/kio/hostinfo.cpp 344b1d8 
> 
> Diff: http://git.reviewboard.kde.org/r/102238/diff
> 
> 
> Testing
> -------
> 
> Tested with the following code based on Albert's post. 
> 
> #include "hostinfo_p.h"
> #include <QtGui/QApplication>
> #include <QtCore/QElapsedTimer>
> #include <QtNetwork/QHostInfo>
> 
> int main(int a, char **b)
> {
>     QApplication app(a, b);
>     QElapsedTimer t;
>     t.start();
>     qDebug() << KIO::HostInfo::lookupHost("www.kde.org", 0).addresses();
>     qDebug() << "Time:" << t.elapsed() << "ms";
> }
> 
> 
> Thanks,
> 
> Dawit
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20110818/853d6648/attachment.htm>