make sure all of our async handles are stopped before stopping the map thread

include/mbgl/map/map.hpp

@@ -160,9 +160,9 @@ private:
     bool async = false;
     std::shared_ptr<uv::loop> loop;
     std::unique_ptr<uv::thread> thread;
-    uv_async_t *async_terminate = nullptr;
-    uv_async_t *async_render = nullptr;
-    uv_async_t *async_cleanup = nullptr;
+    std::unique_ptr<uv_async_t> async_terminate;
+    std::unique_ptr<uv_async_t> async_render;
+    std::unique_ptr<uv_async_t> async_cleanup;
 
 private:
     // If cleared, the next time the render thread attempts to render the map, it will *actually*
@@ -181,6 +181,11 @@ public:
     View &view;
 
 private:
+#ifndef NDEBUG
+    const unsigned long main_thread;
+    unsigned long map_thread = -1;
+#endif
+
     Transform transform;
     TransformState state;
 

include/mbgl/storage/base_request.hpp

@@ -25,7 +25,7 @@ private:
     BaseRequest& operator=(BaseRequest &&) = delete;
 
 public:
-    BaseRequest();
+    BaseRequest(const std::string &path);
     virtual ~BaseRequest();
 
     Callback *add(Callback &&callback, const std::shared_ptr<BaseRequest> &request);
@@ -34,10 +34,14 @@ public:
 
 public:
     const unsigned long thread_id;
+    const std::string path;
     std::unique_ptr<Response> response;
 
 private:
+    // This object may hold a shared_ptr to itself. It does this to prevent destruction of this object
+    // while a request is in progress.
     std::shared_ptr<BaseRequest> self;
+
     std::forward_list<std::unique_ptr<Callback>> callbacks;
 };
 

include/mbgl/storage/file_request.hpp

@@ -20,8 +20,6 @@ public:
     ~FileRequest();
 
 private:
-    const std::string path;
-    const unsigned long thread_id;
     FileRequestBaton *ptr = nullptr;
 
     friend struct FileRequestBaton;

include/mbgl/util/uv_detail.hpp

@@ -2,20 +2,9 @@
 #define MBGL_UTIL_UV_DETAIL
 
 #include <uv.h>
+
 #include <functional>
 #include <cassert>
-
-#ifdef __clang__
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wunused-parameter"
-#endif
-
-#include <boost/lockfree/queue.hpp>
-
-#ifdef __clang__
-#pragma clang diagnostic pop
-#endif
-
 #include <string>
 
 

src/map/map.cpp

@@ -34,7 +34,13 @@ using namespace mbgl;
 Map::Map(View& view)
     : loop(std::make_shared<uv::loop>()),
       thread(std::make_unique<uv::thread>()),
+      async_terminate(new uv_async_t()),
+      async_render(new uv_async_t()),
+      async_cleanup(new uv_async_t()),
       view(view),
+#ifndef NDEBUG
+      main_thread(uv_thread_self()),
+#endif
       transform(view),
       glyphAtlas(std::make_shared<GlyphAtlas>(1024, 1024)),
       spriteAtlas(std::make_shared<SpriteAtlas>(512, 512)),
@@ -50,52 +56,52 @@ Map::Map(View& view)
 }
 
 Map::~Map() {
-    // Clear the style first before the rest of the constructor deletes members of this object.
-    // This is required because members of the style reference the Map object in their destructors.
-    style.reset();
-
     if (async) {
         stop();
     }
 }
 
 void Map::start() {
+    assert(uv_thread_self() == main_thread);
+    assert(!async);
+
     // When starting map rendering in another thread, we perform async/continuously
     // updated rendering. Only in these cases, we attach the async handlers.
     async = true;
 
     // Setup async notifications
-    async_terminate = new uv_async_t();
-    uv_async_init(**loop, async_terminate, terminate);
+    uv_async_init(**loop, async_terminate.get(), terminate);
     async_terminate->data = this;
 
-    async_render = new uv_async_t();
-    uv_async_init(**loop, async_render, render);
+    uv_async_init(**loop, async_render.get(), render);
     async_render->data = this;
 
-    async_cleanup = new uv_async_t();
-    uv_async_init(**loop, async_cleanup, cleanup);
+    uv_async_init(**loop, async_cleanup.get(), cleanup);
     async_cleanup->data = this;
 
     uv_thread_create(*thread, [](void *arg) {
         Map *map = static_cast<Map *>(arg);
+#ifndef NDEBUG
+        map->map_thread = uv_thread_self();
+#endif
 #ifdef __APPLE__
         pthread_setname_np("Map");
 #endif
         map->run();
+#ifndef NDEBUG
+        map->map_thread = -1;
+#endif
     }, this);
 }
 
 void Map::stop() {
-    if (async_terminate != nullptr) {
-        uv_async_send(async_terminate);
-    }
+    assert(uv_thread_self() == main_thread);
+    assert(main_thread != map_thread);
+
+    uv_async_send(async_terminate.get());
 
     uv_thread_join(*thread);
 
-    // Run the event loop once to make sure our async delete handlers are called.
-    uv_run(**loop, UV_RUN_ONCE);
-
     async = false;
 }
 
@@ -104,23 +110,36 @@ void Map::delete_async(uv_handle_t *handle) {
 }
 
 void Map::run() {
+#ifndef NDEBUG
+    if (!async) {
+        map_thread = main_thread;
+    }
+#endif
+    assert(uv_thread_self() == map_thread);
+
     setup();
     prepare();
     uv_run(**loop, UV_RUN_DEFAULT);
 
+    // Run the event loop once more to make sure our async delete handlers are called.
+    uv_run(**loop, UV_RUN_ONCE);
+
     // If the map rendering wasn't started asynchronously, we perform one render
     // *after* all events have been processed.
     if (!async) {
         prepare();
         render();
+#ifndef NDEBUG
+        map_thread = -1;
+#endif
     }
 }
 
 void Map::rerender() {
     // We only send render events if we want to continuously update the map
     // (== async rendering).
-    if (async && async_render != nullptr) {
-        uv_async_send(async_render);
+    if (async) {
+        uv_async_send(async_render.get());
     }
 }
 
@@ -140,7 +159,7 @@ void Map::swapped() {
 
 void Map::cleanup() {
     if (async_cleanup != nullptr) {
-        uv_async_send(async_cleanup);
+        uv_async_send(async_cleanup.get());
     }
 }
 
@@ -153,7 +172,7 @@ void Map::cleanup(uv_async_t *async) {
 
 void Map::render(uv_async_t *async) {
     Map *map = static_cast<Map *>(async->data);
-
+    assert(uv_thread_self() == map->map_thread);
 
     if (map->state.hasSize()) {
         if (map->is_rendered.test_and_set() == false) {
@@ -174,18 +193,23 @@ void Map::render(uv_async_t *async) {
 void Map::terminate(uv_async_t *async) {
     // Closes all open handles on the loop. This means that the loop will automatically terminate.
     Map *map = static_cast<Map *>(async->data);
+    assert(uv_thread_self() == map->map_thread);
+
+    // Remove all of these to make sure they are destructed in the correct thread.
     map->glyphStore.reset();
     map->fileSource.reset();
-    uv_walk(**map->loop, [](uv_handle_t *handle, void */*arg*/) {
-        if (!uv_is_closing(handle)) {
-            uv_close(handle, NULL);
-        }
-    }, NULL);
+    map->style.reset();
+    map->activeSources.clear();
+
+    uv_close((uv_handle_t *)map->async_cleanup.get(), nullptr);
+    uv_close((uv_handle_t *)map->async_render.get(), nullptr);
+    uv_close((uv_handle_t *)map->async_terminate.get(), nullptr);
 }
 
 #pragma mark - Setup
 
 void Map::setup() {
+    assert(uv_thread_self() == map_thread);
     view.make_active();
 
     painter.setup();
@@ -201,6 +225,7 @@ void Map::setStyleJSON(std::string newStyleJSON, const std::string &base) {
     // TODO: Make threadsafe.
     styleJSON.swap(newStyleJSON);
     sprite.reset();
+    assert(style);
     style->loadJSON((const uint8_t *)styleJSON.c_str());
     fileSource->setBase(base);
     glyphStore->setURL(util::mapbox::normalizeGlyphsURL(style->glyph_url, getAccessToken()));
@@ -450,6 +475,8 @@ void Map::setDefaultTransitionDuration(uint64_t duration_milliseconds) {
 }
 
 void Map::updateSources() {
+    assert(uv_thread_self() == map_thread);
+
     // First, disable all existing sources.
     for (const std::shared_ptr<StyleSource> &source : activeSources) {
         source->enabled = false;

src/map/tile_parser.cpp

@@ -38,19 +38,26 @@ namespace mbgl {
 TileParser::~TileParser() = default;
 
 TileParser::TileParser(const std::string &data, VectorTileData &tile,
-                       const std::shared_ptr<const Style> &style,
-                       const std::shared_ptr<GlyphAtlas> &glyphAtlas,
-                       const std::shared_ptr<GlyphStore> &glyphStore,
-                       const std::shared_ptr<SpriteAtlas> &spriteAtlas,
-                       const std::shared_ptr<Sprite> &sprite)
+                       const std::shared_ptr<const Style> &style_,
+                       const std::shared_ptr<GlyphAtlas> &glyphAtlas_,
+                       const std::shared_ptr<GlyphStore> &glyphStore_,
+                       const std::shared_ptr<SpriteAtlas> &spriteAtlas_,
+                       const std::shared_ptr<Sprite> &sprite_)
     : vector_data(pbf((const uint8_t *)data.data(), data.size())),
       tile(tile),
-      style(style),
-      glyphAtlas(glyphAtlas),
-      glyphStore(glyphStore),
-      spriteAtlas(spriteAtlas),
-      sprite(sprite),
+      style(style_),
+      glyphAtlas(glyphAtlas_),
+      glyphStore(glyphStore_),
+      spriteAtlas(spriteAtlas_),
+      sprite(sprite_),
       collision(std::make_unique<Collision>(tile.id.z, 4096, tile.source.tile_size, tile.depth)) {
+    assert(&tile != nullptr);
+    assert(style);
+    assert(glyphAtlas);
+    assert(glyphStore);
+    assert(spriteAtlas);
+    assert(sprite);
+    assert(collision);
 }
 
 void TileParser::parse() {

src/storage/base_request.cpp

@@ -8,7 +8,7 @@
 
 namespace mbgl {
 
-BaseRequest::BaseRequest() : thread_id(uv_thread_self()) {
+BaseRequest::BaseRequest(const std::string &path_) : thread_id(uv_thread_self()), path(path_) {
 }
 
 BaseRequest::~BaseRequest() {
@@ -32,6 +32,7 @@ void BaseRequest::notify() {
     if (response) {
         // Now that we have our private copy, notify all observers.
         for (std::unique_ptr<Callback> &callback : list) {
+            assert(callback);
             (*callback)(*response);
         }
     }

src/storage/file_request.cpp

@@ -5,6 +5,8 @@
 
 #include <cassert>
 
+#include <unistd.h>
+
 namespace mbgl {
 
 struct FileRequestBaton {
@@ -161,7 +163,7 @@ void FileRequestBaton::cleanup(uv_fs_t *req) {
 }
 
 FileRequest::FileRequest(const std::string &path_, uv_loop_t *loop)
-    : BaseRequest(), path(path_), thread_id(uv_thread_self()), ptr(new FileRequestBaton(this, path, loop)) {
+    : BaseRequest(path_), ptr(new FileRequestBaton(this, path, loop)) {
 }
 
 FileRequest::~FileRequest() {

src/storage/file_source.cpp

@@ -13,6 +13,7 @@ FileSource::FileSource(uv_loop_t *loop_)
       store(std::shared_ptr<SQLiteStore>(new SQLiteStore(loop_, "cache.db"))),
       loop(loop_),
       queue(new uv_messenger_t) {
+
     uv_messenger_init(loop, queue, [](void *ptr) {
         std::unique_ptr<std::function<void()>> fn { reinterpret_cast<std::function<void()> *>(ptr) };
         (*fn)();
@@ -22,6 +23,8 @@ FileSource::FileSource(uv_loop_t *loop_)
 FileSource::~FileSource() {
     assert(thread_id == uv_thread_self());
     uv_messenger_stop(queue);
+    // NOTE: We don't need to delete the messenger since it will be deleted by the
+    // uv_messenger_stop() function.
 }
 
 void FileSource::setBase(const std::string &value) {

src/storage/http_request.cpp

@@ -22,7 +22,7 @@ struct CacheRequestBaton {
 };
 
 HTTPRequest::HTTPRequest(ResourceType type_, const std::string &path, uv_loop_t *loop, std::shared_ptr<SQLiteStore> store_)
-    : BaseRequest(), thread_id(uv_thread_self()), store(store_), type(type_) {
+    : BaseRequest(path), thread_id(uv_thread_self()), store(store_), type(type_) {
     cache_baton = new CacheRequestBaton;
     cache_baton->request = this;
     cache_baton->path = path;