<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/122183/">https://git.reviewboard.kde.org/r/122183/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 22nd, 2015, 12:45 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/122183/diff/1/?file=343932#file343932line672" style="color: black; font-weight: bold; text-decoration: underline;">src/currency.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Value CurrencyCategoryPrivate::convert(const Value &value, const Unit &to)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">672</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">loop</span><span class="p">.</span><span class="n">exec</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<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;">Ouch.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hello unexpected reentrancy.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If this code is async, it should provide a Job API instead of masquerading under a sync API with a nasty nested event loop.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or is this running in a separate thread?</p></pre>
</blockquote>
<p>On January 22nd, 2015, 2:07 p.m. UTC, <b>Vishesh Handa</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;">It isn't running in a separate thread.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hmm. This is strange. The previous code is as follows -
QNetworkReply *reply = manager.get(QNetworkRequest(QUrl(URL)));
QFile cacheFile(m_cache);
cacheFile.open(QFile::WriteOnly);
while (!reply->error() && !reply->atEnd()) {
if (reply->bytesAvailable()>0 || reply->waitForReadyRead(500)) {
cacheFile.write(reply->readAll());
}
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QNetworkReply::waitForReadyRead is not implemented, so it uses QIODeivce::waitForReadyRead which just returns false. So this code basicallly keeps looping until we get a reply.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Possible ways to fix this -
1. Event Loop but only process some events
2. A loop as before
3. Async code where we fetch the currency rates in the background and for this result we convert it with the previous rates.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think (3) would be the best option. Opinions?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">Good catch about waitForReadyRead not waiting.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This means it kept the CPU busy, but "other than that" it worked, right?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Solution 1 isn't available in Qt - unless this code is moved to a thread with an event loop, and then the main thread can acquire() on a semaphore until the response is available. This is what I did in KDSoap, this trick allows to implement synchronous behavior on top of async without an event loop in the main thread, but really blocking. However if this code is ever called from the main thread of a GUI application then it'll block completely. So, not good.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Solution 2 (looping) would also block the GUI (even with a small sleep() in there to avoid using too much CPU - even more so, then).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Solution 3 sounds good in terms of performance and GUI-responsiveness, I assume it's OK API wise too, i.e. the users of the library wouldn't consider it a bug? I like it, it should be clean to implement, too.</p></pre>
<br />
<p>- David</p>
<br />
<p>On January 21st, 2015, 2:47 p.m. UTC, Vishesh Handa 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 KDE Frameworks.</div>
<div>By Vishesh Handa.</div>
<p style="color: grey;"><i>Updated Jan. 21, 2015, 2:47 p.m.</i></p>
<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=340819">340819</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kunitconversion
</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;">Currency: Fetch the currency file properly</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Properly run an event loop and wait for the file to be fetched.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also add a test to make sure currency conversion is working.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch also contains -
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> https://git.reviewboard.kde.org/r/122182/
</em> https://git.reviewboard.kde.org/r/122181/
* https://git.reviewboard.kde.org/r/122180/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is because reviewboard refuses to upload only a part of the diff. Please only look at currency.cpp w.r.t the EventLoop.</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;">Test now passes.</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>autotests/convertertest.h <span style="color: grey">(8129a48)</span></li>
<li>autotests/convertertest.cpp <span style="color: grey">(ae4298e)</span></li>
<li>src/currency.cpp <span style="color: grey">(715233c)</span></li>
<li>src/unit.cpp <span style="color: grey">(013b5d7)</span></li>
<li>src/unitcategory.cpp <span style="color: grey">(c34217e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122183/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>