[core] add tests for handling requests with different prioritiesupstream/deprioritize_offline_downloads

- ensure that low priority requests are handled last
- add option to set the number of maximum concurrent requests for tests
- some style fixups

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<OnlineFileRequest*> queue;
         std::list<OnlineFileRequest*>::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<OnlineFileRequest*>(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<OnlineFileRequest*> 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<AsyncRequest> 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<AsyncRequest> 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<AsyncRequest> 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<std::unique_ptr<AsyncRequest>> collector;
+
+    for (int num_reqs = 0; num_reqs < 20; num_reqs++) {
+        if (num_reqs % 2 == 0) {
+            std::unique_ptr<AsyncRequest> 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<AsyncRequest> 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