> 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
> 
>

Reply via email to