From afdfef148200c5256266c9c01b6e3f00ab0a050e Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Sun, 10 Apr 2016 21:54:49 -0700 Subject: [PATCH] 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());