From eedf8cfc7ea4861326b18229cca2ca24d4183ad1 Mon Sep 17 00:00:00 2001 From: Huyen Chau Nguyen Date: Wed, 17 Oct 2018 20:15:08 +0200 Subject: [core] add tests for handling requests with different priorities - ensure that low priority requests are handled last - add option to set the number of maximum concurrent requests for tests - some style fixups --- include/mbgl/storage/online_file_source.hpp | 1 + platform/default/online_file_source.cpp | 33 ++++++----- test/storage/offline_download.test.cpp | 2 +- test/storage/online_file_source.test.cpp | 91 +++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 14 deletions(-) diff --git a/include/mbgl/storage/online_file_source.hpp b/include/mbgl/storage/online_file_source.hpp index 28d70ce544..b921413490 100644 --- a/include/mbgl/storage/online_file_source.hpp +++ b/include/mbgl/storage/online_file_source.hpp @@ -26,6 +26,7 @@ public: // For testing only. void setOnlineStatus(bool); + void setMaximumConcurrentRequestsOverride(uint32_t); private: friend class OnlineFileRequest; diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp index 47ebbbeb45..6c930b3fec 100644 --- a/platform/default/online_file_source.cpp +++ b/platform/default/online_file_source.cpp @@ -98,7 +98,7 @@ public: assert(activeRequests.find(request) == activeRequests.end()); assert(!request->request); - if (activeRequests.size() >= HTTPFileSource::maximumConcurrentRequests()) { + if (activeRequests.size() >= getMaximumConcurrentRequests()) { queueRequest(request); } else { activateRequest(request); @@ -134,7 +134,9 @@ public: auto request = pendingRequests.pop(); - if (request) activateRequest(*request); + if (request) { + activateRequest(*request); + } } bool isPending(OnlineFileRequest* request) { @@ -154,15 +156,19 @@ public: networkIsReachableAgain(); } - void setMaximumConcurrentRequestsOverride(const uint32_t override) { - maximumConcurrentRequestsOverride = override; + void setMaximumConcurrentRequestsOverride(const uint32_t maximumConcurrentRequestsOverride_) { + maximumConcurrentRequestsOverride = maximumConcurrentRequestsOverride_; } private: - uint32_t getMaximumConcurrentRequests() { - if (maximumConcurrentRequestsOverride > 0) return maximumConcurrentRequestsOverride; - else return HTTPFileSource::maximumConcurrentRequests(); + uint32_t getMaximumConcurrentRequests() const { + if (maximumConcurrentRequestsOverride > 0) { + return maximumConcurrentRequestsOverride; + } + else { + return HTTPFileSource::maximumConcurrentRequests(); + } } void networkIsReachableAgain() { @@ -188,17 +194,13 @@ private: std::list queue; std::list::iterator firstLowPriorityRequest; - void remove(OnlineFileRequest* request) { + void remove(const OnlineFileRequest* request) { auto it = std::find(queue.begin(), queue.end(), request); if (it != queue.end()) { if (it == firstLowPriorityRequest) { firstLowPriorityRequest++; } queue.erase(it); - - if (queue.empty()) { - first_low = queue.begin(); - } } } @@ -232,7 +234,7 @@ private: return optional(next); } - bool contains(OnlineFileRequest* request) { + bool contains(OnlineFileRequest* request) const { return (std::find(queue.begin(), queue.end(), request) != queue.end()); } @@ -258,6 +260,7 @@ private: std::unordered_set activeRequests; bool online = true; + uint32_t maximumConcurrentRequestsOverride = 0; HTTPFileSource httpFileSource; util::AsyncTask reachability { std::bind(&Impl::networkIsReachableAgain, this) }; }; @@ -475,4 +478,8 @@ void OnlineFileSource::setOnlineStatus(const bool status) { impl->setOnlineStatus(status); } +void OnlineFileSource::setMaximumConcurrentRequestsOverride(const uint32_t maximumConcurrentRequestsOverride) { + impl->setMaximumConcurrentRequestsOverride(maximumConcurrentRequestsOverride); +} + } // namespace mbgl diff --git a/test/storage/offline_download.test.cpp b/test/storage/offline_download.test.cpp index 01baf29592..4db807a204 100644 --- a/test/storage/offline_download.test.cpp +++ b/test/storage/offline_download.test.cpp @@ -663,7 +663,7 @@ TEST(OfflineDownload, Deactivate) { } -TEST(OfflineDownload, LowPriorityRequests) { +TEST(OfflineDownload, AllOfflineRequestsHaveLowPriority) { OfflineTest test; auto region = test.createRegion(); ASSERT_TRUE(region); diff --git a/test/storage/online_file_source.test.cpp b/test/storage/online_file_source.test.cpp index b5a7c139d3..5021a513e3 100644 --- a/test/storage/online_file_source.test.cpp +++ b/test/storage/online_file_source.test.cpp @@ -425,3 +425,94 @@ TEST(OnlineFileSource, ChangeAPIBaseURL){ fs.setAPIBaseURL(customURL); EXPECT_EQ(customURL, fs.getAPIBaseURL()); } + + +TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequests)) { + util::RunLoop loop; + OnlineFileSource fs; + std::size_t response_counter = 0; + const std::size_t NUM_REQUESTS = 3; + + fs.setMaximumConcurrentRequestsOverride(1); + + NetworkStatus::Set(NetworkStatus::Status::Offline); + + // requesting a low priority resource + Resource low_prio{ Resource::Unknown, "http://127.0.0.1:3000/load/1" }; + low_prio.setPriority(Resource::Priority::Low); + std::unique_ptr req_0 = fs.request(low_prio, [&](Response) { + response_counter++; + req_0.reset(); + EXPECT_EQ(response_counter, NUM_REQUESTS); // make sure this is responded last + loop.stop(); + }); + + // requesting two "regular" resources + Resource regular1{ Resource::Unknown, "http://127.0.0.1:3000/load/2" }; + std::unique_ptr req_1 = fs.request(regular1, [&](Response) { + response_counter++; + req_1.reset(); + }); + Resource regular2{ Resource::Unknown, "http://127.0.0.1:3000/load/3" }; + std::unique_ptr req_2 = fs.request(regular2, [&](Response) { + response_counter++; + req_2.reset(); + }); + + NetworkStatus::Set(NetworkStatus::Status::Online); + + loop.run(); +} + + +TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequestsMany)) { + util::RunLoop loop; + OnlineFileSource fs; + int response_counter = 0; + int correct_low = 0; + int correct_regular = 0; + + + fs.setMaximumConcurrentRequestsOverride(1); + + NetworkStatus::Set(NetworkStatus::Status::Offline); + + std::vector> collector; + + for (int num_reqs = 0; num_reqs < 20; num_reqs++) { + if (num_reqs % 2 == 0) { + std::unique_ptr req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/load/" + std::to_string(num_reqs), Resource::Priority::Regular }, [&](Response) { + response_counter++; + + if (response_counter <= 10) { // count the high priority requests that arrive late correctly + correct_regular++; + } + }); + collector.push_back(std::move(req)); + } + else { + std::unique_ptr req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/load/" + std::to_string(num_reqs), Resource::Priority::Low }, [&](Response) { + response_counter++; + + if (response_counter > 10) { // count the low priority requests that arrive late correctly + correct_low++; + } + + // stop and check correctness after last low priority request is responded + if (20 == response_counter) { + loop.stop(); + for (auto& collected_req : collector) { + collected_req.reset(); + } + ASSERT_TRUE(correct_low >= 9); + ASSERT_TRUE(correct_regular >= 9); + } + }); + collector.push_back(std::move(req)); + } + } + + NetworkStatus::Set(NetworkStatus::Status::Online); + + loop.run(); +} \ No newline at end of file -- cgit v1.2.1