From 4ed3e36181544ee6d18a9249acbaf90ed2564043 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Sep 2015 09:15:33 -0700 Subject: [PATCH 01/11] Update AssetClient::create* methods to not block on invokeMethod --- libraries/networking/src/AssetClient.cpp | 32 ++++++++---------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 2de5173b76..2a45b6b154 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -38,16 +38,6 @@ AssetClient::AssetClient() { } AssetRequest* AssetClient::createRequest(const QString& hash, const QString& extension) { - if (QThread::currentThread() != thread()) { - AssetRequest* req; - QMetaObject::invokeMethod(this, "createRequest", - Qt::BlockingQueuedConnection, - Q_RETURN_ARG(AssetRequest*, req), - Q_ARG(QString, hash), - Q_ARG(QString, extension)); - return req; - } - if (hash.length() != SHA256_HASH_HEX_LENGTH) { qDebug() << "Invalid hash size"; return nullptr; @@ -62,19 +52,15 @@ AssetRequest* AssetClient::createRequest(const QString& hash, const QString& ext return nullptr; } - return new AssetRequest(this, hash, extension); + auto request = new AssetRequest(hash, extension); + + // Move to the AssetClient thread in case we are not currently on that thread (which will usually be the case) + request->moveToThread(thread()); + + return request; } AssetUpload* AssetClient::createUpload(const QString& filename) { - if (QThread::currentThread() != thread()) { - AssetUpload* upload; - QMetaObject::invokeMethod(this, "createUpload", - Qt::BlockingQueuedConnection, - Q_RETURN_ARG(AssetUpload*, upload), - Q_ARG(QString, filename)); - return upload; - } - auto nodeList = DependencyManager::get(); SharedNodePointer assetServer = nodeList->soloNodeOfType(NodeType::AssetServer); @@ -83,7 +69,11 @@ AssetUpload* AssetClient::createUpload(const QString& filename) { return nullptr; } - return new AssetUpload(this, filename); + auto upload = new AssetUpload(this, filename); + + upload->moveToThread(thread()); + + return upload; } bool AssetClient::getAsset(const QString& hash, const QString& extension, DataOffset start, DataOffset end, From 3ae323cb2eb088673d6ce3267b65ed1038ebb079 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Sep 2015 09:17:08 -0700 Subject: [PATCH 02/11] Move pending requests in AssetClient to include node sent to --- libraries/networking/src/AssetClient.cpp | 66 +++++++++++++++++++----- libraries/networking/src/AssetClient.h | 6 +-- libraries/networking/src/Node.h | 8 +++ 3 files changed, 65 insertions(+), 15 deletions(-) diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 2a45b6b154..386460a756 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -108,7 +108,7 @@ bool AssetClient::getAsset(const QString& hash, const QString& extension, DataOf nodeList->sendPacket(std::move(packet), *assetServer); - _pendingRequests[messageID] = callback; + _pendingRequests[assetServer][messageID] = callback; return true; } @@ -133,7 +133,7 @@ bool AssetClient::getAssetInfo(const QString& hash, const QString& extension, Ge nodeList->sendPacket(std::move(packet), *assetServer); - _pendingInfoRequests[messageID] = callback; + _pendingInfoRequests[assetServer][messageID] = callback; return true; } @@ -155,9 +155,23 @@ void AssetClient::handleAssetGetInfoReply(QSharedPointer packet, Share packet->readPrimitive(&info.size); } - if (_pendingInfoRequests.contains(messageID)) { - auto callback = _pendingInfoRequests.take(messageID); - callback(error, info); + // Check if we have any pending requests for this node + auto messageMapIt = _pendingInfoRequests.find(senderNode); + if (messageMapIt != _pendingInfoRequests.end()) { + + // Found the node, get the MessageID -> Callback map + auto& messageCallbackMap = messageMapIt->second; + + // Check if we have this pending request + auto requestIt = messageCallbackMap.find(messageID); + if (requestIt != messageCallbackMap.end()) { + auto callback = requestIt->second; + callback(error, info); + messageCallbackMap.erase(requestIt); + } + + // Although the messageCallbackMap may now be empty, we won't delete the node until we have disconnected from + // it to avoid constantly creating/deleting the map on subsequent requests. } } @@ -184,9 +198,23 @@ void AssetClient::handleAssetGetReply(QSharedPointer packetList, S qDebug() << "Failure getting asset: " << error; } - if (_pendingRequests.contains(messageID)) { - auto callback = _pendingRequests.take(messageID); - callback(error, data); + // Check if we have any pending requests for this node + auto messageMapIt = _pendingRequests.find(senderNode); + if (messageMapIt != _pendingRequests.end()) { + + // Found the node, get the MessageID -> Callback map + auto& messageCallbackMap = messageMapIt->second; + + // Check if we have this pending request + auto requestIt = messageCallbackMap.find(messageID); + if (requestIt != messageCallbackMap.end()) { + auto callback = requestIt->second; + callback(error, data); + messageCallbackMap.erase(requestIt); + } + + // Although the messageCallbackMap may now be empty, we won't delete the node until we have disconnected from + // it to avoid constantly creating/deleting the map on subsequent requests. } } @@ -209,7 +237,7 @@ bool AssetClient::uploadAsset(const QByteArray& data, const QString& extension, nodeList->sendPacketList(std::move(packetList), *assetServer); - _pendingUploads[messageID] = callback; + _pendingUploads[assetServer][messageID] = callback; return true; } @@ -234,8 +262,22 @@ void AssetClient::handleAssetUploadReply(QSharedPointer packet, Shared qDebug() << "Successfully uploaded asset to asset-server - SHA256 hash is " << hashString; } - if (_pendingUploads.contains(messageID)) { - auto callback = _pendingUploads.take(messageID); - callback(error, hashString); + // Check if we have any pending requests for this node + auto messageMapIt = _pendingUploads.find(senderNode); + if (messageMapIt != _pendingUploads.end()) { + + // Found the node, get the MessageID -> Callback map + auto& messageCallbackMap = messageMapIt->second; + + // Check if we have this pending request + auto requestIt = messageCallbackMap.find(messageID); + if (requestIt != messageCallbackMap.end()) { + auto callback = requestIt->second; + callback(error, hashString); + messageCallbackMap.erase(requestIt); + } + + // Although the messageCallbackMap may now be empty, we won't delete the node until we have disconnected from + // it to avoid constantly creating/deleting the map on subsequent requests. } } diff --git a/libraries/networking/src/AssetClient.h b/libraries/networking/src/AssetClient.h index d2f0bcddf6..34f1937dd8 100644 --- a/libraries/networking/src/AssetClient.h +++ b/libraries/networking/src/AssetClient.h @@ -52,9 +52,9 @@ private: bool uploadAsset(const QByteArray& data, const QString& extension, UploadResultCallback callback); static MessageID _currentID; - QHash _pendingRequests; - QHash _pendingInfoRequests; - QHash _pendingUploads; + std::unordered_map> _pendingRequests; + std::unordered_map> _pendingInfoRequests; + std::unordered_map> _pendingUploads; friend class AssetRequest; friend class AssetUpload; diff --git a/libraries/networking/src/Node.h b/libraries/networking/src/Node.h index 573569f92b..5875295716 100644 --- a/libraries/networking/src/Node.h +++ b/libraries/networking/src/Node.h @@ -88,6 +88,14 @@ private: typedef QSharedPointer SharedNodePointer; Q_DECLARE_METATYPE(SharedNodePointer) +template<> +struct std::hash { + std::size_t operator()(const SharedNodePointer& p) const { + // Return the hash of the pointer + return std::hash()(p.data()); + } +}; + QDebug operator<<(QDebug debug, const Node& node); #endif // hifi_Node_h From 42b4c7d4234b674ec1b1893839071f9cbcb1b7fc Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Sep 2015 12:55:01 -0700 Subject: [PATCH 03/11] Update AssetClient to handle disconnections from a node --- libraries/networking/src/AssetClient.cpp | 38 +++++++++++++++++++++++- libraries/networking/src/AssetClient.h | 3 ++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 386460a756..6a98fcbff7 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -31,10 +31,13 @@ AssetClient::AssetClient() { static_cast(dependency)->deleteLater(); }); - auto& packetReceiver = DependencyManager::get()->getPacketReceiver(); + auto nodeList = DependencyManager::get(); + auto& packetReceiver = nodeList->getPacketReceiver(); packetReceiver.registerListener(PacketType::AssetGetInfoReply, this, "handleAssetGetInfoReply"); packetReceiver.registerMessageListener(PacketType::AssetGetReply, this, "handleAssetGetReply"); packetReceiver.registerListener(PacketType::AssetUploadReply, this, "handleAssetUploadReply"); + + connect(nodeList.data(), &LimitedNodeList::nodeKilled, this, &AssetClient::handleNodeKilled); } AssetRequest* AssetClient::createRequest(const QString& hash, const QString& extension) { @@ -281,3 +284,36 @@ void AssetClient::handleAssetUploadReply(QSharedPointer packet, Shared // it to avoid constantly creating/deleting the map on subsequent requests. } } + +void AssetClient::handleNodeKilled(SharedNodePointer node) { + { + auto messageMapIt = _pendingRequests.find(node); + if (messageMapIt != _pendingRequests.end()) { + for (const auto& value : messageMapIt->second) { + value.second(AssetServerError::NetworkError, QByteArray()); + } + messageMapIt->second.clear(); + } + } + + { + auto messageMapIt = _pendingInfoRequests.find(node); + if (messageMapIt != _pendingInfoRequests.end()) { + AssetInfo info { "", 0 }; + for (const auto& value : messageMapIt->second) { + value.second(AssetServerError::NetworkError, info); + } + messageMapIt->second.clear(); + } + } + + { + auto messageMapIt = _pendingUploads.find(node); + if (messageMapIt != _pendingUploads.end()) { + for (const auto& value : messageMapIt->second) { + value.second(AssetServerError::NetworkError, ""); + } + messageMapIt->second.clear(); + } + } +} diff --git a/libraries/networking/src/AssetClient.h b/libraries/networking/src/AssetClient.h index 34f1937dd8..0e8e4d63eb 100644 --- a/libraries/networking/src/AssetClient.h +++ b/libraries/networking/src/AssetClient.h @@ -20,6 +20,7 @@ #include "AssetUtils.h" #include "LimitedNodeList.h" #include "NLPacket.h" +#include "Node.h" class AssetRequest; class AssetUpload; @@ -46,6 +47,8 @@ private slots: void handleAssetGetReply(QSharedPointer packetList, SharedNodePointer senderNode); void handleAssetUploadReply(QSharedPointer packet, SharedNodePointer senderNode); + void handleNodeKilled(SharedNodePointer node); + private: bool getAssetInfo(const QString& hash, const QString& extension, GetInfoCallback callback); bool getAsset(const QString& hash, const QString& extension, DataOffset start, DataOffset end, ReceivedAssetCallback callback); From b564ceb76faf01124f7be85b248431b6e57e0d70 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Sep 2015 13:55:48 -0700 Subject: [PATCH 04/11] Add network error to AssetRequest --- libraries/networking/src/AssetRequest.cpp | 7 +++++-- libraries/networking/src/AssetRequest.h | 3 ++- libraries/networking/src/AssetResourceRequest.cpp | 4 ++++ libraries/networking/src/AssetUtils.h | 5 +++-- 4 files changed, 14 insertions(+), 5 deletions(-) diff --git a/libraries/networking/src/AssetRequest.cpp b/libraries/networking/src/AssetRequest.cpp index b2442cb33d..ac9e1fba73 100644 --- a/libraries/networking/src/AssetRequest.cpp +++ b/libraries/networking/src/AssetRequest.cpp @@ -19,8 +19,8 @@ #include "NetworkLogging.h" #include "NodeList.h" -AssetRequest::AssetRequest(QObject* parent, const QString& hash, const QString& extension) : - QObject(parent), +AssetRequest::AssetRequest(const QString& hash, const QString& extension) : + QObject(), _hash(hash), _extension(extension) { @@ -87,6 +87,9 @@ void AssetRequest::start() { case AssetServerError::InvalidByteRange: _error = InvalidByteRange; break; + case AssetServerError::NetworkError: + _error = Error::NetworkError; + break; default: _error = UnknownError; break; diff --git a/libraries/networking/src/AssetRequest.h b/libraries/networking/src/AssetRequest.h index ba3863ffdd..3748703563 100644 --- a/libraries/networking/src/AssetRequest.h +++ b/libraries/networking/src/AssetRequest.h @@ -35,10 +35,11 @@ public: NotFound, InvalidByteRange, HashVerificationFailed, + NetworkError, UnknownError }; - AssetRequest(QObject* parent, const QString& hash, const QString& extension); + AssetRequest(const QString& hash, const QString& extension); Q_INVOKABLE void start(); diff --git a/libraries/networking/src/AssetResourceRequest.cpp b/libraries/networking/src/AssetResourceRequest.cpp index df5cb6464b..6f92d1d0dd 100644 --- a/libraries/networking/src/AssetResourceRequest.cpp +++ b/libraries/networking/src/AssetResourceRequest.cpp @@ -55,6 +55,10 @@ void AssetResourceRequest::doSend() { case AssetRequest::Error::NotFound: _result = NotFound; break; + case AssetRequest::Error::NetworkError: + qDebug() << "Got a network error for asset resource request: " << _url.toString(); + _result = ServerUnavailable; + break; default: _result = Error; break; diff --git a/libraries/networking/src/AssetUtils.h b/libraries/networking/src/AssetUtils.h index 7281678a10..c944657285 100644 --- a/libraries/networking/src/AssetUtils.h +++ b/libraries/networking/src/AssetUtils.h @@ -24,11 +24,12 @@ const size_t SHA256_HASH_HEX_LENGTH = 64; const uint64_t MAX_UPLOAD_SIZE = 1000 * 1000 * 1000; // 1GB enum AssetServerError : uint8_t { - NoError, + NoError = 0, AssetNotFound, InvalidByteRange, AssetTooLarge, - PermissionDenied + PermissionDenied, + NetworkError = 128 }; const QString ATP_SCHEME = "atp"; From a5ba86514c9851bacd229b3a61652175a4669d89 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Fri, 11 Sep 2015 13:57:19 -0700 Subject: [PATCH 05/11] Remove log message --- libraries/networking/src/AssetResourceRequest.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/libraries/networking/src/AssetResourceRequest.cpp b/libraries/networking/src/AssetResourceRequest.cpp index 6f92d1d0dd..ec6c4bec0d 100644 --- a/libraries/networking/src/AssetResourceRequest.cpp +++ b/libraries/networking/src/AssetResourceRequest.cpp @@ -56,7 +56,6 @@ void AssetResourceRequest::doSend() { _result = NotFound; break; case AssetRequest::Error::NetworkError: - qDebug() << "Got a network error for asset resource request: " << _url.toString(); _result = ServerUnavailable; break; default: From 71456eee5e790d154bf5b8c08b328aaad6e008ed Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Mon, 14 Sep 2015 11:19:47 -0700 Subject: [PATCH 06/11] Add UploadDialog error for NetworkError --- interface/src/ui/AssetUploadDialogFactory.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/interface/src/ui/AssetUploadDialogFactory.cpp b/interface/src/ui/AssetUploadDialogFactory.cpp index 2ffbb298d0..175fc9a12d 100644 --- a/interface/src/ui/AssetUploadDialogFactory.cpp +++ b/interface/src/ui/AssetUploadDialogFactory.cpp @@ -134,6 +134,9 @@ void AssetUploadDialogFactory::handleUploadFinished(AssetUpload* upload, const Q case AssetUpload::FileOpenError: additionalError = "The file could not be opened. Please check your permissions and try again."; break; + case AssetUpload::NetworkError: + additionalError = "The file could not be opened. Please check your network connectivity."; + break; default: // not handled, do not show a message box return; From 6468046c6882a7aad25441b9369034f1fdcf9b80 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Mon, 14 Sep 2015 11:20:32 -0700 Subject: [PATCH 07/11] Update AssetClient callbacks to return whether a response was received --- libraries/networking/src/AssetClient.cpp | 12 ++--- libraries/networking/src/AssetClient.h | 6 +-- libraries/networking/src/AssetRequest.cpp | 58 +++++++++++++---------- libraries/networking/src/AssetUpload.cpp | 32 +++++++------ libraries/networking/src/AssetUtils.h | 3 +- 5 files changed, 62 insertions(+), 49 deletions(-) diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 6a98fcbff7..298f68497e 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -169,7 +169,7 @@ void AssetClient::handleAssetGetInfoReply(QSharedPointer packet, Share auto requestIt = messageCallbackMap.find(messageID); if (requestIt != messageCallbackMap.end()) { auto callback = requestIt->second; - callback(error, info); + callback(true, error, info); messageCallbackMap.erase(requestIt); } @@ -212,7 +212,7 @@ void AssetClient::handleAssetGetReply(QSharedPointer packetList, S auto requestIt = messageCallbackMap.find(messageID); if (requestIt != messageCallbackMap.end()) { auto callback = requestIt->second; - callback(error, data); + callback(true, error, data); messageCallbackMap.erase(requestIt); } @@ -276,7 +276,7 @@ void AssetClient::handleAssetUploadReply(QSharedPointer packet, Shared auto requestIt = messageCallbackMap.find(messageID); if (requestIt != messageCallbackMap.end()) { auto callback = requestIt->second; - callback(error, hashString); + callback(true, error, hashString); messageCallbackMap.erase(requestIt); } @@ -290,7 +290,7 @@ void AssetClient::handleNodeKilled(SharedNodePointer node) { auto messageMapIt = _pendingRequests.find(node); if (messageMapIt != _pendingRequests.end()) { for (const auto& value : messageMapIt->second) { - value.second(AssetServerError::NetworkError, QByteArray()); + value.second(false, AssetServerError::NoError, QByteArray()); } messageMapIt->second.clear(); } @@ -301,7 +301,7 @@ void AssetClient::handleNodeKilled(SharedNodePointer node) { if (messageMapIt != _pendingInfoRequests.end()) { AssetInfo info { "", 0 }; for (const auto& value : messageMapIt->second) { - value.second(AssetServerError::NetworkError, info); + value.second(false, AssetServerError::NoError, info); } messageMapIt->second.clear(); } @@ -311,7 +311,7 @@ void AssetClient::handleNodeKilled(SharedNodePointer node) { auto messageMapIt = _pendingUploads.find(node); if (messageMapIt != _pendingUploads.end()) { for (const auto& value : messageMapIt->second) { - value.second(AssetServerError::NetworkError, ""); + value.second(false, AssetServerError::NoError, ""); } messageMapIt->second.clear(); } diff --git a/libraries/networking/src/AssetClient.h b/libraries/networking/src/AssetClient.h index 0e8e4d63eb..1f5a61dc55 100644 --- a/libraries/networking/src/AssetClient.h +++ b/libraries/networking/src/AssetClient.h @@ -30,9 +30,9 @@ struct AssetInfo { int64_t size; }; -using ReceivedAssetCallback = std::function; -using GetInfoCallback = std::function; -using UploadResultCallback = std::function; +using ReceivedAssetCallback = std::function; +using GetInfoCallback = std::function; +using UploadResultCallback = std::function; class AssetClient : public QObject, public Dependency { Q_OBJECT diff --git a/libraries/networking/src/AssetRequest.cpp b/libraries/networking/src/AssetRequest.cpp index ac9e1fba73..f9d5c70bf5 100644 --- a/libraries/networking/src/AssetRequest.cpp +++ b/libraries/networking/src/AssetRequest.cpp @@ -41,17 +41,28 @@ void AssetRequest::start() { _state = WAITING_FOR_INFO; auto assetClient = DependencyManager::get(); - assetClient->getAssetInfo(_hash, _extension, [this](AssetServerError serverError, AssetInfo info) { + assetClient->getAssetInfo(_hash, _extension, [this](bool responseReceived, AssetServerError serverError, AssetInfo info) { _info = info; - if (serverError != AssetServerError::NoError) { + if (!responseReceived) { + _error = NetworkError; + } else if (serverError != AssetServerError::NoError) { + switch(serverError) { + case AssetServerError::AssetNotFound: + _error = NotFound; + break; + default: + _error = UnknownError; + break; + } + } + + if (_error != NoError) { qCDebug(networking) << "Got error retrieving asset info for" << _hash; - - _state = FINISHED; + + _state = Finished; emit finished(this); - _error = (serverError == AssetServerError::AssetNotFound) ? NotFound : UnknownError; - return; } @@ -63,11 +74,25 @@ void AssetRequest::start() { int start = 0, end = _info.size; auto assetClient = DependencyManager::get(); - assetClient->getAsset(_hash, _extension, start, end, [this, start, end](AssetServerError serverError, + assetClient->getAsset(_hash, _extension, start, end, [this, start, end](bool responseReceived, AssetServerError serverError, const QByteArray& data) { Q_ASSERT(data.size() == (end - start)); - if (serverError == AssetServerError::NoError) { + if (!responseReceived) { + _error = NetworkError; + } else if (serverError != AssetServerError::NoError) { + switch (serverError) { + case AssetServerError::AssetNotFound: + _error = NotFound; + break; + case AssetServerError::InvalidByteRange: + _error = InvalidByteRange; + break; + default: + _error = UnknownError; + break; + } + } else { // we need to check the hash of the received data to make sure it matches what we expect if (hashData(data).toHex() == _hash) { @@ -79,28 +104,13 @@ void AssetRequest::start() { _error = HashVerificationFailed; } - } else { - switch (serverError) { - case AssetServerError::AssetNotFound: - _error = NotFound; - break; - case AssetServerError::InvalidByteRange: - _error = InvalidByteRange; - break; - case AssetServerError::NetworkError: - _error = Error::NetworkError; - break; - default: - _error = UnknownError; - break; - } } if (_error != NoError) { qCDebug(networking) << "Got error retrieving asset" << _hash << "- error code" << _error; } - _state = FINISHED; + _state = Finished; emit finished(this); }); }); diff --git a/libraries/networking/src/AssetUpload.cpp b/libraries/networking/src/AssetUpload.cpp index f51cb4a7b3..9c9d172959 100644 --- a/libraries/networking/src/AssetUpload.cpp +++ b/libraries/networking/src/AssetUpload.cpp @@ -43,20 +43,24 @@ void AssetUpload::start() { qDebug() << "Attempting to upload" << _filename << "to asset-server."; - assetClient->uploadAsset(data, _extension, [this](AssetServerError error, const QString& hash){ - switch (error) { - case AssetServerError::NoError: - _error = NoError; - break; - case AssetServerError::AssetTooLarge: - _error = TooLarge; - break; - case AssetServerError::PermissionDenied: - _error = PermissionDenied; - break; - default: - _error = FileOpenError; - break; + assetClient->uploadAsset(data, _extension, [this](bool responseReceived, AssetServerError error, const QString& hash){ + if (!responseReceived) { + _error = NetworkError; + } else { + switch (error) { + case AssetServerError::NoError: + _error = NoError; + break; + case AssetServerError::AssetTooLarge: + _error = TooLarge; + break; + case AssetServerError::PermissionDenied: + _error = PermissionDenied; + break; + default: + _error = FileOpenError; + break; + } } emit finished(this, hash); }); diff --git a/libraries/networking/src/AssetUtils.h b/libraries/networking/src/AssetUtils.h index c944657285..b18ff329fc 100644 --- a/libraries/networking/src/AssetUtils.h +++ b/libraries/networking/src/AssetUtils.h @@ -28,8 +28,7 @@ enum AssetServerError : uint8_t { AssetNotFound, InvalidByteRange, AssetTooLarge, - PermissionDenied, - NetworkError = 128 + PermissionDenied }; const QString ATP_SCHEME = "atp"; From b8c1c279c62e1e371f519e4b18e8c55b0809de2f Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Mon, 14 Sep 2015 11:22:55 -0700 Subject: [PATCH 08/11] Update AssetRequest::State enum to use camelcase --- libraries/networking/src/AssetRequest.cpp | 6 +++--- libraries/networking/src/AssetRequest.h | 10 +++++----- libraries/networking/src/AssetUpload.h | 1 + 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/libraries/networking/src/AssetRequest.cpp b/libraries/networking/src/AssetRequest.cpp index f9d5c70bf5..c0e5f0b6f2 100644 --- a/libraries/networking/src/AssetRequest.cpp +++ b/libraries/networking/src/AssetRequest.cpp @@ -33,12 +33,12 @@ void AssetRequest::start() { return; } - if (_state != NOT_STARTED) { + if (_state != NotStarted) { qCWarning(networking) << "AssetRequest already started."; return; } - _state = WAITING_FOR_INFO; + _state = WaitingForInfo; auto assetClient = DependencyManager::get(); assetClient->getAssetInfo(_hash, _extension, [this](bool responseReceived, AssetServerError serverError, AssetInfo info) { @@ -66,7 +66,7 @@ void AssetRequest::start() { return; } - _state = WAITING_FOR_DATA; + _state = WaitingForData; _data.resize(info.size); qCDebug(networking) << "Got size of " << _hash << " : " << info.size << " bytes"; diff --git a/libraries/networking/src/AssetRequest.h b/libraries/networking/src/AssetRequest.h index 3748703563..9bd224b35e 100644 --- a/libraries/networking/src/AssetRequest.h +++ b/libraries/networking/src/AssetRequest.h @@ -24,10 +24,10 @@ class AssetRequest : public QObject { Q_OBJECT public: enum State { - NOT_STARTED = 0, - WAITING_FOR_INFO, - WAITING_FOR_DATA, - FINISHED + NotStarted = 0, + WaitingForInfo, + WaitingForData, + Finished }; enum Error { @@ -52,7 +52,7 @@ signals: void progress(qint64 totalReceived, qint64 total); private: - State _state = NOT_STARTED; + State _state = NotStarted; Error _error = NoError; AssetInfo _info; uint64_t _totalReceived { 0 }; diff --git a/libraries/networking/src/AssetUpload.h b/libraries/networking/src/AssetUpload.h index 2dfada06f2..80faa8a9c1 100644 --- a/libraries/networking/src/AssetUpload.h +++ b/libraries/networking/src/AssetUpload.h @@ -28,6 +28,7 @@ public: enum Error { NoError = 0, + NetworkError, Timeout, TooLarge, PermissionDenied, From e87fcb78cd3b7775c8da93014bce04ab5a32c439 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Mon, 14 Sep 2015 11:34:51 -0700 Subject: [PATCH 09/11] Add proper cleanup of AssetRequest in AssetResourceRequest --- .../networking/src/AssetResourceRequest.cpp | 21 +++++++++++++------ .../networking/src/AssetResourceRequest.h | 5 +++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/libraries/networking/src/AssetResourceRequest.cpp b/libraries/networking/src/AssetResourceRequest.cpp index ec6c4bec0d..c52381f30a 100644 --- a/libraries/networking/src/AssetResourceRequest.cpp +++ b/libraries/networking/src/AssetResourceRequest.cpp @@ -12,9 +12,14 @@ #include "AssetResourceRequest.h" #include "AssetClient.h" -#include "AssetRequest.h" #include "AssetUtils.h" +AssetResourceRequest::~AssetResourceRequest() { + if (_assetRequest) { + _assetRequest->deleteLater(); + } +} + void AssetResourceRequest::doSend() { // Make request to atp auto assetClient = DependencyManager::get(); @@ -31,9 +36,9 @@ void AssetResourceRequest::doSend() { return; } - auto request = assetClient->createRequest(hash, extension); + _assetRequest = assetClient->createRequest(hash, extension); - if (!request) { + if (!_assetRequest) { _result = ServerUnavailable; _state = Finished; @@ -42,9 +47,10 @@ void AssetResourceRequest::doSend() { return; } - connect(request, &AssetRequest::progress, this, &AssetResourceRequest::progress); - QObject::connect(request, &AssetRequest::finished, [this](AssetRequest* req) mutable { + connect(_assetRequest, &AssetRequest::progress, this, &AssetResourceRequest::progress); + QObject::connect(_assetRequest, &AssetRequest::finished, [this](AssetRequest* req) mutable { Q_ASSERT(_state == InProgress); + Q_ASSERT(req == _assetRequest); Q_ASSERT(req->getState() == AssetRequest::FINISHED); switch (req->getError()) { @@ -65,9 +71,12 @@ void AssetResourceRequest::doSend() { _state = Finished; emit finished(); + + _assetRequest->deleteLater(); + _assetRequest = nullptr; }); - request->start(); + _assetRequest->start(); } void AssetResourceRequest::onDownloadProgress(qint64 bytesReceived, qint64 bytesTotal) { diff --git a/libraries/networking/src/AssetResourceRequest.h b/libraries/networking/src/AssetResourceRequest.h index fb9c25e092..aee26a5d5a 100644 --- a/libraries/networking/src/AssetResourceRequest.h +++ b/libraries/networking/src/AssetResourceRequest.h @@ -14,18 +14,23 @@ #include +#include "AssetRequest.h" #include "ResourceRequest.h" class AssetResourceRequest : public ResourceRequest { Q_OBJECT public: AssetResourceRequest(QObject* parent, const QUrl& url) : ResourceRequest(parent, url) { } + ~AssetResourceRequest(); protected: virtual void doSend() override; private slots: void onDownloadProgress(qint64 bytesReceived, qint64 bytesTotal); + +private: + AssetRequest* _assetRequest; }; #endif From 42d21e4dc1eec4af2eff094ce3335434923af0b0 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Mon, 14 Sep 2015 11:56:47 -0700 Subject: [PATCH 10/11] Update AssetClient's node killed handling to check if node is asset server first --- libraries/networking/src/AssetClient.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 298f68497e..c13bc07be1 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -286,6 +286,10 @@ void AssetClient::handleAssetUploadReply(QSharedPointer packet, Shared } void AssetClient::handleNodeKilled(SharedNodePointer node) { + if (node->getType() != NodeType::AssetServer) { + return; + } + { auto messageMapIt = _pendingRequests.find(node); if (messageMapIt != _pendingRequests.end()) { From 29d26a1f9a349de957e20cf621603dd5e9fed9e2 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Mon, 14 Sep 2015 12:17:36 -0700 Subject: [PATCH 11/11] Fix namespace issues std:: template specialization --- libraries/networking/src/AssetClient.h | 2 ++ libraries/networking/src/Node.h | 16 +++++++++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/libraries/networking/src/AssetClient.h b/libraries/networking/src/AssetClient.h index 1f5a61dc55..6557954467 100644 --- a/libraries/networking/src/AssetClient.h +++ b/libraries/networking/src/AssetClient.h @@ -34,6 +34,8 @@ using ReceivedAssetCallback = std::function; using UploadResultCallback = std::function; + + class AssetClient : public QObject, public Dependency { Q_OBJECT public: diff --git a/libraries/networking/src/Node.h b/libraries/networking/src/Node.h index 5875295716..38d6678ba5 100644 --- a/libraries/networking/src/Node.h +++ b/libraries/networking/src/Node.h @@ -88,13 +88,15 @@ private: typedef QSharedPointer SharedNodePointer; Q_DECLARE_METATYPE(SharedNodePointer) -template<> -struct std::hash { - std::size_t operator()(const SharedNodePointer& p) const { - // Return the hash of the pointer - return std::hash()(p.data()); - } -}; +namespace std { + template<> + struct hash { + size_t operator()(const SharedNodePointer& p) const { + // Return the hash of the pointer + return hash()(p.data()); + } + }; +} QDebug operator<<(QDebug debug, const Node& node);