Review Request 122183: [KUnitConversion] Currency: Fetch the currency file properly

[David Faure]

> On Jan. 22, 2015, 12:45 p.m., David Faure wrote:
> > src/currency.cpp, line 672
> > <https://git.reviewboard.kde.org/r/122183/diff/1/?file=343932#file343932line672>
> >
> >     Ouch.
> >     
> >     Hello unexpected reentrancy.
> >     
> >     If this code is async, it should provide a Job API instead of masquerading under a sync API with a nasty nested event loop.
> >     
> >     Or is this running in a separate thread?
> 
> Vishesh Handa wrote:
>     It isn't running in a separate thread.
>     
>     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());
>             }
>         }
>     
>     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.
>     
>     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.
>     
>     I think (3) would be the best option. Opinions?

Good catch about waitForReadyRead not waiting.

This means it kept the CPU busy, but "other than that" it worked, right?

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.

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

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.


- David


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


On Jan. 21, 2015, 2:47 p.m., Vishesh Handa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122183/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2015, 2:47 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Bugs: 340819
>     https://bugs.kde.org/show_bug.cgi?id=340819
> 
> 
> Repository: kunitconversion
> 
> 
> Description
> -------
> 
> Currency: Fetch the currency file properly
> 
> Properly run an event loop and wait for the file to be fetched.
> 
> Also add a test to make sure currency conversion is working.
> 
> This patch also contains -
> * https://git.reviewboard.kde.org/r/122182/
> * https://git.reviewboard.kde.org/r/122181/
> * https://git.reviewboard.kde.org/r/122180/
> 
> This is because reviewboard refuses to upload only a part of the diff. Please only look at currency.cpp w.r.t the EventLoop.
> 
> 
> Diffs
> -----
> 
>   autotests/convertertest.h 8129a48 
>   autotests/convertertest.cpp ae4298e 
>   src/currency.cpp 715233c 
>   src/unit.cpp 013b5d7 
>   src/unitcategory.cpp c34217e 
> 
> Diff: https://git.reviewboard.kde.org/r/122183/diff/
> 
> 
> Testing
> -------
> 
> Test now passes.
> 
> 
> Thanks,
> 
> Vishesh Handa
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150122/22f4139c/attachment.html>