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