Merge pull request #7797 from zzmp/fix/cache-thread-safety

Make ResourceCache::resetResourceCounters threadsafe

libraries/networking/src/ResourceCache.cpp

@@ -40,15 +40,14 @@ void ResourceCacheSharedItems::appendPendingRequest(QWeakPointer<Resource> resou
 
 QList<QSharedPointer<Resource>> ResourceCacheSharedItems::getPendingRequests() {
     QList<QSharedPointer<Resource>> result;
+    Lock lock(_mutex);
 
-    {
+    foreach(QSharedPointer<Resource> resource, _pendingRequests) {
-        Lock lock(_mutex);
+        if (resource) {
-        foreach(QSharedPointer<Resource> resource, _pendingRequests) {
+            result.append(resource);
-            if (resource) {
-                result.append(resource);
-            }
         }
     }
+
     return result;
 }
 
@@ -59,20 +58,20 @@ uint32_t ResourceCacheSharedItems::getPendingRequestsCount() const {
 
 QList<QSharedPointer<Resource>> ResourceCacheSharedItems::getLoadingRequests() {
     QList<QSharedPointer<Resource>> result;
+    Lock lock(_mutex);
 
-    {
+    foreach(QSharedPointer<Resource> resource, _loadingRequests) {
-        Lock lock(_mutex);
+        if (resource) {
-        foreach(QSharedPointer<Resource> resource, _loadingRequests) {
+            result.append(resource);
-            if (resource) {
-                result.append(resource);
-            }
         }
     }
+
     return result;
 }
 
 void ResourceCacheSharedItems::removeRequest(QWeakPointer<Resource> resource) {
     Lock lock(_mutex);
+
     // resource can only be removed if it still has a ref-count, as
     // QWeakPointer has no operator== implementation for two weak ptrs, so
     // manually loop in case resource has been freed.
@@ -88,11 +87,11 @@ void ResourceCacheSharedItems::removeRequest(QWeakPointer<Resource> resource) {
 }
 
 QSharedPointer<Resource> ResourceCacheSharedItems::getHighestPendingRequest() {
-    Lock lock(_mutex);
     // look for the highest priority pending request
     int highestIndex = -1;
     float highestPriority = -FLT_MAX;
     QSharedPointer<Resource> highestResource;
+    Lock lock(_mutex);
 
     for (int i = 0; i < _pendingRequests.size();) {
         // Clear any freed resources
@@ -262,12 +261,14 @@ void ResourceCache::refreshAll() {
     clearUnusedResource();
     resetResourceCounters();
 
-    _resourcesLock.lockForRead();
+    QHash<QUrl, QWeakPointer<Resource>> resources;
-    auto resourcesCopy = _resources;
+    {
-    _resourcesLock.unlock();
+        QReadLocker locker(&_resourcesLock);
+        resources = _resources;
+    }
 
     // Refresh all remaining resources in use
-    foreach (QSharedPointer<Resource> resource, resourcesCopy) {
+    foreach (QSharedPointer<Resource> resource, resources) {
         if (resource) {
             resource->refresh();
         }
@@ -317,17 +318,17 @@ void ResourceCache::setRequestLimit(int limit) {
 
 void ResourceCache::getResourceAsynchronously(const QUrl& url) {
     qCDebug(networking) << "ResourceCache::getResourceAsynchronously" << url.toString();
-    _resourcesToBeGottenLock.lockForWrite();
+    QWriteLocker locker(&_resourcesToBeGottenLock);
     _resourcesToBeGotten.enqueue(QUrl(url));
-    _resourcesToBeGottenLock.unlock();
 }
 
 void ResourceCache::checkAsynchronousGets() {
     assert(QThread::currentThread() == thread());
+    QWriteLocker locker(&_resourcesToBeGottenLock);
     if (!_resourcesToBeGotten.isEmpty()) {
-        _resourcesToBeGottenLock.lockForWrite();
         QUrl url = _resourcesToBeGotten.dequeue();
-        _resourcesToBeGottenLock.unlock();
+
+        locker.unlock();
         getResource(url);
     }
 }
@@ -399,8 +400,10 @@ void ResourceCache::removeUnusedResource(const QSharedPointer<Resource>& resourc
     if (_unusedResources.contains(resource->getLRUKey())) {
         _unusedResources.remove(resource->getLRUKey());
         _unusedResourcesSize -= resource->getBytes();
+
+        locker.unlock();
+        resetResourceCounters();
     }
-    resetResourceCounters();
 }
 
 void ResourceCache::reserveUnusedResource(qint64 resourceSize) {
@@ -413,7 +416,9 @@ void ResourceCache::reserveUnusedResource(qint64 resourceSize) {
         it.value()->setCache(nullptr);
         auto size = it.value()->getBytes();
 
+        locker.unlock();
         removeResource(it.value()->getURL(), size);
+        locker.relock();
 
         _unusedResourcesSize -= size;
         _unusedResources.erase(it);
@@ -433,8 +438,16 @@ void ResourceCache::clearUnusedResource() {
 }
 
 void ResourceCache::resetResourceCounters() {
-    _numTotalResources = _resources.size();
+    {
-    _numUnusedResources = _unusedResources.size();
+        QReadLocker locker(&_resourcesLock);
+        _numTotalResources = _resources.size();
+    }
+
+    {
+        QReadLocker locker(&_unusedResourcesLock);
+        _numUnusedResources = _unusedResources.size();
+    }
+
     emit dirty();
 }
 

libraries/networking/src/ResourceCache.h

@@ -64,6 +64,7 @@ class ResourceCacheSharedItems : public Dependency  {
 
     using Mutex = std::mutex;
     using Lock = std::unique_lock<Mutex>;
+
 public:
     void appendPendingRequest(QWeakPointer<Resource> newRequest);
     void appendActiveRequest(QWeakPointer<Resource> newRequest);
@@ -224,26 +225,30 @@ private:
     void resetResourceCounters();
     void removeResource(const QUrl& url, qint64 size = 0);
 
-    QReadWriteLock _resourcesLock { QReadWriteLock::Recursive };
+    void getResourceAsynchronously(const QUrl& url);
-    QHash<QUrl, QWeakPointer<Resource>> _resources;
+
-    int _lastLRUKey = 0;
-    
     static int _requestLimit;
     static int _requestsActive;
 
-    void getResourceAsynchronously(const QUrl& url);
+    // Resources
-    QReadWriteLock _resourcesToBeGottenLock { QReadWriteLock::Recursive };
+    QHash<QUrl, QWeakPointer<Resource>> _resources;
-    QQueue<QUrl> _resourcesToBeGotten;
+    QReadWriteLock _resourcesLock { QReadWriteLock::Recursive };
-    
+    int _lastLRUKey = 0;
-    std::atomic<size_t> _numTotalResources { 0 };
-    std::atomic<size_t> _numUnusedResources { 0 };
 
+    std::atomic<size_t> _numTotalResources { 0 };
     std::atomic<qint64> _totalResourcesSize { 0 };
+
+    // Cached resources
+    QMap<int, QSharedPointer<Resource>> _unusedResources;
+    QReadWriteLock _unusedResourcesLock { QReadWriteLock::Recursive };
+    qint64 _unusedResourcesMaxSize = DEFAULT_UNUSED_MAX_SIZE;
+
+    std::atomic<size_t> _numUnusedResources { 0 };
     std::atomic<qint64> _unusedResourcesSize { 0 };
 
-    qint64 _unusedResourcesMaxSize = DEFAULT_UNUSED_MAX_SIZE;
+    // Pending resources
-    QReadWriteLock _unusedResourcesLock { QReadWriteLock::Recursive };
+    QQueue<QUrl> _resourcesToBeGotten;
-    QMap<int, QSharedPointer<Resource>> _unusedResources;
+    QReadWriteLock _resourcesToBeGottenLock { QReadWriteLock::Recursive };
 };
 
 /// Base class for resources.