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

[Albert Astals Cid]

> On Aug. 14, 2011, 11:02 p.m., Albert Astals Cid wrote:
> > When using your code with my simple test program
> > 
> > #include "hostinfo_p.h"
> > #include <QApplication>
> > #include <QHostInfo>
> > #include <QTime>
> > 
> > int main(int a, char **b)
> > {
> >         QApplication app(a, b);
> >         QTime t;
> >         t.start();
> >         qDebug() << KIO::HostInfo::lookupHost("www.google.com", 1).addresses();
> >         qDebug() << t.elapsed();
> > }
> > 
> > I get 
> > 
> > a.out(4386) KIO::HostInfo::lookupHost: Waiting one second for name lookup thread to start
> > a.out(4386) KIO::HostInfo::lookupHost: Waiting one second for name lookup thread to start
> > a.out(4386) KIO::HostInfo::lookupHost: Waiting one second for name lookup thread to start
> > a.out(4386) KIO::HostInfo::lookupHost: Waiting one second for name lookup thread to start
> > a.out(4386) KIO::HostInfo::lookupHost: Waiting one second for name lookup thread to start
> > a.out(4386) KIO::HostInfo::lookupHost: Failed to start name lookup thread for "www.google.com"
> > () 
> > 55 
> > kDebugStream called after destruction (from virtual KIO::NameLookUpThread::~NameLookUpThread() file kdelibs/kio/kio/hostinfo.cpp line 125)
> > still running ? false 
> > 
> > The kDebugStream line looks ugly and the waiting one second for name lookup are wrong too since what actually has happened is that the thread already finished so the name lookup worked
> > 
> > I am also concerned about you accessing the cache from the main thread and from the helper thread without a lock (it could cause a crash i think)
> 
> Dawit Alemayehu wrote:
>     > kDebugStream called after destruction (from virtual KIO::NameLookUpThread::~NameLookUpThread() file kdelibs/kio/kio/hostinfo.cpp line 125)
>     > still running ? false 
>     
>     That is there only for debugging session and was not meant to be left around.
>     
>     > The kDebugStream line looks ugly and the waiting one second for name lookup are wrong too since what actually has happened is that the thread 
>     > already finished so the name lookup worked.
>     
>     Actually the reason you encountered the error aboveis because I accidentally left the "m_started = false" at the end of the NameLookUpThread::run class. As such the fix is very easy. Remove that line. It was left in by mistake when I was experimenting with things. The same goes for the debug statements. They are there for the testing purposes only and can be easily commented out.
>     
>     > I am also concerned about you accessing the cache from the main thread and from the helper thread without a lock (it could cause a crash i think)
>     
>     huh ? The helper thread is waited upon to complete its job before any code in the main thread accesses the case ; so I fail to see how this can possibly result in what you are stating here.
>     
>

> huh ? The helper thread is waited upon to complete its job before any code in the main thread accesses the case ; so I fail to see how this can possibly result in what you are stating here.
Ok, so you are going to mark the method as thread unsafe then, right?


- Albert


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


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/9fca8504/attachment.htm>