<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://svn.reviewboard.kde.org/r/6271/">http://svn.reviewboard.kde.org/r/6271/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/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 2011-01-04 21:46:47.110697</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Changes</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;">Actually the original patch and my analysis is incorrect! The m_checkOnHold flag is intentionally set to true only the the first try because that is how handing an ioslave over to a new application supposed to work. IOW, you only want to check with the launcher when the KDE application and hence a new instance of the scheduler is initiated for the first time. The cause of bugs like the ones listed above an incorrect if statement order in KIO::SchedulerPrivate::heldSlaveFor. Unfortunately this this logic flaw seems to have been present since the implementation of this mechanism and was never discovered because it is very diffcult to spot or even encounter the bug. Certain conditions had to be met. Anyhow, the gist of the bug is order of the following if statements:
if (canJobReuse) {
if (job->url() == m_urlOnHold) {
// use slave...
} else {
// kill slave...
}
...
...
}
which should actually have been written as
if (job->url() == m_urlOnHold) {
if (canJobReuse) {
// use slave...
} else {
// kill slave...
}
...
...
}
Spot the difference ? The condition that checks whether or not the slave-on-hold should be reused (canJobReuse) should come after it is determined that the current request and the slave-on-hold are retrieving the same resource! Otherwise, any request unrelated to the slave-on-hold will cause the scheduler to kill the slave that is on hold! And that is exactly why it is so difficult to duplicate this problem. Unless the scheduler processes another request before the slave-on-hold can be reused, the problem will never be encountered! The best place to see this bug is attempting to download email attachments in chatty websites like gmail which makes XMLHttpRequests in the background. There this bug becomes very apparent.
The previous patch has been amended to fix this flaw instead of unnecessarily messing around with the m_checkOnHold flag...</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description (updated)</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;">Actually the original patch and my analysis is incorrect! The m_checkOnHold flag is intentionally set to true only the the first try because that is how handing an ioslave over to a new application supposed to work. IOW, you only want to check with the launcher when the KDE application and hence a new instance of the scheduler is initiated...
The bug that caused issues like the ones listed above is a logic flaw, an incorrect if statement order to be precise, in KIO::SchedulerPrivate::heldSlaveFor. I checked the source code of KDE 3.5.x branch and the bug was also present there and as such must have been there from the beginning. The gist of the problem is order of the following if statements:
if (canJobReuse) {
if (job->url() == m_urlOnHold) {
// use slave...
} else {
// kill slave...
}
}
which should actually have been written as
if (job->url() == m_urlOnHold) {
if (canJobReuse) {
// use slave...
} else {
// kill slave...
}
}
Spot the problem ? The ioslave should never be killed just because a new request gets processed before the slave-on-hold can be reused! And yet that is what the current code actually perpetuates that. It also explains why the problem seems random. So long as the scheduler does not process another request before the slave-on-hold is reused, the problem will never show up. Anyways, the patch has been amended to fix this flaw...
</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=123121">123121</a>,
<a href="https://bugs.kde.org/show_bug.cgi?id=148307">148307</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> (updated)</h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdelibs/kio/kio/scheduler.cpp <span style="color: grey">(1211439)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/6271/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>