[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Review Request: Do not terminate threads
From: Dawit A <adawit () kde ! org>
Date: 2011-08-07 4:10:58
Message-ID: CALa28R47AfjYGysUnUuwMXs0cq3s9zQtwco486zuQJit=OSjFA () mail ! gmail ! com
[Download RAW message or body]
On Sat, Aug 6, 2011 at 7:46 AM, Albert Astals Cid <aacid@kde.org> wrote:
> On Dijous 04 Agost 2011 15:28:49 Dawit A wrote:
>> On Thu, Aug 4, 2011 at 5:31 AM, Albert Astals Cid <tsdgeos@terra.es> wro=
te:
>> > =C2=A0 =C2=A0This is an automatically generated e-mail. To reply, visi=
t:
>> > http://git.reviewboard.kde.org/r/102179/
>> >
>> > On August 4th, 2011, 3:19 a.m., *Dawit Alemayehu* wrote:
>> >
>> > I do not like this patch for the very reason you stated. I do not want
>> > the mutex there either because it is rather expensive. As it stands we
>> > start a thread for each lookup right now which in of itself is already
>> > too expensive for my taste. Hence, I will have to think of some other
>> > way to avoid the even worse solution of creating a local even loop.
>> >
>> > Having said that, =C2=A0are you using a slow DNS server ? The only way=
the
>> > lookup thread gets terminated is if your nameserver takes longer than
>> > 1000 ms per query. That is because the two KUriFilterPlugins that use =
it
>> > set a timeout value of that duration. Otherwise, that code path should
>> > not be encountered at all!
>> >
>> > =C2=A0So you do not want a patch that fixes konqueror crashing 75% tim=
es i use
>> > =C2=A0it in exchange of having a small memory leak once in a blue moon=
?
>> > =C2=A0Awesome.
>> >
>> Did I say I do not want the patch ??
>
> That's what i understood when reading your comment, sorry if it is not wh=
at
> you meant.
Well, it is always misunderstandings that cause issues. I am sure I
too would have misunderstood my response if I had bothered to re-read
it. :) Sorry about that. It did not come out how I intended it to come
out.
>> All I said was need to think of a
>> better way to fix this patch! Clam down.
>
> Since you do not want a mutex and you do not want my current solution eit=
her I
> can not think of much more ways of fixing this.
>
> Maybe by doing what Thiago suggests and instead of starting a short lived
> thread each time start a long lived one, though we will still need the mu=
tex
> if we want it to be 100% correct.
I think the event loop based solution is probably the way to go. It is
what both the localdomain and fixhost uri filter plugins used to use
before I refactored the code out and put in the parent class to reduce
the DNS lookups and also avoid code duplication. Anyhow, I have posted
a patch that uses the aforementioned event loop solution at
https://git.reviewboard.kde.org/r/102238/. Can you please see if it
resolves your issues ?
> If you are at the Berlin Desktop Summit maybe we can meet and try to find=
a
> solution together. You can usually find me surrounded by a bunch of other
> spaniards.
Unfortunately, I won't be there.
["kio_hostinfo.patch" (application/octet-stream)]
diff --git kio/kio/hostinfo.cpp kio/kio/hostinfo.cpp
index fcb7889..c1c76a3 100644
--- kio/kio/hostinfo.cpp
+++ kio/kio/hostinfo.cpp
@@ -32,6 +32,7 @@ License along with this library. If not, see <http://www.gnu.org/licenses/>.
#include <QtCore/QFutureWatcher>
#include <QtCore/QMetaType>
#include <QtCore/QtConcurrentRun>
+#include <QtCore/QEventLoop>
#include <QtNetwork/QHostInfo>
#include "kdebug.h"
@@ -58,12 +59,16 @@ namespace KIO
HostInfoAgentPrivate(int cacheSize = 100);
virtual ~HostInfoAgentPrivate() {};
void lookupHost(const QString& hostName, QObject* receiver, const char* member);
+ void lookupHost(const QString& hostName, unsigned long timeout, QHostInfo* info);
QHostInfo lookupCachedHostInfoFor(const QString& hostName);
void cacheLookup(const QHostInfo&);
void setCacheSize(int s) { dnsCache.setMaxCost(s); }
void setTTL(int _ttl) { ttl = _ttl; }
- private slots:
+ private Q_SLOTS:
void queryFinished(const QHostInfo&);
+ void lookupResult(const QHostInfo&);
+ Q_SIGNALS:
+ void lookupDone();
private:
class Result;
class Query;
@@ -77,7 +82,7 @@ namespace KIO
class HostInfoAgentPrivate::Result : public QObject
{
Q_OBJECT
- signals:
+ Q_SIGNALS:
void result(QHostInfo);
private:
friend class HostInfoAgentPrivate;
@@ -101,9 +106,9 @@ namespace KIO
{
return m_hostName;
}
- signals:
+ Q_SIGNALS:
void result(QHostInfo);
- private slots:
+ private Q_SLOTS:
void relayFinished()
{
emit result(m_watcher.result());
@@ -112,57 +117,6 @@ namespace KIO
QFutureWatcher<QHostInfo> m_watcher;
QString m_hostName;
};
-
- class NameLookUpThread : public QThread
- {
- public:
- NameLookUpThread (const QString& name)
- :QThread (0), m_hostName(name), m_started(false)
- {
- }
-
- QHostInfo result() const
- {
- return m_hostInfo;
- }
-
- bool wasStarted() const
- {
- return m_started;
- }
-
- void run()
- {
- m_started = true;
- m_hostInfo = QHostInfo();
-
- // Do not perform a reverse lookup here...
- QHostAddress address (m_hostName);
- if (!address.isNull()) {
- QList<QHostAddress> addressList;
- addressList << address;
- m_hostInfo.setAddresses(addressList);
- return;
- }
-
- // Look up the name in the KIO/KHTML DNS cache...
- m_hostInfo = HostInfo::lookupCachedHostInfoFor(m_hostName);
- if (!m_hostInfo.hostName().isEmpty() && m_hostInfo.error() == QHostInfo::NoError) {
- return;
- }
-
- // Failing all of the above, do the lookup...
- m_hostInfo = QHostInfo::fromName(m_hostName);
- if (!m_hostInfo.hostName().isEmpty() && m_hostInfo.error() == QHostInfo::NoError) {
- HostInfo::cacheLookup(m_hostInfo); // cache the look up...
- }
- }
-
- private:
- QHostInfo m_hostInfo;
- QString m_hostName;
- bool m_started;
- };
}
using namespace KIO;
@@ -177,24 +131,9 @@ void HostInfo::lookupHost(const QString& hostName, QObject* receiver,
QHostInfo HostInfo::lookupHost(const QString& hostName, unsigned long timeout)
{
- NameLookUpThread lookupThread (hostName);
- lookupThread.start();
-
- // Wait for it to start...
- while (!lookupThread.wasStarted()) {
- kDebug() << "Waiting for name lookup thread to start";
- lookupThread.wait(1000);
- }
-
- // Now wait for it to complete...
- if (!lookupThread.wait(timeout)) {
- kDebug() << "Name look up for" << hostName << "failed";
- lookupThread.terminate();
- return QHostInfo();
- }
-
- //kDebug(7022) << "Name look up succeeded for" << hostName;
- return lookupThread.result();
+ QHostInfo hostInfo;
+ hostInfoAgentPrivate->lookupHost(hostName, timeout, &hostInfo);
+ return hostInfo;
}
QHostInfo HostInfo::lookupCachedHostInfoFor(const QString& hostName)
@@ -209,7 +148,8 @@ void HostInfo::cacheLookup(const QHostInfo& info)
void HostInfo::prefetchHost(const QString& hostName)
{
- hostInfoAgentPrivate->lookupHost(hostName, 0, 0);
+ QObject* reciever = 0;
+ hostInfoAgentPrivate->lookupHost(hostName, reciever, 0);
}
void HostInfo::setCacheSize(int s)
@@ -273,6 +213,50 @@ void HostInfoAgentPrivate::lookupHost(const QString& hostName,
query->start(hostName);
}
+void HostInfoAgentPrivate::lookupHost(const QString& hostName,
+ unsigned long timeout, QHostInfo* info)
+{
+ Q_ASSERT(info);
+ if (!info) {
+ return;
+ }
+
+ // Do not perform a reverse lookup here...
+ QHostAddress address (hostName);
+ if (!address.isNull()) {
+ QList<QHostAddress> addressList;
+ addressList << address;
+ info->setAddresses(addressList);
+ return;
+ }
+
+ // Look up the name in the KIO/KHTML DNS cache...
+ *info = lookupCachedHostInfoFor(hostName);
+ if (!info->hostName().isEmpty() && info->error() == QHostInfo::NoError) {
+ return;
+ }
+
+ // Failing all of the above, do the lookup using local event loop to honor timeout.
+ QTimer timer;
+ timer.setInterval(timeout);
+ timer.setSingleShot(true);
+ QEventLoop eventLoop;
+ connect(&timer, SIGNAL(timeout()), &eventLoop, SLOT(quit()));
+ connect(this, SIGNAL(lookupDone()), &eventLoop, SLOT(quit()));
+ const int id = QHostInfo::lookupHost(hostName, this, SLOT(lookUpResult(QHostInfo)));
+ timer.start();
+ eventLoop.exec();
+
+ // timed out...
+ if (!timer.isActive()) {
+ return;
+ }
+
+ timer.stop();
+ QHostInfo::abortHostLookup(id);
+ *info = lookupCachedHostInfoFor(hostName);
+}
+
QHostInfo HostInfoAgentPrivate::lookupCachedHostInfoFor(const QString& hostName)
{
QPair<QHostInfo, QTime>* info = dnsCache.object(hostName);
@@ -304,4 +288,10 @@ void HostInfoAgentPrivate::queryFinished(const QHostInfo& info)
query->deleteLater();
}
+void HostInfoAgentPrivate::lookupResult(const QHostInfo& info)
+{
+ cacheLookup(info);
+ emit lookupDone();
+}
+
#include "hostinfo.moc"
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic