> 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. > > 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. > > David Faure wrote: > 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.
So, in a nutshell I just lock the mutex before writing to m_update / m_unitMap and unlock again rather than always locking at the start of the method that may spawn a nested event loop? - 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
