Review Request 127251: [KUnitConversion] Fix downloading currency exchange rates

[Kai Uwe Broulik]

> On April 24, 2016, 1:57 nachm., David Faure wrote:
> > Better, but I'm still wary of the reentrancy due to the nested event loop usage.
> > 
> > I bet this leads to a complete deadlock:
> > 
> >     QTimer::singleShot(0, this, SLOT(launchConversion()));
> >     launchConversion();
> > 
> > where launchConversion() triggers the KUnitConversion code that this patch is about.
> > 
> > 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.
> > 
> > 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.
> > 
> > Sorry for not realizing this earlier.

>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.

The m_cache variable could also be made local to the convert() function.


- Kai Uwe


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127251/#review94808
-----------------------------------------------------------


On April 24, 2016, 1:26 nachm., Kai Uwe Broulik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127251/
> -----------------------------------------------------------
> 
> (Updated April 24, 2016, 1:26 nachm.)
> 
> 
> Review request for KDE Frameworks and Vishesh Handa.
> 
> 
> Bugs: 345750
>     https://bugs.kde.org/show_bug.cgi?id=345750
> 
> 
> Repository: kunitconversion
> 
> 
> Description
> -------
> 
> QNetworkReply does not implement waitForReadyRead
> 
> Also, the code never actually created the cache dir it was trying to create a file in.
> 
> 
> Diffs
> -----
> 
>   src/currency.cpp ad325d8 
> 
> Diff: https://git.reviewboard.kde.org/r/127251/diff/
> 
> 
> Testing
> -------
> 
> Works now. It's downloaded once and then taken from cache file in ~/.local/share/libkunitconversion/currency.xml
> 
> 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.
> 
> Tests pass (only if I run them on English locale btw)
> 
> Obviously not happy with this being sync but alas that's how the API works.
> 
> 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.
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160424/99545047/attachment-0001.html>