[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.

8 files changed, 24 insertions, 7 deletions

diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp
index d3324cb277..573882da87 100644
--- a/src/mbgl/map/map_context.cpp
+++ b/src/mbgl/map/map_context.cpp
@@ -53,8 +53,7 @@ void MapContext::cleanup() {
     view.notify();
 
     if (styleRequest) {
-        FileSource* fs = util::ThreadContext::getFileSource();
-        fs->cancel(styleRequest);
+        util::ThreadContext::getFileSource()->cancel(styleRequest);
         styleRequest = nullptr;
     }
 
@@ -100,6 +99,7 @@ void MapContext::setStyleURL(const std::string& url) {
 
     if (styleRequest) {
         fs->cancel(styleRequest);
+        styleRequest = nullptr;
     }
 
     styleURL = url;
@@ -114,6 +114,7 @@ void MapContext::setStyleURL(const std::string& url) {
     }
 
     styleRequest = fs->request({ Resource::Kind::Style, styleURL }, util::RunLoop::getLoop(), [this, base](const Response &res) {
+        util::ThreadContext::getFileSource()->cancel(styleRequest);
         styleRequest = nullptr;
 
         if (res.status == Response::Successful) {
diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp
index 27094510a6..c983a57375 100644
--- a/src/mbgl/map/raster_tile_data.cpp
+++ b/src/mbgl/map/raster_tile_data.cpp
@@ -31,6 +31,7 @@ void RasterTileData::request(float pixelRatio,
 
     FileSource* fs = util::ThreadContext::getFileSource();
     req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) {
+        util::ThreadContext::getFileSource()->cancel(req);
         req = nullptr;
 
         if (res.status == Response::NotFound) {
diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp
index 49e1d81610..3e8aba8859 100644
--- a/src/mbgl/map/source.cpp
+++ b/src/mbgl/map/source.cpp
@@ -125,6 +125,7 @@ Source::Source() {}
 Source::~Source() {
     if (req) {
         util::ThreadContext::getFileSource()->cancel(req);
+        req = nullptr;
     }
 }
 
@@ -153,6 +154,7 @@ void Source::load() {
 
     FileSource* fs = util::ThreadContext::getFileSource();
     req = fs->request({ Resource::Kind::Source, info.url }, util::RunLoop::getLoop(), [this](const Response &res) {
+        util::ThreadContext::getFileSource()->cancel(req);
         req = nullptr;
 
         if (res.status != Response::Successful) {
diff --git a/src/mbgl/map/sprite.cpp b/src/mbgl/map/sprite.cpp
index d5628b05b2..02bf0895c1 100644
--- a/src/mbgl/map/sprite.cpp
+++ b/src/mbgl/map/sprite.cpp
@@ -29,9 +29,11 @@ struct Sprite::Loader {
     ~Loader() {
         if (jsonRequest) {
             util::ThreadContext::getFileSource()->cancel(jsonRequest);
+            jsonRequest = nullptr;
         }
         if (spriteRequest) {
             util::ThreadContext::getFileSource()->cancel(spriteRequest);
+            spriteRequest = nullptr;
         }
     }
 };
@@ -52,6 +54,7 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_)
     FileSource* fs = util::ThreadContext::getFileSource();
     loader->jsonRequest = fs->request({ Resource::Kind::SpriteJSON, jsonURL }, util::RunLoop::getLoop(),
                                       [this, jsonURL](const Response& res) {
+        util::ThreadContext::getFileSource()->cancel(loader->jsonRequest);
         loader->jsonRequest = nullptr;
         if (res.status == Response::Successful) {
             loader->data->json = res.data;
@@ -68,6 +71,7 @@ Sprite::Sprite(const std::string& baseUrl, float pixelRatio_)
     loader->spriteRequest =
         fs->request({ Resource::Kind::SpriteImage, spriteURL }, util::RunLoop::getLoop(),
                     [this, spriteURL](const Response& res) {
+            util::ThreadContext::getFileSource()->cancel(loader->spriteRequest);
             loader->spriteRequest = nullptr;
             if (res.status == Response::Successful) {
                 loader->data->image = res.data;
diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp
index 3fe96599d6..a10dd64bd5 100644
--- a/src/mbgl/map/vector_tile_data.cpp
+++ b/src/mbgl/map/vector_tile_data.cpp
@@ -43,6 +43,7 @@ void VectorTileData::request(float pixelRatio, const std::function<void()>& call
 
     FileSource* fs = util::ThreadContext::getFileSource();
     req = fs->request({ Resource::Kind::Tile, url }, util::RunLoop::getLoop(), [url, callback, this](const Response &res) {
+        util::ThreadContext::getFileSource()->cancel(req);
         req = nullptr;
 
         if (res.status == Response::NotFound) {
diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp
index c33728db15..7b3cff8253 100644
--- a/src/mbgl/storage/default_file_source.cpp
+++ b/src/mbgl/storage/default_file_source.cpp
@@ -11,6 +11,7 @@
 #include <mbgl/util/chrono.hpp>
 #include <mbgl/util/thread.hpp>
 #include <mbgl/util/mapbox.hpp>
+#include <mbgl/util/exception.hpp>
 
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wshadow"
@@ -40,6 +41,10 @@ Request* DefaultFileSource::request(const Resource& resource,
                                     Callback callback) {
     assert(l);
 
+    if (!callback) {
+        throw util::MisuseException("FileSource callback can't be empty");
+    }
+
     std::string url;
 
     switch (resource.kind) {
@@ -70,6 +75,7 @@ Request* DefaultFileSource::request(const Resource& resource,
 }
 
 void DefaultFileSource::cancel(Request *req) {
+    assert(req);
     req->cancel();
     thread->invoke(&Impl::cancel, req);
 }
diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp
index 79d441442d..f8ad555379 100644
--- a/src/mbgl/storage/request.cpp
+++ b/src/mbgl/storage/request.cpp
@@ -6,6 +6,7 @@
 
 #include <cassert>
 #include <functional>
+#include <atomic>
 
 namespace mbgl {
 
@@ -40,18 +41,17 @@ void Request::invoke() {
     // The user could supply a null pointer or empty std::function as a callback. In this case, we
     // still do the file request, but we don't need to deliver a result.
     if (callback) {
-        callback(*response);
+        callback(*std::atomic_load(&response));
     }
-    delete this;
 }
 
 Request::~Request() = default;
 
 // Called in the FileSource thread.
 void Request::notify(const std::shared_ptr<const Response> &response_) {
-    assert(!response);
-    response = response_;
-    assert(response);
+    assert(!std::atomic_load(&response));
+    assert(response_);
+    std::atomic_store(&response, response_);
     async->send();
 }
 
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index ba92ce6b4e..f9f8afcb1b 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -75,6 +75,7 @@ GlyphPBF::GlyphPBF(GlyphStore* store,
     });
 
     auto requestCallback = [this, store, fontStack, url](const Response &res) {
+        util::ThreadContext::getFileSource()->cancel(req);
         req = nullptr;
 
         if (res.status != Response::Successful) {
@@ -94,6 +95,7 @@ GlyphPBF::GlyphPBF(GlyphStore* store,
 GlyphPBF::~GlyphPBF() {
     if (req) {
         util::ThreadContext::getFileSource()->cancel(req);
+        req = nullptr;
     }
 }