<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102238/">http://git.reviewboard.kde.org/r/102238/</a>
</td>
</tr>
</table>
<br />
<p>On August 8th, 2011, 2:25 p.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">(Sorry, flaky wifi lost the comment)
I am very much against a nested event loop (QEventLoop::exec), it's a well-known fact nowadays that it creates unexpected re-entrancy and crashes.
And since I just fixed the crash (missing wait() after terminate(), see commit log), I don't think we need this change. However reusing threads might be a good idea (separate issue).</pre>
</blockquote>
<p>On August 8th, 2011, 5:12 p.m., <b>Albert Astals Cid</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What you did seems like breaking the idea of the timeout parameter altogether since you are basically waiting (and blocking the UI) until my slow DNS answers instead of only waiting for the time the timeout specifies.</pre>
</blockquote>
<p>On August 8th, 2011, 7:14 p.m., <b>Dawit Alemayehu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I have the same concerns. See http://lists.kde.org/?l=kde-commits&m=131281315828663&w=2. Under most circumstances, I too am against using local event loops because they are simply evil for reasons David mentioned above. However, I rather make the exception and go that route in this case to avoid constant UI blocking for those with slow DNS.</pre>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">No no, I call wait after *terminate* ! Terminate is "almost instant termination", it kills the thread.
So this does not wait for the DNS lookup to be finished, it only waits for terminate to call the cleanup callback, which should be almost instant (it's just that it's in another thread).
I admit that I didn't fully test the case of a very slow DNS, but quick testing on the flaky network here seemed ok, and until someone proves the contrary, I firmly believe that this does not block for the whole duration of the dns lookup.
If it did, it would mean that terminate would basically do nothing ;)
And no, we definitely want no nested event loop. Anything else, but not that.</pre>
<br />
<p>- David</p>
<br />
<p>On August 7th, 2011, 4:07 a.m., Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated Aug. 7, 2011, 4:07 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch replaces the creation of a new thread for every DNS lookup with a local even loop based solution. This change addresses the issue of crashes that can arise from terminating the thread when the desired timeout duration is exceeded. See http://git.reviewboard.kde.org/r/102179/.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kio/kio/hostinfo.cpp <span style="color: grey">(fcb7889)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102238/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>