[core] Blocking the Map Thread when destroying TileData objects

include/mbgl/util/run_loop.hpp

@@ -1,8 +1,11 @@
 #ifndef MBGL_UTIL_RUN_LOOP
 #define MBGL_UTIL_RUN_LOOP
 
+#include <mbgl/platform/log.hpp>
+#include <mbgl/util/chrono.hpp>
 #include <mbgl/util/noncopyable.hpp>
 #include <mbgl/util/util.hpp>
+#include <mbgl/util/thread_context.hpp>
 #include <mbgl/util/work_task.hpp>
 #include <mbgl/util/work_request.hpp>
 
@@ -58,7 +61,7 @@ class RunLoop : private util::noncopyable {
 
     // Post the cancellable work fn(args...) to this RunLoop.
     template <class Fn, class... Args>
-    std::unique_ptr<AsyncRequest>
+    std::unique_ptr<WorkRequest>
     invokeCancellable(Fn&& fn, Args&&... args) {
         auto flag = std::make_shared<std::atomic<bool>>();
         *flag = false;
@@ -76,7 +79,7 @@ class RunLoop : private util::noncopyable {
 
     // Invoke fn(args...) on this RunLoop, then invoke callback(results...) on the current RunLoop.
     template <class Fn, class Cb, class... Args>
-    std::unique_ptr<AsyncRequest>
+    std::unique_ptr<WorkRequest>
     invokeWithCallback(Fn&& fn, Cb&& callback, Args&&... args) {
         auto flag = std::make_shared<std::atomic<bool>>();
         *flag = false;
@@ -138,8 +141,33 @@ class RunLoop : private util::noncopyable {
         // If the task has completed and the after callback has executed, this will
         // do nothing.
         void cancel() override {
+#if defined(DEBUG)
+            auto now = Clock::now();
+
+            if (!tryCancel()) {
+                std::lock_guard<std::recursive_mutex> lock(mutex);
+                *canceled = true;
+
+                using std::chrono::duration_cast;
+                long elapsed = duration_cast<Milliseconds>(Clock::now() - now).count();
+
+                if (ThreadContext::currentlyOn(ThreadType::Map)) {
+                    Log::Warning(mbgl::Event::General, "Map thread blocked for %ld ms.", elapsed);
+                }
+            };
+#else
             std::lock_guard<std::recursive_mutex> lock(mutex);
             *canceled = true;
+#endif
+        }
+
+        bool tryCancel() override {
+            if (mutex.try_lock()) {
+                *canceled = true;
+                mutex.unlock();
+                return true;
+            }
+            return false;
         }
 
     private:

include/mbgl/util/work_request.hpp

@@ -15,6 +15,8 @@ class WorkRequest : public AsyncRequest {
     WorkRequest(Task);
     ~WorkRequest() override;
 
+    bool tryCancel();
+
 private:
     std::shared_ptr<WorkTask> task;
 };

include/mbgl/util/work_task.hpp

@@ -14,6 +14,7 @@ class WorkTask : private util::noncopyable {
 
     virtual void operator()() = 0;
     virtual void cancel() = 0;
+    virtual bool tryCancel() = 0;
 };
 
 } // namespace mbgl

src/mbgl/source/source.cpp

@@ -254,13 +254,15 @@ TileData::State Source::addTile(const TileID& tileID, const StyleUpdateParameter
 
         // If we don't find working tile data, we're just going to load it.
         if (type == SourceType::Raster) {
-            newTile->data = std::make_shared<RasterTileData>(normalizedID,
-                                                             parameters.pixelRatio,
-                                                             info->tiles.at(0),
-                                                             parameters.texturePool,
-                                                             parameters.worker,
-                                                             parameters.fileSource,
-                                                             callback);
+            newTile->data = std::shared_ptr<RasterTileData>(new RasterTileData(
+                normalizedID,
+                parameters.pixelRatio,
+                info->tiles.at(0),
+                parameters.texturePool,
+                parameters.worker,
+                parameters.fileSource,
+                callback
+            ), [this](TileData* data) { tileDataDeleter.add(data); });
         } else {
             std::unique_ptr<GeometryTileMonitor> monitor;
 
@@ -275,12 +277,14 @@ TileData::State Source::addTile(const TileID& tileID, const StyleUpdateParameter
                 return TileData::State::invalid;
             }
 
-            newTile->data = std::make_shared<VectorTileData>(normalizedID,
-                                                             std::move(monitor),
-                                                             id,
-                                                             parameters.style,
-                                                             parameters.mode,
-                                                             callback);
+            newTile->data = std::shared_ptr<VectorTileData>(new VectorTileData{
+                normalizedID,
+                std::move(monitor),
+                id,
+                parameters.style,
+                parameters.mode,
+                callback
+            }, [this](TileData* data) { tileDataDeleter.add(data); });
         }
 
         tileDataMap.emplace(newTile->data->id, newTile->data);
@@ -443,7 +447,7 @@ bool Source::update(const StyleUpdateParameters& parameters) {
         bool obsolete = retain_data.find(tile->id) == retain_data.end();
         if (obsolete) {
             if (!tileCache.has(tile->id.normalized().to_uint64())) {
-                tile->cancel();
+                tile->tryCancel();
             }
             return true;
         } else {

src/mbgl/source/source.hpp

@@ -3,6 +3,7 @@
 
 #include <mbgl/tile/tile_data.hpp>
 #include <mbgl/tile/tile_cache.hpp>
+#include <mbgl/tile/tile_data_deleter.hpp>
 #include <mbgl/source/source_info.hpp>
 
 #include <mbgl/util/mat4.hpp>
@@ -111,6 +112,8 @@ class Source : private util::noncopyable {
 
     Observer nullObserver;
     Observer* observer = &nullObserver;
+
+    TileDataDeleter tileDataDeleter;
 };
 
 } // namespace mbgl

src/mbgl/tile/raster_tile_data.cpp

@@ -45,7 +45,9 @@ RasterTileData::RasterTileData(const TileID& id_,
 
             workRequest.reset();
             workRequest = worker.parseRasterTile(std::make_unique<RasterBucket>(texturePool), res.data, [this, callback] (RasterTileParseResult result) {
+                WorkCompletedNotifier notifier(this);
                 workRequest.reset();
+
                 if (state != State::loaded) {
                     return;
                 }
@@ -67,19 +69,31 @@ RasterTileData::RasterTileData(const TileID& id_,
 }
 
 RasterTileData::~RasterTileData() {
-    cancel();
+    assert(tryCancel());
+    workRequest.reset();
 }
 
 Bucket* RasterTileData::getBucket(StyleLayer const&) {
     return bucket.get();
 }
 
-void RasterTileData::cancel() {
+bool RasterTileData::tryCancel(bool force) {
     if (state != State::obsolete) {
         state = State::obsolete;
     }
+
     req = nullptr;
     workRequest.reset();
+
+    if (force) {
+        workRequest.reset();
+    }
+
+    if (workRequest) {
+        return workRequest->tryCancel();
+    }
+
+    return true;
 }
 
 bool RasterTileData::hasData() const {

src/mbgl/tile/raster_tile_data.hpp

@@ -8,6 +8,7 @@ namespace mbgl {
 
 class FileSource;
 class AsyncRequest;
+class WorkRequest;
 class StyleLayer;
 namespace gl { class TexturePool; }
 
@@ -22,7 +23,7 @@ class RasterTileData : public TileData {
                    const std::function<void(std::exception_ptr)>& callback);
     ~RasterTileData();
 
-    void cancel() override;
+    bool tryCancel(bool force = false) override;
     Bucket* getBucket(StyleLayer const &layer_desc) override;
     bool hasData() const override;
 
@@ -31,7 +32,7 @@ class RasterTileData : public TileData {
     Worker& worker;
     std::unique_ptr<AsyncRequest> req;
     std::unique_ptr<Bucket> bucket;
-    std::unique_ptr<AsyncRequest> workRequest;
+    std::unique_ptr<WorkRequest> workRequest;
 };
 
 } // namespace mbgl

src/mbgl/tile/tile_data.hpp

@@ -21,6 +21,13 @@ class DebugBucket;
 
 class TileData : private util::noncopyable {
 public:
+    class Observer {
+    public:
+        virtual ~Observer() = default;
+
+        virtual void onTileDataWorkCompleted(TileData*) {};
+    };
+
     // initial:
     //   Initial state, only used when the TileData object is created.
     //
@@ -73,8 +80,11 @@ class TileData : private util::noncopyable {
     TileData(const TileID&);
     virtual ~TileData();
 
-    // Mark this tile as no longer needed and cancel any pending work.
-    virtual void cancel() = 0;
+    // Mark this tile as no longer needed and cancel any pending work. Returns
+    // true if the work was canceled, false otherwise. Canceling is not always
+    // possible because in some circumstances it might block. Force will force
+    // the cancellation and always return true, but it can potentially block.
+    virtual bool tryCancel(bool force = false) = 0;
 
     virtual Bucket* getBucket(const StyleLayer&) = 0;
 
@@ -95,6 +105,10 @@ class TileData : private util::noncopyable {
 
     void dumpDebugLogs() const;
 
+    void setObserver(Observer* observer_) {
+        observer = observer_;
+    }
+
     const TileID id;
     optional<SystemTimePoint> modified;
     optional<SystemTimePoint> expires;
@@ -103,7 +117,21 @@ class TileData : private util::noncopyable {
     std::unique_ptr<DebugBucket> debugBucket;
 
 protected:
+    class WorkCompletedNotifier {
+    public:
+        WorkCompletedNotifier(TileData* data_) : data(data_) {}
+        ~WorkCompletedNotifier() {
+            data->observer->onTileDataWorkCompleted(data);
+        }
+
+    private:
+        TileData* data;
+    };
+
     std::atomic<State> state;
+
+    Observer nullObserver;
+    Observer* observer = &nullObserver;
 };
 
 } // namespace mbgl

src/mbgl/tile/tile_data_deleter.cpp

@@ -0,0 +1,34 @@
+#include <mbgl/tile/tile_data_deleter.hpp>
+
+#include <mbgl/tile/tile_data.hpp>
+
+namespace mbgl {
+
+TileDataDeleter::~TileDataDeleter() {
+    // Force the canceling of all pending TileData
+    // before destroying. We have assert()s on Debug
+    // mode on TileData to make sure it doesn't block
+    // at the destructor unless explicitly canceled.
+    for (auto& entry : tileDataMap) {
+        entry.first->tryCancel(true);
+    }
+}
+
+void TileDataDeleter::add(TileData* data) {
+    std::unique_ptr<TileData> tileData(data);
+
+    // This custom deleter will try to cancel the task if it is
+    // non-blocking, otherwise we keep it and subscribe to the
+    // event when the work is completed. When the event triggers
+    // we delete the TileData because this time it won't block.
+    if (!tileData->tryCancel()) {
+        data->setObserver(this);
+        tileDataMap.emplace(data, std::move(tileData));
+    }
+}
+
+void TileDataDeleter::onTileDataWorkCompleted(TileData* data) {
+    tileDataMap.erase(data);
+}
+
+} // namespace mbgl

src/mbgl/tile/tile_data_deleter.hpp

@@ -0,0 +1,29 @@
+#ifndef MBGL_MAP_TILE_DATA_DELETER
+#define MBGL_MAP_TILE_DATA_DELETER
+
+#include <mbgl/tile/tile_data.hpp>
+#include <mbgl/util/noncopyable.hpp>
+
+#include <memory>
+#include <unordered_map>
+
+namespace mbgl {
+
+class TileData;
+
+class TileDataDeleter : public TileData::Observer, private util::noncopyable {
+public:
+    virtual ~TileDataDeleter();
+
+    void add(TileData*);
+
+    // TileData::Observer implementation.
+    void onTileDataWorkCompleted(TileData*) override;
+
+private:
+    std::unordered_map<TileData*, std::unique_ptr<TileData>> tileDataMap;
+};
+
+} // namespace mbgl
+
+#endif