From 6c526e56370a0f4dee9a4b6ed45489fcb14e3ff1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20Ka=CC=88fer?= Date: Tue, 16 Sep 2014 14:48:46 +0200 Subject: [PATCH] make sure all of our async handles are stopped before stopping the map thread --- include/mbgl/map/map.hpp | 11 ++-- include/mbgl/storage/base_request.hpp | 6 ++- include/mbgl/storage/file_request.hpp | 2 - include/mbgl/util/uv_detail.hpp | 13 +---- src/map/map.cpp | 77 ++++++++++++++++++--------- src/map/tile_parser.cpp | 27 ++++++---- src/storage/base_request.cpp | 3 +- src/storage/file_request.cpp | 4 +- src/storage/file_source.cpp | 3 ++ src/storage/http_request.cpp | 2 +- 10 files changed, 92 insertions(+), 56 deletions(-) diff --git a/include/mbgl/map/map.hpp b/include/mbgl/map/map.hpp index 98951a375..3b2c97122 100644 --- a/include/mbgl/map/map.hpp +++ b/include/mbgl/map/map.hpp @@ -160,9 +160,9 @@ private: bool async = false; std::shared_ptr loop; std::unique_ptr thread; - uv_async_t *async_terminate = nullptr; - uv_async_t *async_render = nullptr; - uv_async_t *async_cleanup = nullptr; + std::unique_ptr async_terminate; + std::unique_ptr async_render; + std::unique_ptr 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; diff --git a/include/mbgl/storage/base_request.hpp b/include/mbgl/storage/base_request.hpp index b8ebd368e..05ef1e4a7 100644 --- a/include/mbgl/storage/base_request.hpp +++ b/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 &request); @@ -34,10 +34,14 @@ public: public: const unsigned long thread_id; + const std::string path; std::unique_ptr 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 self; + std::forward_list> callbacks; }; diff --git a/include/mbgl/storage/file_request.hpp b/include/mbgl/storage/file_request.hpp index 156fd6dfe..331ed8421 100644 --- a/include/mbgl/storage/file_request.hpp +++ b/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; diff --git a/include/mbgl/util/uv_detail.hpp b/include/mbgl/util/uv_detail.hpp index c65c247ba..eefb5ac8d 100644 --- a/include/mbgl/util/uv_detail.hpp +++ b/include/mbgl/util/uv_detail.hpp @@ -2,20 +2,9 @@ #define MBGL_UTIL_UV_DETAIL #include + #include #include - -#ifdef __clang__ -#pragma clang diagnostic push -#pragma clang diagnostic ignored "-Wunused-parameter" -#endif - -#include - -#ifdef __clang__ -#pragma clang diagnostic pop -#endif - #include diff --git a/src/map/map.cpp b/src/map/map.cpp index 1f9c1428f..7db75cca0 100644 --- a/src/map/map.cpp +++ b/src/map/map.cpp @@ -34,7 +34,13 @@ using namespace mbgl; Map::Map(View& view) : loop(std::make_shared()), thread(std::make_unique()), + 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(1024, 1024)), spriteAtlas(std::make_shared(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(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(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(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 &source : activeSources) { source->enabled = false; diff --git a/src/map/tile_parser.cpp b/src/map/tile_parser.cpp index 6923c1a42..8fbcfa45d 100644 --- a/src/map/tile_parser.cpp +++ b/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 &style, - const std::shared_ptr &glyphAtlas, - const std::shared_ptr &glyphStore, - const std::shared_ptr &spriteAtlas, - const std::shared_ptr &sprite) + const std::shared_ptr &style_, + const std::shared_ptr &glyphAtlas_, + const std::shared_ptr &glyphStore_, + const std::shared_ptr &spriteAtlas_, + const std::shared_ptr &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(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() { diff --git a/src/storage/base_request.cpp b/src/storage/base_request.cpp index fb476e098..76d7fe648 100644 --- a/src/storage/base_request.cpp +++ b/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 : list) { + assert(callback); (*callback)(*response); } } diff --git a/src/storage/file_request.cpp b/src/storage/file_request.cpp index ccbd896b9..3fe0db0c2 100644 --- a/src/storage/file_request.cpp +++ b/src/storage/file_request.cpp @@ -5,6 +5,8 @@ #include +#include + 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() { diff --git a/src/storage/file_source.cpp b/src/storage/file_source.cpp index 1ee902aa7..43c51def7 100644 --- a/src/storage/file_source.cpp +++ b/src/storage/file_source.cpp @@ -13,6 +13,7 @@ FileSource::FileSource(uv_loop_t *loop_) store(std::shared_ptr(new SQLiteStore(loop_, "cache.db"))), loop(loop_), queue(new uv_messenger_t) { + uv_messenger_init(loop, queue, [](void *ptr) { std::unique_ptr> fn { reinterpret_cast *>(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) { diff --git a/src/storage/http_request.cpp b/src/storage/http_request.cpp index 56eef9268..966356a61 100644 --- a/src/storage/http_request.cpp +++ b/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 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;