From afdfef148200c5256266c9c01b6e3f00ab0a050e Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Sun, 10 Apr 2016 21:54:49 -0700 Subject: [PATCH 1/9] Make sure we don't use raw resource ptr --- .../GlobalServicesScriptingInterface.cpp | 4 +- interface/src/ui/Stats.cpp | 11 ++-- libraries/animation/src/AnimNodeLoader.cpp | 13 ++--- libraries/animation/src/AnimNodeLoader.h | 2 +- libraries/networking/src/ResourceCache.cpp | 51 ++++++++++--------- libraries/networking/src/ResourceCache.h | 35 +++++++------ .../src/AnimInverseKinematicsTests.cpp | 1 - tests/networking/src/ResourceTests.cpp | 12 +++-- 8 files changed, 71 insertions(+), 58 deletions(-) diff --git a/interface/src/scripting/GlobalServicesScriptingInterface.cpp b/interface/src/scripting/GlobalServicesScriptingInterface.cpp index 7dac0247bd..d56a987d2b 100644 --- a/interface/src/scripting/GlobalServicesScriptingInterface.cpp +++ b/interface/src/scripting/GlobalServicesScriptingInterface.cpp @@ -122,8 +122,8 @@ void DownloadInfoResultFromScriptValue(const QScriptValue& object, DownloadInfoR DownloadInfoResult GlobalServicesScriptingInterface::getDownloadInfo() { DownloadInfoResult result; - foreach(Resource* resource, ResourceCache::getLoadingRequests()) { - result.downloading.append(resource->getProgress() * 100.0f); + foreach(QSharedPointer resource, ResourceCache::getLoadingRequests()) { + result.downloading.append(resource ? (resource->getProgress() * 100.0f) : 0.0f); } result.pending = ResourceCache::getPendingRequestCount(); return result; diff --git a/interface/src/ui/Stats.cpp b/interface/src/ui/Stats.cpp index 39a3a1f192..d19f7e2cab 100644 --- a/interface/src/ui/Stats.cpp +++ b/interface/src/ui/Stats.cpp @@ -196,7 +196,7 @@ void Stats::updateStats(bool force) { STAT_UPDATE(audioMixerPps, -1); } - QList loadingRequests = ResourceCache::getLoadingRequests(); + QList> loadingRequests = ResourceCache::getLoadingRequests(); STAT_UPDATE(downloads, loadingRequests.size()); STAT_UPDATE(downloadLimit, ResourceCache::getRequestLimit()) STAT_UPDATE(downloadsPending, ResourceCache::getPendingRequestCount()); @@ -205,7 +205,8 @@ void Stats::updateStats(bool force) { bool shouldUpdateUrls = _downloads != _downloadUrls.size(); if (!shouldUpdateUrls) { for (int i = 0; i < _downloads; i++) { - if (loadingRequests[i]->getURL().toString() != _downloadUrls[i]) { + auto request = loadingRequests[i].lock(); + if (!request || request->getURL().toString() != _downloadUrls[i]) { shouldUpdateUrls = true; break; } @@ -214,8 +215,10 @@ void Stats::updateStats(bool force) { // If the urls have changed, update the list if (shouldUpdateUrls) { _downloadUrls.clear(); - foreach (Resource* resource, loadingRequests) { - _downloadUrls << resource->getURL().toString(); + foreach (QSharedPointer request, loadingRequests) { + if (request) { + _downloadUrls << request->getURL().toString(); + } } emit downloadUrlsChanged(); } diff --git a/libraries/animation/src/AnimNodeLoader.cpp b/libraries/animation/src/AnimNodeLoader.cpp index 70cf7e248f..df39c55e5d 100644 --- a/libraries/animation/src/AnimNodeLoader.cpp +++ b/libraries/animation/src/AnimNodeLoader.cpp @@ -568,12 +568,13 @@ bool processStateMachineNode(AnimNode::Pointer node, const QJsonObject& jsonObj, } AnimNodeLoader::AnimNodeLoader(const QUrl& url) : - _url(url), - _resource(nullptr) { - - _resource = new Resource(url); - connect(_resource, &Resource::loaded, this, &AnimNodeLoader::onRequestDone); - connect(_resource, &Resource::failed, this, &AnimNodeLoader::onRequestError); + _url(url) +{ + _resource = QSharedPointer::create(url); + _resource->setSelf(_resource); + connect(_resource.data(), &Resource::loaded, this, &AnimNodeLoader::onRequestDone); + connect(_resource.data(), &Resource::failed, this, &AnimNodeLoader::onRequestError); + _resource->ensureLoading(); } AnimNode::Pointer AnimNodeLoader::load(const QByteArray& contents, const QUrl& jsonUrl) { diff --git a/libraries/animation/src/AnimNodeLoader.h b/libraries/animation/src/AnimNodeLoader.h index d6fdfa7e2c..a1bd4a1f17 100644 --- a/libraries/animation/src/AnimNodeLoader.h +++ b/libraries/animation/src/AnimNodeLoader.h @@ -41,7 +41,7 @@ protected slots: protected: QUrl _url; - Resource* _resource; + QSharedPointer _resource; private: // no copies diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index 2273902263..11d0f31217 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -110,7 +110,7 @@ QSharedPointer ResourceCache::getResource(const QUrl& url, const QUrl& } if (QThread::currentThread() != thread()) { - assert(delayLoad); + assert(delayLoad && !extra); getResourceAsynchronously(url); return QSharedPointer(); } @@ -198,17 +198,17 @@ void ResourceCache::updateTotalSize(const qint64& oldSize, const qint64& newSize emit dirty(); } -void ResourceCacheSharedItems::appendActiveRequest(Resource* resource) { +void ResourceCacheSharedItems::appendActiveRequest(QWeakPointer resource) { Lock lock(_mutex); _loadingRequests.append(resource); } -void ResourceCacheSharedItems::appendPendingRequest(Resource* resource) { +void ResourceCacheSharedItems::appendPendingRequest(QWeakPointer resource) { Lock lock(_mutex); _pendingRequests.append(resource); } -QList> ResourceCacheSharedItems::getPendingRequests() const { +QList> ResourceCacheSharedItems::getPendingRequests() const { Lock lock(_mutex); return _pendingRequests; } @@ -218,23 +218,24 @@ uint32_t ResourceCacheSharedItems::getPendingRequestsCount() const { return _pendingRequests.size(); } -QList ResourceCacheSharedItems::getLoadingRequests() const { +QList> ResourceCacheSharedItems::getLoadingRequests() const { Lock lock(_mutex); return _loadingRequests; } -void ResourceCacheSharedItems::removeRequest(Resource* resource) { +void ResourceCacheSharedItems::removeRequest(QWeakPointer resource) { Lock lock(_mutex); - _loadingRequests.removeOne(resource); + _loadingRequests.removeAll(resource); } -Resource* ResourceCacheSharedItems::getHighestPendingRequest() { +QSharedPointer ResourceCacheSharedItems::getHighestPendingRequest() { Lock lock(_mutex); // look for the highest priority pending request int highestIndex = -1; float highestPriority = -FLT_MAX; + QSharedPointer highestResource; for (int i = 0; i < _pendingRequests.size();) { - Resource* resource = _pendingRequests.at(i).data(); + auto resource = _pendingRequests.at(i).lock(); if (!resource) { _pendingRequests.removeAt(i); continue; @@ -243,16 +244,25 @@ Resource* ResourceCacheSharedItems::getHighestPendingRequest() { if (priority >= highestPriority) { highestPriority = priority; highestIndex = i; + highestResource = resource; } i++; } if (highestIndex >= 0) { - return _pendingRequests.takeAt(highestIndex); + _pendingRequests.takeAt(highestIndex); } - return nullptr; + return highestResource; } -bool ResourceCache::attemptRequest(Resource* resource) { +const QList> ResourceCache::getLoadingRequests() { + return DependencyManager::get()->getLoadingRequests(); +} + +int ResourceCache::getPendingRequestCount() { + return DependencyManager::get()->getPendingRequestsCount(); +} + +bool ResourceCache::attemptRequest(QSharedPointer resource) { auto sharedItems = DependencyManager::get(); if (_requestsActive >= _requestLimit) { @@ -267,7 +277,7 @@ bool ResourceCache::attemptRequest(Resource* resource) { return true; } -void ResourceCache::requestCompleted(Resource* resource) { +void ResourceCache::requestCompleted(QSharedPointer resource) { auto sharedItems = DependencyManager::get(); sharedItems->removeRequest(resource); --_requestsActive; @@ -291,11 +301,6 @@ Resource::Resource(const QUrl& url, bool delayLoad) : _request(nullptr) { init(); - - // start loading immediately unless instructed otherwise - if (!(_startedLoading || delayLoad)) { - QTimer::singleShot(0, this, &Resource::ensureLoading); - } } Resource::~Resource() { @@ -303,7 +308,7 @@ Resource::~Resource() { _request->disconnect(this); _request->deleteLater(); _request = nullptr; - ResourceCache::requestCompleted(this); + ResourceCache::requestCompleted(_self); } } @@ -356,7 +361,7 @@ void Resource::refresh() { _request->disconnect(this); _request->deleteLater(); _request = nullptr; - ResourceCache::requestCompleted(this); + ResourceCache::requestCompleted(_self); } init(); @@ -401,7 +406,7 @@ void Resource::init() { void Resource::attemptRequest() { _startedLoading = true; - ResourceCache::attemptRequest(this); + ResourceCache::attemptRequest(_self); } void Resource::finishedLoading(bool success) { @@ -433,7 +438,7 @@ void Resource::makeRequest() { if (!_request) { qCDebug(networking).noquote() << "Failed to get request for" << _url.toDisplayString(); - ResourceCache::requestCompleted(this); + ResourceCache::requestCompleted(_self); finishedLoading(false); return; } @@ -465,7 +470,7 @@ void Resource::handleReplyFinished() { return; } - ResourceCache::requestCompleted(this); + ResourceCache::requestCompleted(_self); auto result = _request->getResult(); if (result == ResourceRequest::Success) { diff --git a/libraries/networking/src/ResourceCache.h b/libraries/networking/src/ResourceCache.h index 84eba1cdc0..034ca74882 100644 --- a/libraries/networking/src/ResourceCache.h +++ b/libraries/networking/src/ResourceCache.h @@ -62,21 +62,20 @@ class ResourceCacheSharedItems : public Dependency { using Mutex = std::mutex; using Lock = std::unique_lock; public: - void appendPendingRequest(Resource* newRequest); - void appendActiveRequest(Resource* newRequest); - void removeRequest(Resource* doneRequest); - QList> getPendingRequests() const; + void appendPendingRequest(QWeakPointer newRequest); + void appendActiveRequest(QWeakPointer newRequest); + void removeRequest(QWeakPointer doneRequest); + QList> getPendingRequests() const; uint32_t getPendingRequestsCount() const; - QList getLoadingRequests() const; - Resource* getHighestPendingRequest(); + QList> getLoadingRequests() const; + QSharedPointer getHighestPendingRequest(); private: - ResourceCacheSharedItems() { } - virtual ~ResourceCacheSharedItems() { } + ResourceCacheSharedItems() = default; mutable Mutex _mutex; - QList> _pendingRequests; - QList _loadingRequests; + QList> _pendingRequests; + QList> _loadingRequests; }; @@ -89,6 +88,8 @@ class ResourceCache : public QObject { Q_PROPERTY(size_t sizeCached READ getSizeCachedResources NOTIFY dirty) public: + virtual ~ResourceCache(); + size_t getNumTotalResources() const { return _numTotalResources; } size_t getSizeTotalResources() const { return _totalResourcesSize; } @@ -105,14 +106,10 @@ public: void setUnusedResourceCacheSize(qint64 unusedResourcesMaxSize); qint64 getUnusedResourceCacheSize() const { return _unusedResourcesMaxSize; } - static const QList getLoadingRequests() - { return DependencyManager::get()->getLoadingRequests(); } + static const QList> getLoadingRequests(); - static int getPendingRequestCount() - { return DependencyManager::get()->getPendingRequestsCount(); } + static int getPendingRequestCount(); - ResourceCache(QObject* parent = NULL); - virtual ~ResourceCache(); void refreshAll(); void refresh(const QUrl& url); @@ -127,6 +124,8 @@ protected slots: void updateTotalSize(const qint64& oldSize, const qint64& newSize); protected: + ResourceCache(QObject* parent = nullptr); + /// Loads a resource from the specified URL. /// \param fallback a fallback URL to load if the desired one is unavailable /// \param delayLoad if true, don't load the resource immediately; wait until load is first requested @@ -143,8 +142,8 @@ protected: /// Attempt to load a resource if requests are below the limit, otherwise queue the resource for loading /// \return true if the resource began loading, otherwise false if the resource is in the pending queue - static bool attemptRequest(Resource* resource); - static void requestCompleted(Resource* resource); + static bool attemptRequest(QSharedPointer resource); + static void requestCompleted(QSharedPointer resource); static bool attemptHighestPriorityRequest(); private: diff --git a/tests/animation/src/AnimInverseKinematicsTests.cpp b/tests/animation/src/AnimInverseKinematicsTests.cpp index 2b10892f82..f36b8891ff 100644 --- a/tests/animation/src/AnimInverseKinematicsTests.cpp +++ b/tests/animation/src/AnimInverseKinematicsTests.cpp @@ -11,7 +11,6 @@ #include -#include #include #include #include diff --git a/tests/networking/src/ResourceTests.cpp b/tests/networking/src/ResourceTests.cpp index f18f676398..e6dcf18230 100644 --- a/tests/networking/src/ResourceTests.cpp +++ b/tests/networking/src/ResourceTests.cpp @@ -34,7 +34,7 @@ void ResourceTests::initTestCase() { networkAccessManager.setCache(cache); } -static Resource* resource = nullptr; +static QSharedPointer resource; static bool waitForSignal(QObject *sender, const char *signal, int timeout = 1000) { @@ -55,7 +55,8 @@ void ResourceTests::downloadFirst() { // download the Mery fst file QUrl meryUrl = QUrl("http://hifi-public.s3.amazonaws.com/marketplace/contents/e21c0b95-e502-4d15-8c41-ea2fc40f1125/3585ddf674869a67d31d5964f7b52de1.fst"); - resource = new Resource(meryUrl, false); + resource = QSharedPointer::create(meryUrl, false); + resource->setSelf(resource); const int timeout = 1000; QEventLoop loop; @@ -67,6 +68,8 @@ void ResourceTests::downloadFirst() { loop.connect(resource, SIGNAL(failed(QNetworkReply::NetworkError)), SLOT(quit())); loop.connect(&timer, SIGNAL(timeout()), SLOT(quit())); timer.start(); + + resource->ensureLoading(); loop.exec(); QVERIFY(resource->isLoaded()); @@ -76,7 +79,8 @@ void ResourceTests::downloadAgain() { // download the Mery fst file QUrl meryUrl = QUrl("http://hifi-public.s3.amazonaws.com/marketplace/contents/e21c0b95-e502-4d15-8c41-ea2fc40f1125/3585ddf674869a67d31d5964f7b52de1.fst"); - resource = new Resource(meryUrl, false); + resource = QSharedPointer::create(meryUrl, false); + resource->setSelf(resource); const int timeout = 1000; QEventLoop loop; @@ -88,6 +92,8 @@ void ResourceTests::downloadAgain() { loop.connect(resource, SIGNAL(failed(QNetworkReply::NetworkError)), SLOT(quit())); loop.connect(&timer, SIGNAL(timeout()), SLOT(quit())); timer.start(); + + resource->ensureLoading(); loop.exec(); QVERIFY(resource->isLoaded()); From 5eb4f63573b163742e8a2f57612b0281820fe61b Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Sun, 10 Apr 2016 23:26:06 -0700 Subject: [PATCH 2/9] Use ResourceManager in anim loader --- libraries/animation/src/AnimNodeLoader.cpp | 20 ++++++++++++++------ libraries/animation/src/AnimNodeLoader.h | 8 ++++---- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/libraries/animation/src/AnimNodeLoader.cpp b/libraries/animation/src/AnimNodeLoader.cpp index df39c55e5d..f5d9215253 100644 --- a/libraries/animation/src/AnimNodeLoader.cpp +++ b/libraries/animation/src/AnimNodeLoader.cpp @@ -570,11 +570,19 @@ bool processStateMachineNode(AnimNode::Pointer node, const QJsonObject& jsonObj, AnimNodeLoader::AnimNodeLoader(const QUrl& url) : _url(url) { - _resource = QSharedPointer::create(url); - _resource->setSelf(_resource); - connect(_resource.data(), &Resource::loaded, this, &AnimNodeLoader::onRequestDone); - connect(_resource.data(), &Resource::failed, this, &AnimNodeLoader::onRequestError); - _resource->ensureLoading(); + auto request = ResourceManager::createResourceRequest(this, url); + if (request) { + connect(request, &ResourceRequest::finished, this, [this, request]() { + if (request->getResult() == ResourceRequest::Success) { + onRequestDone(request->getData()); + } else { + onRequestError(request->getResult()); + } + request->deleteLater(); + }); + + request->send(); + } } AnimNode::Pointer AnimNodeLoader::load(const QByteArray& contents, const QUrl& jsonUrl) { @@ -621,6 +629,6 @@ void AnimNodeLoader::onRequestDone(const QByteArray data) { } } -void AnimNodeLoader::onRequestError(QNetworkReply::NetworkError netError) { +void AnimNodeLoader::onRequestError(ResourceRequest::Result netError) { emit error((int)netError, "Resource download error"); } diff --git a/libraries/animation/src/AnimNodeLoader.h b/libraries/animation/src/AnimNodeLoader.h index a1bd4a1f17..edb4729753 100644 --- a/libraries/animation/src/AnimNodeLoader.h +++ b/libraries/animation/src/AnimNodeLoader.h @@ -15,11 +15,11 @@ #include #include -#include #include "AnimNode.h" -class Resource; +#include + class AnimNodeLoader : public QObject { Q_OBJECT @@ -37,11 +37,11 @@ protected: protected slots: void onRequestDone(const QByteArray data); - void onRequestError(QNetworkReply::NetworkError error); + void onRequestError(ResourceRequest::Result error); protected: QUrl _url; - QSharedPointer _resource; + private: // no copies From e45939d18f5e4d8bda5232a44c4e25d090344ae3 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Sun, 10 Apr 2016 23:48:49 -0700 Subject: [PATCH 3/9] Make sure ResourceCacheSharedItems deals with strong refs --- .../GlobalServicesScriptingInterface.cpp | 4 +- interface/src/ui/Stats.cpp | 11 ++--- libraries/networking/src/ResourceCache.cpp | 42 ++++++++++++++----- libraries/networking/src/ResourceCache.h | 12 +++--- 4 files changed, 42 insertions(+), 27 deletions(-) diff --git a/interface/src/scripting/GlobalServicesScriptingInterface.cpp b/interface/src/scripting/GlobalServicesScriptingInterface.cpp index d56a987d2b..e6d87a0260 100644 --- a/interface/src/scripting/GlobalServicesScriptingInterface.cpp +++ b/interface/src/scripting/GlobalServicesScriptingInterface.cpp @@ -122,8 +122,8 @@ void DownloadInfoResultFromScriptValue(const QScriptValue& object, DownloadInfoR DownloadInfoResult GlobalServicesScriptingInterface::getDownloadInfo() { DownloadInfoResult result; - foreach(QSharedPointer resource, ResourceCache::getLoadingRequests()) { - result.downloading.append(resource ? (resource->getProgress() * 100.0f) : 0.0f); + foreach(auto resource, ResourceCache::getLoadingRequests()) { + result.downloading.append(resource->getProgress() * 100.0f); } result.pending = ResourceCache::getPendingRequestCount(); return result; diff --git a/interface/src/ui/Stats.cpp b/interface/src/ui/Stats.cpp index d19f7e2cab..d1cb4cf3c2 100644 --- a/interface/src/ui/Stats.cpp +++ b/interface/src/ui/Stats.cpp @@ -196,7 +196,7 @@ void Stats::updateStats(bool force) { STAT_UPDATE(audioMixerPps, -1); } - QList> loadingRequests = ResourceCache::getLoadingRequests(); + auto loadingRequests = ResourceCache::getLoadingRequests(); STAT_UPDATE(downloads, loadingRequests.size()); STAT_UPDATE(downloadLimit, ResourceCache::getRequestLimit()) STAT_UPDATE(downloadsPending, ResourceCache::getPendingRequestCount()); @@ -205,8 +205,7 @@ void Stats::updateStats(bool force) { bool shouldUpdateUrls = _downloads != _downloadUrls.size(); if (!shouldUpdateUrls) { for (int i = 0; i < _downloads; i++) { - auto request = loadingRequests[i].lock(); - if (!request || request->getURL().toString() != _downloadUrls[i]) { + if (loadingRequests[i]->getURL().toString() != _downloadUrls[i]) { shouldUpdateUrls = true; break; } @@ -215,10 +214,8 @@ void Stats::updateStats(bool force) { // If the urls have changed, update the list if (shouldUpdateUrls) { _downloadUrls.clear(); - foreach (QSharedPointer request, loadingRequests) { - if (request) { - _downloadUrls << request->getURL().toString(); - } + foreach (auto resource, loadingRequests) { + _downloadUrls << resource->getURL().toString(); } emit downloadUrlsChanged(); } diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index 11d0f31217..6c74da749c 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -41,9 +41,9 @@ void ResourceCache::refreshAll() { resetResourceCounters(); // Refresh all remaining resources in use - foreach (auto resource, _resources) { - if (!resource.isNull()) { - resource.data()->refresh(); + foreach (QSharedPointer resource, _resources) { + if (resource) { + resource->refresh(); } } } @@ -110,7 +110,7 @@ QSharedPointer ResourceCache::getResource(const QUrl& url, const QUrl& } if (QThread::currentThread() != thread()) { - assert(delayLoad && !extra); + assert(delayLoad); getResourceAsynchronously(url); return QSharedPointer(); } @@ -208,9 +208,19 @@ void ResourceCacheSharedItems::appendPendingRequest(QWeakPointer resou _pendingRequests.append(resource); } -QList> ResourceCacheSharedItems::getPendingRequests() const { - Lock lock(_mutex); - return _pendingRequests; +QList> ResourceCacheSharedItems::getPendingRequests() { + QList> result; + + { + Lock lock(_mutex); + foreach(QSharedPointer resource, _pendingRequests) { + if (resource) { + result.append(resource); + } + } + _pendingRequests.removeAll(QWeakPointer()); + } + return result; } uint32_t ResourceCacheSharedItems::getPendingRequestsCount() const { @@ -218,9 +228,19 @@ uint32_t ResourceCacheSharedItems::getPendingRequestsCount() const { return _pendingRequests.size(); } -QList> ResourceCacheSharedItems::getLoadingRequests() const { - Lock lock(_mutex); - return _loadingRequests; +QList> ResourceCacheSharedItems::getLoadingRequests() { + QList> result; + + { + Lock lock(_mutex); + foreach(QSharedPointer resource, _loadingRequests) { + if (resource) { + result.append(resource); + } + } + _loadingRequests.removeAll(QWeakPointer()); + } + return result; } void ResourceCacheSharedItems::removeRequest(QWeakPointer resource) { @@ -254,7 +274,7 @@ QSharedPointer ResourceCacheSharedItems::getHighestPendingRequest() { return highestResource; } -const QList> ResourceCache::getLoadingRequests() { +QList> ResourceCache::getLoadingRequests() { return DependencyManager::get()->getLoadingRequests(); } diff --git a/libraries/networking/src/ResourceCache.h b/libraries/networking/src/ResourceCache.h index 034ca74882..94fcafee9e 100644 --- a/libraries/networking/src/ResourceCache.h +++ b/libraries/networking/src/ResourceCache.h @@ -65,9 +65,9 @@ public: void appendPendingRequest(QWeakPointer newRequest); void appendActiveRequest(QWeakPointer newRequest); void removeRequest(QWeakPointer doneRequest); - QList> getPendingRequests() const; + QList> getPendingRequests(); uint32_t getPendingRequestsCount() const; - QList> getLoadingRequests() const; + QList> getLoadingRequests(); QSharedPointer getHighestPendingRequest(); private: @@ -88,8 +88,6 @@ class ResourceCache : public QObject { Q_PROPERTY(size_t sizeCached READ getSizeCachedResources NOTIFY dirty) public: - virtual ~ResourceCache(); - size_t getNumTotalResources() const { return _numTotalResources; } size_t getSizeTotalResources() const { return _totalResourcesSize; } @@ -106,10 +104,12 @@ public: void setUnusedResourceCacheSize(qint64 unusedResourcesMaxSize); qint64 getUnusedResourceCacheSize() const { return _unusedResourcesMaxSize; } - static const QList> getLoadingRequests(); + static QList> getLoadingRequests(); static int getPendingRequestCount(); + ResourceCache(QObject* parent = nullptr); + virtual ~ResourceCache(); void refreshAll(); void refresh(const QUrl& url); @@ -124,8 +124,6 @@ protected slots: void updateTotalSize(const qint64& oldSize, const qint64& newSize); protected: - ResourceCache(QObject* parent = nullptr); - /// Loads a resource from the specified URL. /// \param fallback a fallback URL to load if the desired one is unavailable /// \param delayLoad if true, don't load the resource immediately; wait until load is first requested From 5cc90ce8f0296c83abe41108c9b7e295ac01fe11 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Mon, 11 Apr 2016 11:22:53 -0700 Subject: [PATCH 4/9] Make sure resource req are cleaned up with there parents --- libraries/networking/src/ResourceManager.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libraries/networking/src/ResourceManager.cpp b/libraries/networking/src/ResourceManager.cpp index 8fc8e160b0..f33dbc161d 100644 --- a/libraries/networking/src/ResourceManager.cpp +++ b/libraries/networking/src/ResourceManager.cpp @@ -110,6 +110,9 @@ ResourceRequest* ResourceManager::createResourceRequest(QObject* parent, const Q } Q_ASSERT(request); + if (parent) { + QObject::connect(parent, &QObject::destroyed, request, &QObject::deleteLater); + } request->moveToThread(&_thread); return request; } From 05895f628ac7bcccf8ebc3376e2644a92993b566 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Tue, 12 Apr 2016 13:34:33 -0700 Subject: [PATCH 5/9] Revert to using a Resource --- libraries/animation/src/AnimNodeLoader.cpp | 20 ++++++-------------- libraries/animation/src/AnimNodeLoader.h | 9 +++++---- libraries/networking/src/ResourceCache.cpp | 2 -- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/libraries/animation/src/AnimNodeLoader.cpp b/libraries/animation/src/AnimNodeLoader.cpp index f5d9215253..df39c55e5d 100644 --- a/libraries/animation/src/AnimNodeLoader.cpp +++ b/libraries/animation/src/AnimNodeLoader.cpp @@ -570,19 +570,11 @@ bool processStateMachineNode(AnimNode::Pointer node, const QJsonObject& jsonObj, AnimNodeLoader::AnimNodeLoader(const QUrl& url) : _url(url) { - auto request = ResourceManager::createResourceRequest(this, url); - if (request) { - connect(request, &ResourceRequest::finished, this, [this, request]() { - if (request->getResult() == ResourceRequest::Success) { - onRequestDone(request->getData()); - } else { - onRequestError(request->getResult()); - } - request->deleteLater(); - }); - - request->send(); - } + _resource = QSharedPointer::create(url); + _resource->setSelf(_resource); + connect(_resource.data(), &Resource::loaded, this, &AnimNodeLoader::onRequestDone); + connect(_resource.data(), &Resource::failed, this, &AnimNodeLoader::onRequestError); + _resource->ensureLoading(); } AnimNode::Pointer AnimNodeLoader::load(const QByteArray& contents, const QUrl& jsonUrl) { @@ -629,6 +621,6 @@ void AnimNodeLoader::onRequestDone(const QByteArray data) { } } -void AnimNodeLoader::onRequestError(ResourceRequest::Result netError) { +void AnimNodeLoader::onRequestError(QNetworkReply::NetworkError netError) { emit error((int)netError, "Resource download error"); } diff --git a/libraries/animation/src/AnimNodeLoader.h b/libraries/animation/src/AnimNodeLoader.h index edb4729753..6db001c261 100644 --- a/libraries/animation/src/AnimNodeLoader.h +++ b/libraries/animation/src/AnimNodeLoader.h @@ -13,13 +13,13 @@ #include +#include #include #include #include "AnimNode.h" -#include - +class Resource; class AnimNodeLoader : public QObject { Q_OBJECT @@ -37,11 +37,12 @@ protected: protected slots: void onRequestDone(const QByteArray data); - void onRequestError(ResourceRequest::Result error); + void onRequestError(QNetworkReply::NetworkError error); protected: QUrl _url; - + QSharedPointer _resource; + private: // no copies diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index 6c74da749c..2f90f36670 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -218,7 +218,6 @@ QList> ResourceCacheSharedItems::getPendingRequests() { result.append(resource); } } - _pendingRequests.removeAll(QWeakPointer()); } return result; } @@ -238,7 +237,6 @@ QList> ResourceCacheSharedItems::getLoadingRequests() { result.append(resource); } } - _loadingRequests.removeAll(QWeakPointer()); } return result; } From c8aeecdabdacefa902528d2f8c3f2bb298f1f575 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Tue, 12 Apr 2016 13:41:22 -0700 Subject: [PATCH 6/9] Don't fire onGeoMappLoading twice --- .../src/model-networking/ModelCache.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/libraries/model-networking/src/model-networking/ModelCache.cpp b/libraries/model-networking/src/model-networking/ModelCache.cpp index b8deef9f27..387c59e7f4 100644 --- a/libraries/model-networking/src/model-networking/ModelCache.cpp +++ b/libraries/model-networking/src/model-networking/ModelCache.cpp @@ -46,6 +46,7 @@ private slots: private: GeometryResource::Pointer _geometryResource; + QMetaObject::Connection _connection; }; void GeometryMappingResource::downloadFinished(const QByteArray& data) { @@ -77,21 +78,26 @@ void GeometryMappingResource::downloadFinished(const QByteArray& data) { if (_geometryResource->isLoaded()) { onGeometryMappingLoaded(!_geometryResource->getURL().isEmpty()); } else { - connect(_geometryResource.data(), &Resource::finished, this, &GeometryMappingResource::onGeometryMappingLoaded); + if (_connection) { + disconnect(_connection); + } + + _connection = connect(_geometryResource.data(), &Resource::finished, + this, &GeometryMappingResource::onGeometryMappingLoaded); } } } void GeometryMappingResource::onGeometryMappingLoaded(bool success) { - if (success) { + if (success && _geometryResource) { _geometry = _geometryResource->_geometry; _shapes = _geometryResource->_shapes; _meshes = _geometryResource->_meshes; _materials = _geometryResource->_materials; - } - // Avoid holding onto extra references - _geometryResource.reset(); + // Avoid holding onto extra references + _geometryResource.reset(); + } finishedLoading(success); } From 364f935b893505afd6873816ecf2815ad7bb41dd Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Tue, 12 Apr 2016 13:45:12 -0700 Subject: [PATCH 7/9] Allow cleanup of the old request --- libraries/networking/src/ResourceCache.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index 2f90f36670..5f7cf7926a 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -450,7 +450,10 @@ void Resource::reinsert() { void Resource::makeRequest() { - Q_ASSERT(!_request); + if (_request) { + _request->disconnect(); + _request->deleteLater(); + } _request = ResourceManager::createResourceRequest(this, _activeUrl); From 785eda44cd8df122f9ba37aae2058065b2aeb97c Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Wed, 13 Apr 2016 13:23:11 -0700 Subject: [PATCH 8/9] CR --- libraries/model-networking/src/model-networking/ModelCache.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libraries/model-networking/src/model-networking/ModelCache.cpp b/libraries/model-networking/src/model-networking/ModelCache.cpp index 387c59e7f4..1f21b0b574 100644 --- a/libraries/model-networking/src/model-networking/ModelCache.cpp +++ b/libraries/model-networking/src/model-networking/ModelCache.cpp @@ -97,6 +97,8 @@ void GeometryMappingResource::onGeometryMappingLoaded(bool success) { // Avoid holding onto extra references _geometryResource.reset(); + // Make sure connection will not trigger again + disconnect(_connection); // FIXME Should not have to do this } finishedLoading(success); From 4ed8573f74479fe0da71c18676417cc1266f0e83 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Thu, 14 Apr 2016 17:28:24 -0700 Subject: [PATCH 9/9] CR --- interface/src/scripting/GlobalServicesScriptingInterface.cpp | 2 +- interface/src/ui/Stats.cpp | 2 +- libraries/networking/src/ResourceCache.cpp | 2 +- libraries/networking/src/ResourceCache.h | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/interface/src/scripting/GlobalServicesScriptingInterface.cpp b/interface/src/scripting/GlobalServicesScriptingInterface.cpp index e6d87a0260..e8d63a6d99 100644 --- a/interface/src/scripting/GlobalServicesScriptingInterface.cpp +++ b/interface/src/scripting/GlobalServicesScriptingInterface.cpp @@ -122,7 +122,7 @@ void DownloadInfoResultFromScriptValue(const QScriptValue& object, DownloadInfoR DownloadInfoResult GlobalServicesScriptingInterface::getDownloadInfo() { DownloadInfoResult result; - foreach(auto resource, ResourceCache::getLoadingRequests()) { + foreach(const auto& resource, ResourceCache::getLoadingRequests()) { result.downloading.append(resource->getProgress() * 100.0f); } result.pending = ResourceCache::getPendingRequestCount(); diff --git a/interface/src/ui/Stats.cpp b/interface/src/ui/Stats.cpp index d1cb4cf3c2..ec4b2280b6 100644 --- a/interface/src/ui/Stats.cpp +++ b/interface/src/ui/Stats.cpp @@ -214,7 +214,7 @@ void Stats::updateStats(bool force) { // If the urls have changed, update the list if (shouldUpdateUrls) { _downloadUrls.clear(); - foreach (auto resource, loadingRequests) { + foreach (const auto& resource, loadingRequests) { _downloadUrls << resource->getURL().toString(); } emit downloadUrlsChanged(); diff --git a/libraries/networking/src/ResourceCache.cpp b/libraries/networking/src/ResourceCache.cpp index 5f7cf7926a..aa8022e89b 100644 --- a/libraries/networking/src/ResourceCache.cpp +++ b/libraries/networking/src/ResourceCache.cpp @@ -295,7 +295,7 @@ bool ResourceCache::attemptRequest(QSharedPointer resource) { return true; } -void ResourceCache::requestCompleted(QSharedPointer resource) { +void ResourceCache::requestCompleted(QWeakPointer resource) { auto sharedItems = DependencyManager::get(); sharedItems->removeRequest(resource); --_requestsActive; diff --git a/libraries/networking/src/ResourceCache.h b/libraries/networking/src/ResourceCache.h index 94fcafee9e..ad9ba10a7c 100644 --- a/libraries/networking/src/ResourceCache.h +++ b/libraries/networking/src/ResourceCache.h @@ -141,7 +141,7 @@ protected: /// Attempt to load a resource if requests are below the limit, otherwise queue the resource for loading /// \return true if the resource began loading, otherwise false if the resource is in the pending queue static bool attemptRequest(QSharedPointer resource); - static void requestCompleted(QSharedPointer resource); + static void requestCompleted(QWeakPointer resource); static bool attemptHighestPriorityRequest(); private: