<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/127251/">https://git.reviewboard.kde.org/r/127251/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 24th, 2016, 1:57 nachm. 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;">Better, but I'm still wary of the reentrancy due to the nested event loop usage.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I bet this leads to a complete deadlock:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #008000; font-weight: bold">QTimer</span><span style="color: #666666">:</span><span style="color: #AA22FF">:singleShot</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">0</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">this</span><span style="color: #666666">,</span> <span style="color: #008000; font-weight: bold">SLOT</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">launchConversion</span><span style="color: #666666">()));</span>
<span style="color: #008000; font-weight: bold">launchConversion</span><span style="color: #666666">();</span>
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">where launchConversion() triggers the KUnitConversion code that this patch is about.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While waiting for the first conversion, the timer will fire (given the nested QEventLoop usage), and we'll enter this code again, and hit mutex.lock(), and then wait forever, since we'll never go back to the event loop to finish the first conversion and unlock the mutex.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Maybe the mutex can be removed, actually. All that needs to be done is for m_update to be turned into a local variable (there's no reason for it to be a member variable, right?). The QSaveFile usage fixes the case where two threads would write to m_cache at the same time, or the case where one writes and one reads. Or is the mutex needed for the if() at the top? In any case I recommend not holding the mutex while being in the event loop.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sorry for not realizing this earlier.</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;">From what I can tell the m_update is so it updates it either on first run or when the cache file is outdated/missing. I don't know the internals of KUnitConversion, ie. if an instance of Currency is shared or not, and how to ensure that for subsequent runs of convert() I don't needlessly probe the cache file again.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The m_cache variable could also be made local to the convert() function.</p></pre>
<br />
<p>- Kai Uwe</p>
<br />
<p>On April 24th, 2016, 1:26 nachm. UTC, Kai Uwe Broulik 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 and Vishesh Handa.</div>
<div>By Kai Uwe Broulik.</div>
<p style="color: grey;"><i>Updated April 24, 2016, 1:26 nachm.</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=345750">345750</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;">QNetworkReply does not implement waitForReadyRead</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, the code never actually created the cache dir it was trying to create a file in.</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;">Works now. It's downloaded once and then taken from cache file in ~/.local/share/libkunitconversion/currency.xml</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Given it's a Tier 2 framework doesn't make sense to add KIO now, also failed to reproduce the crashes mentioned in the code.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Tests pass (only if I run them on English locale btw)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Obviously not happy with this being sync but alas that's how the API works.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not sure if this is a KRunner bug or KUnitConverison but if I enter "5 USD" it converts fine, if I enter "5 usd" it returns zero for all the currencies it converted to.</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>src/currency.cpp <span style="color: grey">(ad325d8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127251/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>