> 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
