From 096a3edf39d23fbd4baa134938c16fed4f2e199c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= <mail@kkaefer.com>
Date: Thu, 15 Oct 2015 15:46:12 +0200
Subject: [PATCH] [core] use RAII-style lifetime tracking of Request objects

---
 src/mbgl/map/map_context.cpp        | 15 +++------------
 src/mbgl/map/map_context.hpp        |  2 +-
 src/mbgl/map/raster_tile_data.cpp   |  6 +-----
 src/mbgl/map/raster_tile_data.hpp   |  3 ++-
 src/mbgl/map/source.cpp             |  8 +-------
 src/mbgl/map/source.hpp             |  3 ++-
 src/mbgl/map/sprite.cpp             | 18 +++---------------
 src/mbgl/map/vector_tile_data.cpp   |  6 +-----
 src/mbgl/map/vector_tile_data.hpp   |  3 ++-
 src/mbgl/storage/request_holder.cpp | 12 ++++++++++++
 src/mbgl/storage/request_holder.hpp | 26 ++++++++++++++++++++++++++
 src/mbgl/text/glyph_pbf.cpp         |  8 +-------
 src/mbgl/text/glyph_pbf.hpp         |  3 ++-
 13 files changed, 57 insertions(+), 56 deletions(-)
 create mode 100644 src/mbgl/storage/request_holder.cpp
 create mode 100644 src/mbgl/storage/request_holder.hpp

diff --git a/src/mbgl/map/map_context.cpp b/src/mbgl/map/map_context.cpp
index 573882da8..0888ef284 100644
--- a/src/mbgl/map/map_context.cpp
+++ b/src/mbgl/map/map_context.cpp
@@ -52,10 +52,7 @@ MapContext::~MapContext() {
 void MapContext::cleanup() {
     view.notify();
 
-    if (styleRequest) {
-        util::ThreadContext::getFileSource()->cancel(styleRequest);
-        styleRequest = nullptr;
-    }
+    styleRequest = nullptr;
 
     // Explicit resets currently necessary because these abandon resources that need to be
     // cleaned up by glObjectStore.performCleanup();
@@ -95,13 +92,7 @@ void MapContext::setStyleURL(const std::string& url) {
         return;
     }
 
-    FileSource* fs = util::ThreadContext::getFileSource();
-
-    if (styleRequest) {
-        fs->cancel(styleRequest);
-        styleRequest = nullptr;
-    }
-
+    styleRequest = nullptr;
     styleURL = url;
     styleJSON.clear();
 
@@ -113,8 +104,8 @@ void MapContext::setStyleURL(const std::string& url) {
         base = styleURL.substr(0, pos + 1);
     }
 
+    FileSource* fs = util::ThreadContext::getFileSource();
     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/map_context.hpp b/src/mbgl/map/map_context.hpp
index b78d3283a..028a05e4b 100644
--- a/src/mbgl/map/map_context.hpp
+++ b/src/mbgl/map/map_context.hpp
@@ -90,7 +90,7 @@ private:
     std::string styleURL;
     std::string styleJSON;
 
-    Request* styleRequest = nullptr;
+    RequestHolder styleRequest;
 
     Map::StillImageCallback callback;
     size_t sourceCacheSize;
diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp
index c983a5737..4ae9189a0 100644
--- a/src/mbgl/map/raster_tile_data.cpp
+++ b/src/mbgl/map/raster_tile_data.cpp
@@ -31,7 +31,6 @@ 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) {
@@ -78,9 +77,6 @@ void RasterTileData::cancel() {
     if (state != State::obsolete) {
         state = State::obsolete;
     }
-    if (req) {
-        util::ThreadContext::getFileSource()->cancel(req);
-        req = nullptr;
-    }
+    req = nullptr;
     workRequest.reset();
 }
diff --git a/src/mbgl/map/raster_tile_data.hpp b/src/mbgl/map/raster_tile_data.hpp
index f5711e506..5c8b42ef9 100644
--- a/src/mbgl/map/raster_tile_data.hpp
+++ b/src/mbgl/map/raster_tile_data.hpp
@@ -4,6 +4,7 @@
 #include <mbgl/map/tile_data.hpp>
 #include <mbgl/style/style_properties.hpp>
 #include <mbgl/renderer/raster_bucket.hpp>
+#include <mbgl/storage/request_holder.hpp>
 
 namespace mbgl {
 
@@ -28,7 +29,7 @@ public:
 private:
     const SourceInfo& source;
     Worker& worker;
-    Request* req = nullptr;
+    RequestHolder req;
 
     RasterLayoutProperties layout;
     RasterBucket bucket;
diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp
index 3e8aba885..d723e308b 100644
--- a/src/mbgl/map/source.cpp
+++ b/src/mbgl/map/source.cpp
@@ -122,12 +122,7 @@ std::string SourceInfo::tileURL(const TileID& id, float pixelRatio) const {
 
 Source::Source() {}
 
-Source::~Source() {
-    if (req) {
-        util::ThreadContext::getFileSource()->cancel(req);
-        req = nullptr;
-    }
-}
+Source::~Source() = default;
 
 bool Source::isLoaded() const {
     if (!loaded) {
@@ -154,7 +149,6 @@ 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/source.hpp b/src/mbgl/map/source.hpp
index fceb5962a..f5b95ef87 100644
--- a/src/mbgl/map/source.hpp
+++ b/src/mbgl/map/source.hpp
@@ -5,6 +5,7 @@
 #include <mbgl/map/tile_data.hpp>
 #include <mbgl/map/tile_cache.hpp>
 #include <mbgl/style/types.hpp>
+#include <mbgl/storage/request_holder.hpp>
 
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/mat4.hpp>
@@ -129,7 +130,7 @@ private:
     std::map<TileID, std::weak_ptr<TileData>> tile_data;
     TileCache cache;
 
-    Request* req = nullptr;
+    RequestHolder req;
     Observer* observer_ = nullptr;
 };
 
diff --git a/src/mbgl/map/sprite.cpp b/src/mbgl/map/sprite.cpp
index 02bf0895c..9c099b4aa 100644
--- a/src/mbgl/map/sprite.cpp
+++ b/src/mbgl/map/sprite.cpp
@@ -5,6 +5,7 @@
 #include <mbgl/storage/file_source.hpp>
 #include <mbgl/storage/resource.hpp>
 #include <mbgl/storage/response.hpp>
+#include <mbgl/storage/request_holder.hpp>
 #include <mbgl/util/exception.hpp>
 #include <mbgl/util/raster.hpp>
 #include <mbgl/util/thread.hpp>
@@ -23,19 +24,8 @@ struct Sprite::Loader {
     bool loadedImage = false;
     std::unique_ptr<Data> data = std::make_unique<Data>();
 
-    Request* jsonRequest = nullptr;
-    Request* spriteRequest = nullptr;
-
-    ~Loader() {
-        if (jsonRequest) {
-            util::ThreadContext::getFileSource()->cancel(jsonRequest);
-            jsonRequest = nullptr;
-        }
-        if (spriteRequest) {
-            util::ThreadContext::getFileSource()->cancel(spriteRequest);
-            spriteRequest = nullptr;
-        }
-    }
+    RequestHolder jsonRequest;
+    RequestHolder spriteRequest;
 };
 
 Sprite::Sprite(const std::string& baseUrl, float pixelRatio_)
@@ -54,7 +44,6 @@ 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;
@@ -71,7 +60,6 @@ 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 a10dd64bd..6aa643b16 100644
--- a/src/mbgl/map/vector_tile_data.cpp
+++ b/src/mbgl/map/vector_tile_data.cpp
@@ -43,7 +43,6 @@ 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) {
@@ -139,9 +138,6 @@ void VectorTileData::cancel() {
     if (state != State::obsolete) {
         state = State::obsolete;
     }
-    if (req) {
-        util::ThreadContext::getFileSource()->cancel(req);
-        req = nullptr;
-    }
+    req = nullptr;
     workRequest.reset();
 }
diff --git a/src/mbgl/map/vector_tile_data.hpp b/src/mbgl/map/vector_tile_data.hpp
index c004e804b..1a1ff8406 100644
--- a/src/mbgl/map/vector_tile_data.hpp
+++ b/src/mbgl/map/vector_tile_data.hpp
@@ -3,6 +3,7 @@
 
 #include <mbgl/map/tile_data.hpp>
 #include <mbgl/map/tile_worker.hpp>
+#include <mbgl/storage/request_holder.hpp>
 
 #include <atomic>
 
@@ -40,7 +41,7 @@ private:
     std::unique_ptr<WorkRequest> workRequest;
     bool parsing = false;
     const SourceInfo& source;
-    Request* req = nullptr;
+    RequestHolder req;
     std::string data;
     float lastAngle = 0;
     float currentAngle;
diff --git a/src/mbgl/storage/request_holder.cpp b/src/mbgl/storage/request_holder.cpp
new file mode 100644
index 000000000..3a038623c
--- /dev/null
+++ b/src/mbgl/storage/request_holder.cpp
@@ -0,0 +1,12 @@
+#include <mbgl/storage/request_holder.hpp>
+#include <mbgl/storage/file_source.hpp>
+#include <mbgl/util/thread_context.hpp>
+
+namespace mbgl {
+
+void RequestHolder::Deleter::operator()(Request* req) const {
+    // This function is called by the unique_ptr's Deleter.
+    util::ThreadContext::getFileSource()->cancel(req);
+}
+
+}
diff --git a/src/mbgl/storage/request_holder.hpp b/src/mbgl/storage/request_holder.hpp
new file mode 100644
index 000000000..62edbfde7
--- /dev/null
+++ b/src/mbgl/storage/request_holder.hpp
@@ -0,0 +1,26 @@
+#ifndef MBGL_STORAGE_REQUEST_HOLDER
+#define MBGL_STORAGE_REQUEST_HOLDER
+
+#include <memory>
+
+namespace mbgl {
+
+class Request;
+
+class RequestHolder {
+public:
+    inline RequestHolder& operator=(Request* req) {
+        ptr = std::unique_ptr<Request, Deleter>(req);
+        return *this;
+    }
+
+private:
+    struct Deleter {
+        void operator()(Request*) const;
+    };
+    std::unique_ptr<Request, Deleter> ptr;
+};
+
+}
+
+#endif
diff --git a/src/mbgl/text/glyph_pbf.cpp b/src/mbgl/text/glyph_pbf.cpp
index f9f8afcb1..e37e656d9 100644
--- a/src/mbgl/text/glyph_pbf.cpp
+++ b/src/mbgl/text/glyph_pbf.cpp
@@ -75,7 +75,6 @@ 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) {
@@ -92,12 +91,7 @@ GlyphPBF::GlyphPBF(GlyphStore* store,
     req = fs->request({ Resource::Kind::Glyphs, url }, util::RunLoop::getLoop(), requestCallback);
 }
 
-GlyphPBF::~GlyphPBF() {
-    if (req) {
-        util::ThreadContext::getFileSource()->cancel(req);
-        req = nullptr;
-    }
-}
+GlyphPBF::~GlyphPBF() = default;
 
 void GlyphPBF::parse(GlyphStore* store, const std::string& fontStack, const std::string& url) {
     if (data.empty()) {
diff --git a/src/mbgl/text/glyph_pbf.hpp b/src/mbgl/text/glyph_pbf.hpp
index 2aa2134d1..205824bfe 100644
--- a/src/mbgl/text/glyph_pbf.hpp
+++ b/src/mbgl/text/glyph_pbf.hpp
@@ -3,6 +3,7 @@
 
 #include <mbgl/text/glyph.hpp>
 #include <mbgl/util/noncopyable.hpp>
+#include <mbgl/storage/request_holder.hpp>
 
 #include <atomic>
 #include <functional>
@@ -44,7 +45,7 @@ private:
     std::string data;
     std::atomic<bool> parsed;
 
-    Request* req = nullptr;
+    RequestHolder req;
 
     Observer* observer = nullptr;
 };