<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119211/">https://git.reviewboard.kde.org/r/119211/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 10th, 2014, 5:08 p.m. UTC, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But why CMD_REPARSECONFIGURATION especially?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If the slave is on hold, will anything else get through? IOW should the test just be extended with "|| suspended", no test on cmd?</p></pre>
</blockquote>
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I started out doing just that at first, but that resulted in a rather long pause whenever I clicked on a link that resulted in a download dialog. The pause seems to be caused by the the job getting suspended and then put on hold where the hold portion waits for the ioslave to be returned to klauncher by invoking "KToolInvocation::klauncher()->waitForSlave(d->m_pid)". That causes a delay because the command to put the ioslave on hold will be queued up due to the added "|| suspended" check and will only succeed once the call times after some period (I gu
ess 25 secs).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">When I thought more about this, I realized that this problem is only likely to affect this particular command because the scheduler is the one that controls how ioslaves are used and it does the right thing by not to sending commands to ioslaves on hold without first putting them back into service. Unfortunately CMD_REPARSECONFIGURATION is an exception. Hence, this fix that I am not totally sure belongs here. Perhaps the correct thing to do would be to fix the problem in the scheduler by putting the ioslave on hold back into service, sending it the reparse configuration request, and putting back on hold again? I dunno, but it seemed like an overkill by comparison to this patch.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyways, this is the reason why I did what I did. I knew it would probably be safe to send the reparse configuration request when the ioslave is resumed, but I was not sure that was the case for all other commands.</p></pre>
<br />
<p>- Dawit</p>
<br />
<p>On July 10th, 2014, 12:04 p.m. UTC, Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for kdelibs, David Faure and Thiago Macieira.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated July 10, 2014, 12:04 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch addresses the issue where attempting to send CMD_REPARSECONFIGURATION to suspended ioslaves results in a freezing of the application that sent the request. This problem can be clearly reproduced by following the steps outlined in the bug report.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am not entirely sure whether or not this fix really belongs in kio/connection.cpp, but I could not find a more convenient location to queue the command request to ensure the ioslave receives the reparse configuration command if it is ever reused again.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Run the steps given in the bug report before and after the fix.</p></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/connection.cpp <span style="color: grey">(99aea0b)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119211/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>