[core] all requests have to be canceled explicitly now

By not automatically destroying Request objects after the result has been delivered, we are making sure that we can potentially fire the callback multiple times without adverse effects. This means that you have to hold on to the result of fs->request(), can explicitly cancel it if you don't want to be notified of data changes anymore. Not doing so will monitor the request indefinitely and will prevent the app from exiting.

13 files changed, 70 insertions, 35 deletions

diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp
index 87de62025e..0fba2ba5e7 100644
--- a/test/storage/cache_response.cpp
+++ b/test/storage/cache_response.cpp
@@ -15,7 +15,8 @@ TEST_F(Storage, CacheResponse) {
 
     const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/cache" };
 
-    fs.request(resource, uv_default_loop(), [&](const Response &res) {
+    Request* req = fs.request(resource, uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Response 1", res.data);
         EXPECT_LT(0, res.expires);
@@ -23,7 +24,8 @@ TEST_F(Storage, CacheResponse) {
         EXPECT_EQ("", res.etag);
         EXPECT_EQ("", res.message);
 
-        fs.request(resource, uv_default_loop(), [&, res](const Response &res2) {
+        req = fs.request(resource, uv_default_loop(), [&, res](const Response &res2) {
+            fs.cancel(req);
             EXPECT_EQ(res.status, res2.status);
             EXPECT_EQ(res.data, res2.data);
             EXPECT_EQ(res.expires, res2.expires);
diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp
index 1bfd6941b7..8ebedef01b 100644
--- a/test/storage/cache_revalidate.cpp
+++ b/test/storage/cache_revalidate.cpp
@@ -16,7 +16,8 @@ TEST_F(Storage, CacheRevalidate) {
     DefaultFileSource fs(&cache);
 
     const Resource revalidateSame { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" };
-    fs.request(revalidateSame, uv_default_loop(), [&](const Response &res) {
+    Request* req = fs.request(revalidateSame, uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Response", res.data);
         EXPECT_EQ(0, res.expires);
@@ -24,7 +25,8 @@ TEST_F(Storage, CacheRevalidate) {
         EXPECT_EQ("snowfall", res.etag);
         EXPECT_EQ("", res.message);
 
-        fs.request(revalidateSame, uv_default_loop(), [&, res](const Response &res2) {
+        req = fs.request(revalidateSame, uv_default_loop(), [&, res](const Response &res2) {
+            fs.cancel(req);
             EXPECT_EQ(Response::Successful, res2.status);
             EXPECT_EQ("Response", res2.data);
             // We use this to indicate that a 304 reply came back.
@@ -40,7 +42,8 @@ TEST_F(Storage, CacheRevalidate) {
 
     const Resource revalidateModified{ Resource::Unknown,
                                        "http://127.0.0.1:3000/revalidate-modified" };
-    fs.request(revalidateModified, uv_default_loop(), [&](const Response &res) {
+    Request* req2 = fs.request(revalidateModified, uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req2);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Response", res.data);
         EXPECT_EQ(0, res.expires);
@@ -48,7 +51,8 @@ TEST_F(Storage, CacheRevalidate) {
         EXPECT_EQ("", res.etag);
         EXPECT_EQ("", res.message);
 
-        fs.request(revalidateModified, uv_default_loop(), [&, res](const Response &res2) {
+        req2 = fs.request(revalidateModified, uv_default_loop(), [&, res](const Response &res2) {
+            fs.cancel(req2);
             EXPECT_EQ(Response::Successful, res2.status);
             EXPECT_EQ("Response", res2.data);
             // We use this to indicate that a 304 reply came back.
@@ -62,7 +66,8 @@ TEST_F(Storage, CacheRevalidate) {
     });
 
     const Resource revalidateEtag { Resource::Unknown, "http://127.0.0.1:3000/revalidate-etag" };
-    fs.request(revalidateEtag, uv_default_loop(), [&](const Response &res) {
+    Request* req3 = fs.request(revalidateEtag, uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req3);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Response 1", res.data);
         EXPECT_EQ(0, res.expires);
@@ -70,7 +75,8 @@ TEST_F(Storage, CacheRevalidate) {
         EXPECT_EQ("response-1", res.etag);
         EXPECT_EQ("", res.message);
 
-        fs.request(revalidateEtag, uv_default_loop(), [&, res](const Response &res2) {
+        req3 = fs.request(revalidateEtag, uv_default_loop(), [&, res](const Response &res2) {
+            fs.cancel(req3);
             EXPECT_EQ(Response::Successful, res2.status);
             EXPECT_EQ("Response 2", res2.data);
             EXPECT_EQ(0, res2.expires);
diff --git a/test/storage/database.cpp b/test/storage/database.cpp
index b40474c004..20f96305b1 100644
--- a/test/storage/database.cpp
+++ b/test/storage/database.cpp
@@ -181,7 +181,7 @@ TEST_F(Storage, DatabaseLockedWrite) {
         response->data = "Demo";
         cache.put({ Resource::Unknown, "mapbox://test" }, response);
         cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr<Response> res) {
-            EXPECT_NE(nullptr, res.get());
+            ASSERT_NE(nullptr, res.get());
             EXPECT_EQ("Demo", res->data);
         });
 
@@ -258,7 +258,7 @@ TEST_F(Storage, DatabaseDeleted) {
         response->data = "Demo";
         cache.put({ Resource::Unknown, "mapbox://test" }, response);
         cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr<Response> res) {
-            EXPECT_NE(nullptr, res.get());
+            ASSERT_NE(nullptr, res.get());
             EXPECT_EQ("Demo", res->data);
         });
 
@@ -275,7 +275,7 @@ TEST_F(Storage, DatabaseDeleted) {
         response->data = "Demo";
         cache.put({ Resource::Unknown, "mapbox://test" }, response);
         cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr<Response> res) {
-            EXPECT_NE(nullptr, res.get());
+            ASSERT_NE(nullptr, res.get());
             EXPECT_EQ("Demo", res->data);
         });
 
@@ -305,7 +305,7 @@ TEST_F(Storage, DatabaseInvalid) {
         response->data = "Demo";
         cache.put({ Resource::Unknown, "mapbox://test" }, response);
         cache.get({ Resource::Unknown, "mapbox://test" }, [] (std::unique_ptr<Response> res) {
-            EXPECT_NE(nullptr, res.get());
+            ASSERT_NE(nullptr, res.get());
             EXPECT_EQ("Demo", res->data);
         });
 
diff --git a/test/storage/directory_reading.cpp b/test/storage/directory_reading.cpp
index 4459b42c2e..11298dff8b 100644
--- a/test/storage/directory_reading.cpp
+++ b/test/storage/directory_reading.cpp
@@ -15,8 +15,9 @@ TEST_F(Storage, AssetReadDirectory) {
     DefaultFileSource fs(nullptr);
 #endif
 
-    fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(),
+    Request* req = fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(),
                [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Error, res.status);
         EXPECT_EQ(0ul, res.data.size());
         EXPECT_EQ(0, res.expires);
diff --git a/test/storage/file_reading.cpp b/test/storage/file_reading.cpp
index 8db911f65e..dc0620b78b 100644
--- a/test/storage/file_reading.cpp
+++ b/test/storage/file_reading.cpp
@@ -16,8 +16,9 @@ TEST_F(Storage, AssetEmptyFile) {
     DefaultFileSource fs(nullptr);
 #endif
 
-    fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(),
+    Request* req = fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(),
                [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ(0ul, res.data.size());
         EXPECT_EQ(0, res.expires);
@@ -41,8 +42,9 @@ TEST_F(Storage, AssetNonEmptyFile) {
     DefaultFileSource fs(nullptr);
 #endif
 
-    fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" },
+    Request* req = fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" },
                uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ(16ul, res.data.size());
         EXPECT_EQ(0, res.expires);
@@ -67,8 +69,9 @@ TEST_F(Storage, AssetNonExistentFile) {
     DefaultFileSource fs(nullptr);
 #endif
 
-    fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" },
+    Request* req = fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" },
                uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Error, res.status);
         EXPECT_EQ(0ul, res.data.size());
         EXPECT_EQ(0, res.expires);
diff --git a/test/storage/http_cancel.cpp b/test/storage/http_cancel.cpp
index a73182da55..2eb599eeaa 100644
--- a/test/storage/http_cancel.cpp
+++ b/test/storage/http_cancel.cpp
@@ -36,7 +36,8 @@ TEST_F(Storage, HTTPCancelMultiple) {
     auto req2 = fs.request(resource, uv_default_loop(), [&](const Response &) {
         ADD_FAILURE() << "Callback should not be called";
     });
-    fs.request(resource, uv_default_loop(), [&](const Response &res) {
+    Request* req = fs.request(resource, uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Hello World!", res.data);
         EXPECT_EQ(0, res.expires);
diff --git a/test/storage/http_coalescing.cpp b/test/storage/http_coalescing.cpp
index 5c90009f84..9dfa855501 100644
--- a/test/storage/http_coalescing.cpp
+++ b/test/storage/http_coalescing.cpp
@@ -40,8 +40,12 @@ TEST_F(Storage, HTTPCoalescing) {
 
     const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" };
 
+    Request* reqs[total];
     for (int i = 0; i < total; i++) {
-        fs.request(resource, uv_default_loop(), complete);
+        reqs[i] = fs.request(resource, uv_default_loop(), [&complete, &fs, &reqs, i] (const Response &res) {
+            fs.cancel(reqs[i]);
+            complete(res);
+        });
     }
 
     uv_run(uv_default_loop(), UV_RUN_DEFAULT);
diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp
index 7edce61e5c..aebc75405d 100644
--- a/test/storage/http_error.cpp
+++ b/test/storage/http_error.cpp
@@ -30,8 +30,9 @@ TEST_F(Storage, HTTPError) {
 
     auto start = uv_hrtime();
 
-    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(),
+    Request* req1 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(),
                [&](const Response &res) {
+        fs.cancel(req1);
         const auto duration = double(uv_hrtime() - start) / 1e9;
         EXPECT_LT(1, duration) << "Backoff timer didn't wait 1 second";
         EXPECT_GT(1.2, duration) << "Backoff timer fired too late";
@@ -45,8 +46,9 @@ TEST_F(Storage, HTTPError) {
         HTTPTemporaryError.finish();
     });
 
-    fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(),
+    Request* req2 = fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(),
                [&](const Response &res) {
+        fs.cancel(req2);
         const auto duration = double(uv_hrtime() - start) / 1e9;
         // 1.5 seconds == 4 retries, with a 500ms timeout (see above).
         EXPECT_LT(1.5, duration) << "Resource wasn't retried the correct number of times";
diff --git a/test/storage/http_header_parsing.cpp b/test/storage/http_header_parsing.cpp
index 5f18b163a4..13eab99125 100644
--- a/test/storage/http_header_parsing.cpp
+++ b/test/storage/http_header_parsing.cpp
@@ -15,9 +15,10 @@ TEST_F(Storage, HTTPHeaderParsing) {
 
     DefaultFileSource fs(nullptr);
 
-    fs.request({ Resource::Unknown,
+    Request* req1 = fs.request({ Resource::Unknown,
                  "http://127.0.0.1:3000/test?modified=1420794326&expires=1420797926&etag=foo" },
                uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req1);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Hello World!", res.data);
         EXPECT_EQ(1420797926, res.expires);
@@ -30,8 +31,9 @@ TEST_F(Storage, HTTPHeaderParsing) {
     int64_t now = std::chrono::duration_cast<std::chrono::seconds>(
                        SystemClock::now().time_since_epoch()).count();
 
-    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" },
+    Request* req2 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" },
                uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req2);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Hello World!", res.data);
         EXPECT_GT(2, std::abs(res.expires - now - 120)) << "Expiration date isn't about 120 seconds in the future";
diff --git a/test/storage/http_issue_1369.cpp b/test/storage/http_issue_1369.cpp
index 44c190c825..954cb3b3f5 100644
--- a/test/storage/http_issue_1369.cpp
+++ b/test/storage/http_issue_1369.cpp
@@ -30,7 +30,8 @@ TEST_F(Storage, HTTPIssue1369) {
         ADD_FAILURE() << "Callback should not be called";
     });
     fs.cancel(req);
-    fs.request(resource, uv_default_loop(), [&](const Response &res) {
+    req = fs.request(resource, uv_default_loop(), [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Hello World!", res.data);
         EXPECT_EQ(0, res.expires);
diff --git a/test/storage/http_load.cpp b/test/storage/http_load.cpp
index 2347a76ab4..fa0468a848 100644
--- a/test/storage/http_load.cpp
+++ b/test/storage/http_load.cpp
@@ -15,11 +15,15 @@ TEST_F(Storage, HTTPLoad) {
     const int max = 10000;
     int number = 1;
 
-    std::function<void()> req = [&]() {
+    Request* reqs[concurrency];
+
+    std::function<void(int)> req = [&](int i) {
         const auto current = number++;
-        fs.request({ Resource::Unknown,
+        reqs[i] = fs.request({ Resource::Unknown,
                      std::string("http://127.0.0.1:3000/load/") + std::to_string(current) },
-                   uv_default_loop(), [&, current](const Response &res) {
+                   uv_default_loop(), [&, i, current](const Response &res) {
+            fs.cancel(reqs[i]);
+            reqs[i] = nullptr;
             EXPECT_EQ(Response::Successful, res.status);
             EXPECT_EQ(std::string("Request ") +  std::to_string(current), res.data);
             EXPECT_EQ(0, res.expires);
@@ -28,7 +32,7 @@ TEST_F(Storage, HTTPLoad) {
             EXPECT_EQ("", res.message);
 
             if (number <= max) {
-                req();
+                req(i);
             } else if (current == max) {
                 HTTPLoad.finish();
             }
@@ -37,7 +41,7 @@ TEST_F(Storage, HTTPLoad) {
 
 
     for (int i = 0; i < concurrency; i++) {
-        req();
+        req(i);
     }
 
     uv_run(uv_default_loop(), UV_RUN_DEFAULT);
diff --git a/test/storage/http_other_loop.cpp b/test/storage/http_other_loop.cpp
index 64612f13df..43a655fba4 100644
--- a/test/storage/http_other_loop.cpp
+++ b/test/storage/http_other_loop.cpp
@@ -12,8 +12,9 @@ TEST_F(Storage, HTTPOtherLoop) {
     // This file source launches a separate thread to do the processing.
     DefaultFileSource fs(nullptr);
 
-    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(),
+    Request* req = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(),
                [&](const Response &res) {
+        fs.cancel(req);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Hello World!", res.data);
         EXPECT_EQ(0, res.expires);
diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp
index 78f69cae30..84af943025 100644
--- a/test/storage/http_reading.cpp
+++ b/test/storage/http_reading.cpp
@@ -3,6 +3,7 @@
 #include <uv.h>
 
 #include <mbgl/storage/default_file_source.hpp>
+#include <mbgl/util/exception.hpp>
 
 #include <future>
 
@@ -17,8 +18,9 @@ TEST_F(Storage, HTTPReading) {
 
     const auto mainThread = uv_thread_self();
 
-    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(),
+    Request* req1 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(),
                [&](const Response &res) {
+        fs.cancel(req1);
         EXPECT_EQ(uv_thread_self(), mainThread);
         EXPECT_EQ(Response::Successful, res.status);
         EXPECT_EQ("Hello World!", res.data);
@@ -29,8 +31,9 @@ TEST_F(Storage, HTTPReading) {
         HTTPTest.finish();
     });
 
-    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(),
+    Request* req2 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(),
                [&](const Response &res) {
+        fs.cancel(req2);
         EXPECT_EQ(uv_thread_self(), mainThread);
         EXPECT_EQ(Response::NotFound, res.status);
         EXPECT_EQ("", res.message);
@@ -40,8 +43,9 @@ TEST_F(Storage, HTTPReading) {
         HTTP404.finish();
     });
 
-    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/permanent-error" }, uv_default_loop(),
+    Request* req3 = fs.request({ Resource::Unknown, "http://127.0.0.1:3000/permanent-error" }, uv_default_loop(),
                [&](const Response &res) {
+        fs.cancel(req3);
         EXPECT_EQ(uv_thread_self(), mainThread);
         EXPECT_EQ(Response::Error, res.status);
         EXPECT_EQ("HTTP status code 500", res.message);
@@ -61,10 +65,14 @@ TEST_F(Storage, HTTPNoCallback) {
 
     DefaultFileSource fs(nullptr);
 
-    fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(),
+    try {
+        fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(),
                nullptr);
-
-    uv_run(uv_default_loop(), UV_RUN_DEFAULT);
+    } catch (const util::MisuseException& ex) {
+        EXPECT_EQ(std::string(ex.what()), "FileSource callback can't be empty");
+    } catch (const std::exception&) {
+        EXPECT_TRUE(false) << "Unhandled exception.";
+    }
 
     HTTPTest.finish();
 }