From 0542b81a036db3754ec042b6f49ba5c13f7c1b67 Mon Sep 17 00:00:00 2001 From: Leon Richardt Date: Sat, 16 Jan 2021 18:25:56 +0100 Subject: [PATCH] feat: add a `finally` callback to NetworkRequests (#2350) Co-authored-by: Rasmus Karlsson --- src/common/NetworkCommon.hpp | 1 + src/common/NetworkPrivate.cpp | 53 +++++++++++++- src/common/NetworkPrivate.hpp | 1 + src/common/NetworkRequest.cpp | 6 ++ src/common/NetworkRequest.hpp | 1 + tests/src/NetworkRequest.cpp | 125 ++++++++++++++++++++++++++++++++++ 6 files changed, 186 insertions(+), 1 deletion(-) diff --git a/src/common/NetworkCommon.hpp b/src/common/NetworkCommon.hpp index 858069e4e..5e4f5678a 100644 --- a/src/common/NetworkCommon.hpp +++ b/src/common/NetworkCommon.hpp @@ -12,6 +12,7 @@ class NetworkResult; using NetworkSuccessCallback = std::function; using NetworkErrorCallback = std::function; using NetworkReplyCreatedCallback = std::function; +using NetworkFinallyCallback = std::function; enum class NetworkRequestType { Get, diff --git a/src/common/NetworkPrivate.cpp b/src/common/NetworkPrivate.cpp index c68bc3d4a..e61bc4fe9 100644 --- a/src/common/NetworkPrivate.cpp +++ b/src/common/NetworkPrivate.cpp @@ -132,6 +132,7 @@ void loadUncached(const std::shared_ptr &data) data->timer_, &QTimer::timeout, worker, [reply, data]() { qCDebug(chatterinoCommon) << "Aborted!"; reply->abort(); + if (data->onError_) { postToThread([data] { @@ -139,6 +140,13 @@ void loadUncached(const std::shared_ptr &data) {}, NetworkResult::timedoutStatus)); }); } + + if (data->finally_) + { + postToThread([data] { + data->finally_(); + }); + } }); } @@ -159,17 +167,26 @@ void loadUncached(const std::shared_ptr &data) if (reply->error() == QNetworkReply::NetworkError::OperationCanceledError) { - //operation cancelled, most likely timed out + // Operation cancelled, most likely timed out return; } + if (data->onError_) { auto status = reply->attribute( QNetworkRequest::HttpStatusCodeAttribute); + // TODO: Should this always be run on the GUI thread? postToThread([data, code = status.toInt()] { data->onError_(NetworkResult({}, code)); }); } + + if (data->finally_) + { + postToThread([data] { + data->finally_(); + }); + } return; } @@ -196,6 +213,16 @@ void loadUncached(const std::shared_ptr &data) // log("finished {}", data->request_.url().toString()); reply->deleteLater(); + + if (data->finally_) + { + if (data->executeConcurrently_) + QtConcurrent::run([finally = std::move(data->finally_)] { + finally(); + }); + else + data->finally_(); + } }; if (data->timer_ != nullptr) @@ -271,6 +298,30 @@ void loadCached(const std::shared_ptr &data) }); } } + + if (data->finally_) + { + if (data->executeConcurrently_ || isGuiThread()) + { + if (data->hasCaller_ && !data->caller_.get()) + { + return; + } + + data->finally_(); + } + else + { + postToThread([data]() { + if (data->hasCaller_ && !data->caller_.get()) + { + return; + } + + data->finally_(); + }); + } + } } } diff --git a/src/common/NetworkPrivate.hpp b/src/common/NetworkPrivate.hpp index c25439f0c..050207cee 100644 --- a/src/common/NetworkPrivate.hpp +++ b/src/common/NetworkPrivate.hpp @@ -44,6 +44,7 @@ struct NetworkData { NetworkReplyCreatedCallback onReplyCreated_; NetworkErrorCallback onError_; NetworkSuccessCallback onSuccess_; + NetworkFinallyCallback finally_; NetworkRequestType requestType_ = NetworkRequestType::Get; diff --git a/src/common/NetworkRequest.cpp b/src/common/NetworkRequest.cpp index c39100b1b..0b6beb22f 100644 --- a/src/common/NetworkRequest.cpp +++ b/src/common/NetworkRequest.cpp @@ -79,6 +79,12 @@ NetworkRequest NetworkRequest::onSuccess(NetworkSuccessCallback cb) && return std::move(*this); } +NetworkRequest NetworkRequest::finally(NetworkFinallyCallback cb) && +{ + this->data->finally_ = cb; + return std::move(*this); +} + NetworkRequest NetworkRequest::header(const char *headerName, const char *value) && { diff --git a/src/common/NetworkRequest.hpp b/src/common/NetworkRequest.hpp index 509505079..2480eabb3 100644 --- a/src/common/NetworkRequest.hpp +++ b/src/common/NetworkRequest.hpp @@ -42,6 +42,7 @@ public: NetworkRequest onReplyCreated(NetworkReplyCreatedCallback cb) &&; NetworkRequest onError(NetworkErrorCallback cb) &&; NetworkRequest onSuccess(NetworkSuccessCallback cb) &&; + NetworkRequest finally(NetworkFinallyCallback cb) &&; NetworkRequest payload(const QByteArray &payload) &&; NetworkRequest cache() &&; diff --git a/tests/src/NetworkRequest.cpp b/tests/src/NetworkRequest.cpp index 8a04a11b3..8af91514f 100644 --- a/tests/src/NetworkRequest.cpp +++ b/tests/src/NetworkRequest.cpp @@ -67,6 +67,44 @@ TEST(NetworkRequest, Success) EXPECT_TRUE(NetworkManager::workerThread.isRunning()); } +TEST(NetworkRequest, FinallyCallbackOnSuccess) +{ + const std::vector codes{200, 201, 202, 203, 204, 205, 206}; + + EXPECT_TRUE(NetworkManager::workerThread.isRunning()); + + for (const auto code : codes) + { + auto url = getStatusURL(code); + std::mutex mut; + bool requestDone = false; + std::condition_variable requestDoneCondition; + + bool finallyCalled = false; + + NetworkRequest(url) + .finally( + [&mut, &requestDone, &requestDoneCondition, &finallyCalled] { + finallyCalled = true; + + { + std::unique_lock lck(mut); + requestDone = true; + } + requestDoneCondition.notify_one(); + }) + .execute(); + + // Wait for the request to finish + std::unique_lock lck(mut); + requestDoneCondition.wait(lck, [&requestDone] { + return requestDone; + }); + + EXPECT_TRUE(finallyCalled); + } +} + TEST(NetworkRequest, Error) { const std::vector codes{ @@ -118,6 +156,47 @@ TEST(NetworkRequest, Error) EXPECT_TRUE(NetworkManager::workerThread.isRunning()); } +TEST(NetworkRequest, FinallyCallbackOnError) +{ + const std::vector codes{ + 400, 401, 402, 403, 404, 405, 406, 407, 408, 409, 410, + 411, 412, 413, 414, 418, 500, 501, 502, 503, 504, + }; + + EXPECT_TRUE(NetworkManager::workerThread.isRunning()); + + for (const auto code : codes) + { + auto url = getStatusURL(code); + std::mutex mut; + bool requestDone = false; + std::condition_variable requestDoneCondition; + + bool finallyCalled = false; + + NetworkRequest(url) + .finally( + [&mut, &requestDone, &requestDoneCondition, &finallyCalled] { + finallyCalled = true; + + { + std::unique_lock lck(mut); + requestDone = true; + } + requestDoneCondition.notify_one(); + }) + .execute(); + + // Wait for the request to finish + std::unique_lock lck(mut); + requestDoneCondition.wait(lck, [&requestDone] { + return requestDone; + }); + + EXPECT_TRUE(finallyCalled); + } +} + TEST(NetworkRequest, TimeoutTimingOut) { EXPECT_TRUE(NetworkManager::workerThread.isRunning()); @@ -209,3 +288,49 @@ TEST(NetworkRequest, TimeoutNotTimingOut) EXPECT_TRUE(NetworkManager::workerThread.isRunning()); } + +TEST(NetworkRequest, FinallyCallbackOnTimeout) +{ + EXPECT_TRUE(NetworkManager::workerThread.isRunning()); + + auto url = "http://httpbin.org/delay/5"; + + std::mutex mut; + bool requestDone = false; + std::condition_variable requestDoneCondition; + bool finallyCalled = false; + bool onSuccessCalled = false; + bool onErrorCalled = false; + + NetworkRequest(url) + .timeout(1000) + .onSuccess([&](NetworkResult result) -> Outcome { + onSuccessCalled = true; + return Success; + }) + .onError([&](NetworkResult result) { + onErrorCalled = true; + EXPECT_EQ(result.status(), NetworkResult::timedoutStatus); + }) + .finally([&] { + finallyCalled = true; + + { + std::unique_lock lck(mut); + requestDone = true; + } + requestDoneCondition.notify_one(); + }) + .execute(); + + // Wait for the request to finish + std::unique_lock lck(mut); + requestDoneCondition.wait(lck, [&requestDone] { + return requestDone; + }); + + EXPECT_TRUE(finallyCalled); + EXPECT_TRUE(onErrorCalled); + EXPECT_FALSE(onSuccessCalled); + EXPECT_TRUE(NetworkManager::workerThread.isRunning()); +}