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

[David Faure]

> On April 24, 2016, 1:57 p.m., 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.
> 
> Kai Uwe Broulik wrote:
>     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.

I don't know much about kunitconversion either, so I had a look, and the reason that code leads to multithreading issue is indeed because all the instances of everything are shared between all threads (the entry point is the Converter class, and all Converter instances share the same global Private, which has maps to unit categories, which have maps to units. So yes, instances of CurrencyPrivate (which is the one that matters, not Currency) are shared (AFAICS).

The m_cache variable is a constant value set in the constructor, there's no problem with it being a member, and it doesn't need to be read with the mutex locked. The instance of CurrencyPrivate is created by the first thread which creates a Converter... hmm it's converter.cpp itself that is missing a mutex to protect m_categories, but that's another story.

I see the point about m_update, I was wrong about it not being useful as a member. Forgot the case where the cache already exists on disk, so we need to parse it on first run. That should however be done with the mutex locked, otherwise two threads might be loading the cache and setting the conversion factors into Units at the same time.

Anyway we're also back to: we need to handle the case of same-thread re-entrancy into this same code, due to the nested event loop. I don't see a way to wait for the download, from a nested event loop. So I see two solutions:
- starting another download and waiting for it (which requires that the mutex is not locked during the event loop; it has to be relocked before setting m_update though)
- or returning an error in this case (could be detected by keeping a list of QThread pointers, and adding currentThread to that list before starting the download). It's a corner case hopefully, but an error would certainly be better than a deadlock.

>From a correctness point of view the first solution seems better. It's also simpler, it just means moving the use of the mutex around the data that is written and read by multiple threads (as per definition of mutexes)... and that's only m_update and m_unitMap, AFAICS.


- David


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


On April 24, 2016, 1:26 p.m., 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 p.m.)
> 
> 
> 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/20160506/25db285e/attachment-0001.html>