From 19881f8b24c94ef66cc3ee72eebc73eeb37af2af Mon Sep 17 00:00:00 2001 From: "Thiago Marcos P. Santos" Date: Tue, 28 Apr 2015 21:56:42 +0300 Subject: [PATCH] Cancel all pending requests before leaving This still has the unwanted side effect off blocking the Map thread waiting for the workers to return (because they hold a reference to the Map thread main loop). Maybe the only way to solve this would be having a I/O thread for dispatching messages. The Request is not bound anymore to the invoker thread because the workers will request for glyphs, but the reply will arrive at the Map thread. --- include/mbgl/storage/request.hpp | 2 -- src/mbgl/map/environment.cpp | 1 - src/mbgl/map/resource_loader.cpp | 3 +-- src/mbgl/map/source.cpp | 20 ++++++++++++++------ src/mbgl/map/source.hpp | 6 ++++-- src/mbgl/storage/request.cpp | 3 +-- src/mbgl/text/glyph_store.cpp | 14 +++++++++++--- src/mbgl/text/glyph_store.hpp | 6 ++++++ 8 files changed, 37 insertions(+), 18 deletions(-) diff --git a/include/mbgl/storage/request.hpp b/include/mbgl/storage/request.hpp index a01808ce8..5d979a281 100644 --- a/include/mbgl/storage/request.hpp +++ b/include/mbgl/storage/request.hpp @@ -19,8 +19,6 @@ namespace mbgl { class Response; class Request : private util::noncopyable { - MBGL_STORE_THREAD(tid) - public: using Callback = std::function; Request(const Resource &resource, uv_loop_t *loop, Callback callback); diff --git a/src/mbgl/map/environment.cpp b/src/mbgl/map/environment.cpp index dd6c1f193..5857d4bdf 100644 --- a/src/mbgl/map/environment.cpp +++ b/src/mbgl/map/environment.cpp @@ -128,7 +128,6 @@ void Environment::requestAsync(const Resource& resource, Request* Environment::request(const Resource& resource, std::function callback) { - assert(currentlyOn(ThreadType::Map)); return fileSource.request(resource, util::RunLoop::current.get()->get(), std::move(callback)); } diff --git a/src/mbgl/map/resource_loader.cpp b/src/mbgl/map/resource_loader.cpp index 5dc0ab9d6..b3aa35b74 100644 --- a/src/mbgl/map/resource_loader.cpp +++ b/src/mbgl/map/resource_loader.cpp @@ -37,10 +37,9 @@ void ResourceLoader::setStyle(Style* style) { style_ = style; - Environment& env = Environment::Get(); for (const auto& source : style->sources) { source->setObserver(this); - source->load(accessToken_, env); + source->load(accessToken_); } } diff --git a/src/mbgl/map/source.cpp b/src/mbgl/map/source.cpp index 9b6631da9..8b0bf905f 100644 --- a/src/mbgl/map/source.cpp +++ b/src/mbgl/map/source.cpp @@ -113,11 +113,13 @@ std::string SourceInfo::tileURL(const TileID& id, float pixelRatio) const { return result; } -Source::Source() -{ -} +Source::Source() {} -Source::~Source() {} +Source::~Source() { + if (req) { + Environment::Get().cancelRequest(req); + } +} bool Source::isLoaded() const { if (!loaded) { @@ -136,7 +138,7 @@ bool Source::isLoaded() const { // Note: This is a separate function that must be called exactly once after creation // The reason this isn't part of the constructor is that calling shared_from_this() in // the constructor fails. -void Source::load(const std::string& accessToken, Environment& env) { +void Source::load(const std::string& accessToken) { if (info.url.empty()) { loaded = true; return; @@ -145,7 +147,9 @@ void Source::load(const std::string& accessToken, Environment& env) { util::ptr source = shared_from_this(); const std::string url = util::mapbox::normalizeSourceURL(info.url, accessToken); - env.request({ Resource::Kind::JSON, url }, [source](const Response &res) { + req = Environment::Get().request({ Resource::Kind::JSON, url }, [source](const Response &res) { + source->clearRequest(); + if (res.status != Response::Successful) { Log::Warning(Event::General, "Failed to load source TileJSON: %s", res.message.c_str()); return; @@ -505,6 +509,10 @@ void Source::onLowMemory() { cache.clear(); } +void Source::clearRequest() { + req = nullptr; +} + void Source::setObserver(Observer* observer) { observer_ = observer; } diff --git a/src/mbgl/map/source.hpp b/src/mbgl/map/source.hpp index dd02cdc3b..eb7950225 100644 --- a/src/mbgl/map/source.hpp +++ b/src/mbgl/map/source.hpp @@ -29,6 +29,7 @@ class Sprite; class TexturePool; class Style; class Painter; +class Request; class TransformState; class Tile; struct ClipID; @@ -63,7 +64,7 @@ public: Source(); ~Source(); - void load(const std::string& accessToken, Environment&); + void load(const std::string& accessToken); bool isLoaded() const; void load(MapData&, Environment&, std::function callback); @@ -87,6 +88,7 @@ public: void setCacheSize(size_t); void onLowMemory(); + void clearRequest(); void setObserver(Observer* observer); @@ -128,7 +130,7 @@ private: std::map> tile_data; TileCache cache; - + Request* req = nullptr; Observer* observer_ = nullptr; }; diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index d413cabbe..53a882f2a 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -1,5 +1,6 @@ #include +#include #include #include @@ -33,7 +34,6 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_) // Called in the originating thread. void Request::notifyCallback() { - MBGL_VERIFY_THREAD(tid) if (!canceled) { invoke(); } else { @@ -84,7 +84,6 @@ void Request::notify(const std::shared_ptr &response_) { // Called in the originating thread. void Request::cancel() { - MBGL_VERIFY_THREAD(tid) assert(async); assert(!canceled); canceled = util::make_unique(); diff --git a/src/mbgl/text/glyph_store.cpp b/src/mbgl/text/glyph_store.cpp index 80af0db98..71c1be6d9 100644 --- a/src/mbgl/text/glyph_store.cpp +++ b/src/mbgl/text/glyph_store.cpp @@ -142,9 +142,9 @@ void FontStack::lineWrap(Shaping &shaping, const float lineHeight, const float m GlyphPBF::GlyphPBF(const std::string& glyphURL, const std::string& fontStack, GlyphRange glyphRange, - Environment& env, + Environment& env_, const GlyphLoadedCallback& callback) - : parsed(false) { + : parsed(false), env(env_) { // Load the glyph set URL std::string url = util::replaceTokens(glyphURL, [&](const std::string &name) -> std::string { if (name == "fontstack") return util::percentEncode(fontStack); @@ -153,7 +153,9 @@ GlyphPBF::GlyphPBF(const std::string& glyphURL, }); // The prepare call jumps back to the main thread. - env.requestAsync({ Resource::Kind::Glyphs, url }, [&, url, callback](const Response &res) { + req = env.request({ Resource::Kind::Glyphs, url }, [&, url, callback](const Response &res) { + req = nullptr; + if (res.status != Response::Successful) { // Something went wrong with loading the glyph pbf. const std::string msg = std::string { "[ERROR] failed to load glyphs: " } + url + " message: " + res.message; @@ -170,6 +172,12 @@ GlyphPBF::GlyphPBF(const std::string& glyphURL, }); } +GlyphPBF::~GlyphPBF() { + if (req) { + env.cancelRequest(req); + } +} + void GlyphPBF::parse(FontStack &stack) { std::lock_guard lock(mtx); diff --git a/src/mbgl/text/glyph_store.hpp b/src/mbgl/text/glyph_store.hpp index 6b7e2b8bd..a41eec533 100644 --- a/src/mbgl/text/glyph_store.hpp +++ b/src/mbgl/text/glyph_store.hpp @@ -17,6 +17,7 @@ namespace mbgl { class FileSource; class Environment; +class Request; class SDFGlyph { public: @@ -56,6 +57,7 @@ public: GlyphRange glyphRange, Environment &env, const GlyphLoadedCallback& callback); + ~GlyphPBF(); void parse(FontStack &stack); bool isParsed() const; @@ -68,6 +70,10 @@ private: std::string data; std::atomic_bool parsed; + + Environment& env; + Request* req = nullptr; + mutable std::mutex mtx; };