| davidedmundson | |
| dfaure | |
| hein | |
| mpyne |
| Frameworks |
This problems persist from while a go, i'm not sure to date, but QML engine and/or engine to load icons do it on multiple threads. This result in invalid icons when cache is invalidated.
Definitely we talk to rare corner case, so resizing file can lead to undefined results , trying to protect it.
It looks like to KIconLoader::loadIcon is called from multiple threads, but i'm not sure, since i'm not read it on Qt sources or docs.
| Lint Skipped |
| Unit Tests Skipped |
It looks like to KIconLoader::loadIcon is called from multiple threads, but i'm not sure
It's important to know a bug before trying to fix the bug, otherwise you can't know what to fix.
Plasma::IconItem definitely does things in the main thread. That's the one we use in the taskmanager.
Also worth noting QIcon::fromTheme itself isn't thread safe, so if we did use it from multiple threads you'd have crashes anyway.
@davidedmundson i don't have much time to test right now, so that's why my reviews looks like *guessing*. I want to know you are sure that loadIcon is called safety, so maybe dbus notifier
connect(s_globalData, &KIconLoaderGlobalData::iconChanged, this, &KIconLoaderPrivate::_k_refreshIcons, Qt::QueuedConnection);
This is the wrong approach for thread safety (wrong level of locking). It should come with a multithreaded unittest so it can be checked for safety (with clang+tsan for instance). And yes the concept overall seems wrong anyway as QIcon isn't threadsafe in the first place, AFAIK.
A separate KIconLoader instance, used only to fetch paths and not icons, could possibly be used in a secondary thread, but only that. In which case the fix would look very different from this.
| src/kiconloader.cpp | ||
|---|---|---|
| 616 | This locks and unlocks three times in a row, which is not only slightly inefficient, it also means that another thread could interleave calls in between, and get completely inconsistent state. | |
| 1829 | similar here, if another thread changes mIconAvailability between the previous line and this one, you'll end up comparing iterators for a different container, that will never work | |
| src/kiconloader.cpp | ||
|---|---|---|
| 616 | Yeah, i saw it, i'll made a sequenceLock like this void sequenceLock(std::function<void()> func)
{
QMutexLocker locker(&mutex);
func():
}
//usage
S_D(d)->sequenceLocker([d, _appname, extraSearchPaths]() {
d->mIconCache->clear();
d->clear();
d->init(_appname, extraSearchPaths);
});Are you ok with such approach? | |
| 1829 | Here with sure with other approach. | |
| src/kiconloader.cpp | ||
|---|---|---|
| 616 | Even QMutexLocker locker(&mutex); will not present or it'll deadlock. Only sequenceLocker with func() in. | |
Fix deadlock, icon of DragonPlayer and its related is not showed, i have no idea why !?
You fixed the small details I found, but the big picture is still very wrong.
Using the same KIconLoader in multiple threads cannot work and doesn't make sense: one thread could addAppDir(), or reinit, messing up results for another thread.
Use a different KIconLoader per thread, and then the only thing you have to make threadsafe is any use of a global object, but not the kiconloader itself, which CANNOT be made threadsafe given its API (theme() is a perfect example).
| src/kiconeffect.cpp | ||
|---|---|---|
| 74 | unrelated to this patch, right? | |
| 76 | warning, very wrong if there's a 'q' pointer... | |
| src/kiconloader.cpp | ||
| 1327 | And now such things could be written in a much more standard and readable way with a QMutexLocker at the beginning of the method, rather then the S_D macro and proxy hack. | |
| 1655 | This is not threadsafe, it's wishful-thinking-threadsafe. So I'll say it again: use a different KIconLoader instance in every thread, MUCH simpler, MUCH safer. | |
| src/kiconloader.h | ||
| 482 ↗ | (On Diff #36492) | unrelated to this patch, please split this out |
Holy crap, now MutexGuard doesn't even guarantee that the mutex will be locked, and yet you proceed, without the mutex locked, creating race conditions.
Are you reading any of my comments?
A very very strong -2 to this patch.
Fix icon loading issue (Dragonplayer related)
*Achievements
Deleting cache in ~/.cache (kcache and / or qml cache) does not result in broken icons.
| tests/kiconloadertest.cpp | ||
|---|---|---|
| 19 | But why not use a different icon loader per thread? | |
| tests/kiconloadertest.cpp | ||
|---|---|---|
| 19 | Because that's scenario when you give global pointer to Qt, no? | |
Also notice functions after https://phabricator.kde.org/source/kiconthemes/browse/master/src/kiconloader.cpp$1693 there is no guaranteed that they should be run in same thread. I manage to downgrade Qt 5.11 and upgrade again there is not broken icons after.
Furthermore if we manage to make new instance of iconloader every time it will significantly increase memory usage, it can be manage to create iconloader per thread i.e. global map that holds per thread iconloader instances, it will have downside of bigger memory usage and mutex lookup in the map.
It happen again, so it's not a threading issue
@hein when you hover grouped task in taskmanager click in thumbnail end up with broken icons, sometimes, kcache in ~/.cache is generated at same that icons are broken in plasmashell
what you think?
https://phabricator.kde.org/source/kiconthemes/browse/master/src/kiconloader.cpp$603
So the problem maybe not here as IconLoader loads icons but in icon writer
@davidedmundson , @dfaure who writes icon-cache.kcache in ~/.cache ?
icon-cache.kcache is maintained by KSharedDataCache (the mIconCache object in KIconLoaderPrivate in kiconloader.cpp).
@mpyne , i'm unsure about file deletion and sync other than that it looks correct. Finding best lock algorithm can be *potential* problem, but surely it's not my best.
The patch looks OK but I'm a bit worried about trying to lock the cache at the same time we're potentially trying to delete a KSharedDataCache so I'm going to recompile and do some stress and sanity tests first.
This change has worked fine for me in my testing (including KDE games and Plasma themes) so I think it's fine to commit.
I've test it too, but after kernel upgrade i don't notice it anymore, it can be a kernel regression, it still looks valuable to you, @mpyne ?
It looks can happen https://www.reddit.com/r/kde/comments/9hovrv/ive_been_having_this_randomly_icons_appears_to_be/
But i'm not sure that patch can handle it.