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