From 8e375fd076b57350a6e535d6896860df68821c3e Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 1 Apr 2016 10:08:44 -0700 Subject: [PATCH] Fix asset resource request not properly cleaning up --- libraries/networking/src/AssetClient.cpp | 87 +++++++++++++------ libraries/networking/src/AssetClient.h | 24 +++-- libraries/networking/src/AssetRequest.cpp | 18 +++- libraries/networking/src/AssetRequest.h | 3 + .../networking/src/AssetResourceRequest.h | 2 +- libraries/networking/src/MappingRequest.cpp | 35 ++++++-- libraries/networking/src/MappingRequest.h | 4 + libraries/networking/src/ResourceRequest.h | 1 + 8 files changed, 130 insertions(+), 44 deletions(-) diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 446009ac23..0d640b4e80 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -200,7 +200,7 @@ AssetUpload* AssetClient::createUpload(const QByteArray& data) { return upload; } -bool AssetClient::getAsset(const QString& hash, DataOffset start, DataOffset end, +MessageID AssetClient::getAsset(const QString& hash, DataOffset start, DataOffset end, ReceivedAssetCallback callback, ProgressCallback progressCallback) { if (hash.length() != SHA256_HASH_HEX_LENGTH) { qCWarning(asset_client) << "Invalid hash size"; @@ -230,17 +230,16 @@ bool AssetClient::getAsset(const QString& hash, DataOffset start, DataOffset end _pendingRequests[assetServer][messageID] = { QSharedPointer(), callback, progressCallback }; - - return true; + return messageID; } else { callback(false, AssetServerError::NoError, QByteArray()); - return false; + return INVALID_MESSAGE_ID; } } -bool AssetClient::getAssetInfo(const QString& hash, GetInfoCallback callback) { +MessageID AssetClient::getAssetInfo(const QString& hash, GetInfoCallback callback) { auto nodeList = DependencyManager::get(); SharedNodePointer assetServer = nodeList->soloNodeOfType(NodeType::AssetServer); @@ -257,10 +256,10 @@ bool AssetClient::getAssetInfo(const QString& hash, GetInfoCallback callback) { _pendingInfoRequests[assetServer][messageID] = callback; - return true; + return messageID; } else { callback(false, AssetServerError::NoError, { "", 0 }); - return false; + return INVALID_MESSAGE_ID; } } @@ -352,7 +351,7 @@ void AssetClient::handleAssetGetReply(QSharedPointer message, S } } -bool AssetClient::getAssetMapping(const AssetPath& path, MappingOperationCallback callback) { +MessageID AssetClient::getAssetMapping(const AssetPath& path, MappingOperationCallback callback) { auto nodeList = DependencyManager::get(); SharedNodePointer assetServer = nodeList->soloNodeOfType(NodeType::AssetServer); @@ -370,14 +369,14 @@ bool AssetClient::getAssetMapping(const AssetPath& path, MappingOperationCallbac _pendingMappingRequests[assetServer][messageID] = callback; - return true; + return messageID; } else { callback(false, AssetServerError::NoError, QSharedPointer()); - return false; + return INVALID_MESSAGE_ID; } } -bool AssetClient::getAllAssetMappings(MappingOperationCallback callback) { +MessageID AssetClient::getAllAssetMappings(MappingOperationCallback callback) { auto nodeList = DependencyManager::get(); SharedNodePointer assetServer = nodeList->soloNodeOfType(NodeType::AssetServer); @@ -393,14 +392,14 @@ bool AssetClient::getAllAssetMappings(MappingOperationCallback callback) { _pendingMappingRequests[assetServer][messageID] = callback; - return true; + return messageID; } else { callback(false, AssetServerError::NoError, QSharedPointer()); - return false; + return INVALID_MESSAGE_ID; } } -bool AssetClient::deleteAssetMappings(const AssetPathList& paths, MappingOperationCallback callback) { +MessageID AssetClient::deleteAssetMappings(const AssetPathList& paths, MappingOperationCallback callback) { auto nodeList = DependencyManager::get(); SharedNodePointer assetServer = nodeList->soloNodeOfType(NodeType::AssetServer); @@ -422,14 +421,14 @@ bool AssetClient::deleteAssetMappings(const AssetPathList& paths, MappingOperati _pendingMappingRequests[assetServer][messageID] = callback; - return true; + return messageID; } else { callback(false, AssetServerError::NoError, QSharedPointer()); - return false; + return INVALID_MESSAGE_ID; } } -bool AssetClient::setAssetMapping(const QString& path, const AssetHash& hash, MappingOperationCallback callback) { +MessageID AssetClient::setAssetMapping(const QString& path, const AssetHash& hash, MappingOperationCallback callback) { auto nodeList = DependencyManager::get(); SharedNodePointer assetServer = nodeList->soloNodeOfType(NodeType::AssetServer); @@ -448,14 +447,14 @@ bool AssetClient::setAssetMapping(const QString& path, const AssetHash& hash, Ma _pendingMappingRequests[assetServer][messageID] = callback; - return true; + return messageID; } else { callback(false, AssetServerError::NoError, QSharedPointer()); - return false; + return INVALID_MESSAGE_ID; } } -bool AssetClient::renameAssetMapping(const AssetPath& oldPath, const AssetPath& newPath, MappingOperationCallback callback) { +MessageID AssetClient::renameAssetMapping(const AssetPath& oldPath, const AssetPath& newPath, MappingOperationCallback callback) { auto nodeList = DependencyManager::get(); SharedNodePointer assetServer = nodeList->soloNodeOfType(NodeType::AssetServer); @@ -474,15 +473,53 @@ bool AssetClient::renameAssetMapping(const AssetPath& oldPath, const AssetPath& _pendingMappingRequests[assetServer][messageID] = callback; - return true; + return messageID; } else { callback(false, AssetServerError::NoError, QSharedPointer()); - return false; + return INVALID_MESSAGE_ID; } } -bool AssetClient::uploadAsset(const QByteArray& data, UploadResultCallback callback) { +bool AssetClient::cancelMappingRequest(MessageID id) { + for (auto& kv : _pendingMappingRequests) { + if (kv.second.erase(id)) { + return true; + } + } + return false; +} + +bool AssetClient::cancelGetAssetInfoRequest(MessageID id) { + for (auto& kv : _pendingInfoRequests) { + if (kv.second.erase(id)) { + return true; + } + } + return false; +} + +bool AssetClient::cancelGetAssetRequest(MessageID id) { + // Search through each pending mapping request for id `id` + for (auto& kv : _pendingRequests) { + if (kv.second.erase(id)) { + return true; + } + } + return false; +} + +bool AssetClient::cancelUploadAssetRequest(MessageID id) { + // Search through each pending mapping request for id `id` + for (auto& kv : _pendingUploads) { + if (kv.second.erase(id)) { + return true; + } + } + return false; +} + +MessageID AssetClient::uploadAsset(const QByteArray& data, UploadResultCallback callback) { auto nodeList = DependencyManager::get(); SharedNodePointer assetServer = nodeList->soloNodeOfType(NodeType::AssetServer); @@ -500,10 +537,10 @@ bool AssetClient::uploadAsset(const QByteArray& data, UploadResultCallback callb _pendingUploads[assetServer][messageID] = callback; - return true; + return messageID; } else { callback(false, AssetServerError::NoError, QString()); - return false; + return INVALID_MESSAGE_ID; } } diff --git a/libraries/networking/src/AssetClient.h b/libraries/networking/src/AssetClient.h index f1890377dc..c0e95c0d1d 100644 --- a/libraries/networking/src/AssetClient.h +++ b/libraries/networking/src/AssetClient.h @@ -60,6 +60,8 @@ public: Q_INVOKABLE AssetUpload* createUpload(const QString& filename); Q_INVOKABLE AssetUpload* createUpload(const QByteArray& data); + static const MessageID INVALID_MESSAGE_ID = 0; + public slots: void init(); @@ -75,16 +77,21 @@ private slots: void handleNodeKilled(SharedNodePointer node); private: - bool getAssetMapping(const AssetHash& hash, MappingOperationCallback callback); - bool getAllAssetMappings(MappingOperationCallback callback); - bool setAssetMapping(const QString& path, const AssetHash& hash, MappingOperationCallback callback); - bool deleteAssetMappings(const AssetPathList& paths, MappingOperationCallback callback); - bool renameAssetMapping(const AssetPath& oldPath, const AssetPath& newPath, MappingOperationCallback callback); + MessageID getAssetMapping(const AssetHash& hash, MappingOperationCallback callback); + MessageID getAllAssetMappings(MappingOperationCallback callback); + MessageID setAssetMapping(const QString& path, const AssetHash& hash, MappingOperationCallback callback); + MessageID deleteAssetMappings(const AssetPathList& paths, MappingOperationCallback callback); + MessageID renameAssetMapping(const AssetPath& oldPath, const AssetPath& newPath, MappingOperationCallback callback); - bool getAssetInfo(const QString& hash, GetInfoCallback callback); - bool getAsset(const QString& hash, DataOffset start, DataOffset end, + MessageID getAssetInfo(const QString& hash, GetInfoCallback callback); + MessageID getAsset(const QString& hash, DataOffset start, DataOffset end, ReceivedAssetCallback callback, ProgressCallback progressCallback); - bool uploadAsset(const QByteArray& data, UploadResultCallback callback); + MessageID uploadAsset(const QByteArray& data, UploadResultCallback callback); + + bool cancelMappingRequest(MessageID id); + bool cancelGetAssetInfoRequest(MessageID id); + bool cancelGetAssetRequest(MessageID id); + bool cancelUploadAssetRequest(MessageID id); struct GetAssetRequestData { QSharedPointer message; @@ -100,6 +107,7 @@ private: friend class AssetRequest; friend class AssetUpload; + friend class MappingRequest; friend class GetMappingRequest; friend class GetAllMappingsRequest; friend class SetMappingRequest; diff --git a/libraries/networking/src/AssetRequest.cpp b/libraries/networking/src/AssetRequest.cpp index e40b8d2cbe..17413cf4d3 100644 --- a/libraries/networking/src/AssetRequest.cpp +++ b/libraries/networking/src/AssetRequest.cpp @@ -25,6 +25,16 @@ AssetRequest::AssetRequest(const QString& hash) : { } +AssetRequest::~AssetRequest() { + auto assetClient = DependencyManager::get(); + if (_assetRequestID) { + assetClient->cancelGetAssetRequest(_assetRequestID); + } + if (_assetInfoRequestID) { + assetClient->cancelGetAssetInfoRequest(_assetInfoRequestID); + } +} + void AssetRequest::start() { if (QThread::currentThread() != thread()) { QMetaObject::invokeMethod(this, "start", Qt::AutoConnection); @@ -60,7 +70,9 @@ void AssetRequest::start() { _state = WaitingForInfo; auto assetClient = DependencyManager::get(); - assetClient->getAssetInfo(_hash, [this](bool responseReceived, AssetServerError serverError, AssetInfo info) { + _assetInfoRequestID = assetClient->getAssetInfo(_hash, [this](bool responseReceived, AssetServerError serverError, AssetInfo info) { + _assetInfoRequestID = AssetClient::INVALID_MESSAGE_ID; + _info = info; if (!responseReceived) { @@ -92,8 +104,10 @@ void AssetRequest::start() { int start = 0, end = _info.size; auto assetClient = DependencyManager::get(); - assetClient->getAsset(_hash, start, end, [this, start, end](bool responseReceived, AssetServerError serverError, + _assetRequestID = assetClient->getAsset(_hash, start, end, [this, start, end](bool responseReceived, AssetServerError serverError, const QByteArray& data) { + _assetRequestID = AssetClient::INVALID_MESSAGE_ID; + if (!responseReceived) { _error = NetworkError; } else if (serverError != AssetServerError::NoError) { diff --git a/libraries/networking/src/AssetRequest.h b/libraries/networking/src/AssetRequest.h index b98ac4ec4b..c0bde9d8a8 100644 --- a/libraries/networking/src/AssetRequest.h +++ b/libraries/networking/src/AssetRequest.h @@ -41,6 +41,7 @@ public: }; AssetRequest(const QString& hash); + virtual ~AssetRequest() override; Q_INVOKABLE void start(); @@ -61,6 +62,8 @@ private: QString _hash; QByteArray _data; int _numPendingRequests { 0 }; + MessageID _assetRequestID { AssetClient::INVALID_MESSAGE_ID }; + MessageID _assetInfoRequestID { AssetClient::INVALID_MESSAGE_ID }; }; #endif diff --git a/libraries/networking/src/AssetResourceRequest.h b/libraries/networking/src/AssetResourceRequest.h index f517c57030..6839db0628 100644 --- a/libraries/networking/src/AssetResourceRequest.h +++ b/libraries/networking/src/AssetResourceRequest.h @@ -21,7 +21,7 @@ class AssetResourceRequest : public ResourceRequest { Q_OBJECT public: AssetResourceRequest(const QUrl& url) : ResourceRequest(url) { } - ~AssetResourceRequest(); + virtual ~AssetResourceRequest() override; protected: virtual void doSend() override; diff --git a/libraries/networking/src/MappingRequest.cpp b/libraries/networking/src/MappingRequest.cpp index 409957db42..d3950f9f30 100644 --- a/libraries/networking/src/MappingRequest.cpp +++ b/libraries/networking/src/MappingRequest.cpp @@ -15,7 +15,12 @@ #include -#include "AssetClient.h" +MappingRequest::~MappingRequest() { + auto assetClient = DependencyManager::get(); + if (_mappingRequestID) { + assetClient->cancelMappingRequest(_mappingRequestID); + } +} void MappingRequest::start() { if (QThread::currentThread() != thread()) { @@ -60,7 +65,10 @@ void GetMappingRequest::doStart() { auto assetClient = DependencyManager::get(); - assetClient->getAssetMapping(_path, [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + _mappingRequestID = assetClient->getAssetMapping(_path, + [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + + _mappingRequestID = AssetClient::INVALID_MESSAGE_ID; if (!responseReceived) { _error = NetworkError; } else { @@ -89,7 +97,11 @@ GetAllMappingsRequest::GetAllMappingsRequest() { void GetAllMappingsRequest::doStart() { auto assetClient = DependencyManager::get(); - assetClient->getAllAssetMappings([this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + _mappingRequestID = assetClient->getAllAssetMappings( + [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + + _mappingRequestID = AssetClient::INVALID_MESSAGE_ID; + if (!responseReceived) { _error = NetworkError; } else { @@ -137,7 +149,10 @@ void SetMappingRequest::doStart() { auto assetClient = DependencyManager::get(); - assetClient->setAssetMapping(_path, _hash, [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + _mappingRequestID = assetClient->setAssetMapping(_path, _hash, + [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + + _mappingRequestID = AssetClient::INVALID_MESSAGE_ID; if (!responseReceived) { _error = NetworkError; } else { @@ -177,7 +192,10 @@ void DeleteMappingsRequest::doStart() { auto assetClient = DependencyManager::get(); - assetClient->deleteAssetMappings(_paths, [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + _mappingRequestID = assetClient->deleteAssetMappings(_paths, + [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + + _mappingRequestID = AssetClient::INVALID_MESSAGE_ID; if (!responseReceived) { _error = NetworkError; } else { @@ -216,9 +234,10 @@ void RenameMappingRequest::doStart() { auto assetClient = DependencyManager::get(); - assetClient->renameAssetMapping(_oldPath, _newPath, [this, assetClient](bool responseReceived, - AssetServerError error, - QSharedPointer message) { + _mappingRequestID = assetClient->renameAssetMapping(_oldPath, _newPath, + [this, assetClient](bool responseReceived, AssetServerError error, QSharedPointer message) { + + _mappingRequestID = AssetClient::INVALID_MESSAGE_ID; if (!responseReceived) { _error = NetworkError; } else { diff --git a/libraries/networking/src/MappingRequest.h b/libraries/networking/src/MappingRequest.h index 206e383c33..274d9dbd19 100644 --- a/libraries/networking/src/MappingRequest.h +++ b/libraries/networking/src/MappingRequest.h @@ -17,6 +17,7 @@ #include #include "AssetUtils.h" +#include "AssetClient.h" class MappingRequest : public QObject { Q_OBJECT @@ -31,12 +32,15 @@ public: UnknownError }; + virtual ~MappingRequest(); + Q_INVOKABLE void start(); Error getError() const { return _error; } Q_INVOKABLE QString getErrorString() const; protected: Error _error { NoError }; + MessageID _mappingRequestID { AssetClient::INVALID_MESSAGE_ID }; private: virtual void doStart() = 0; diff --git a/libraries/networking/src/ResourceRequest.h b/libraries/networking/src/ResourceRequest.h index f940221d9d..83dc8fb7d7 100644 --- a/libraries/networking/src/ResourceRequest.h +++ b/libraries/networking/src/ResourceRequest.h @@ -21,6 +21,7 @@ class ResourceRequest : public QObject { Q_OBJECT public: ResourceRequest(const QUrl& url); + virtual ~ResourceRequest() = default; enum State { NotStarted = 0,