From 9781785ab73e8394e8b92625cc4741952f47955d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Wed, 4 Mar 2015 12:32:14 +0100 Subject: scope Requests to an Environment object for easier cancelation we are now scoping all file requests to an environment object. The FileSource implementation treats this as an opaque pointer, but allows canceling all Requests that are associated with that pointer. This is necessary to abort all file requests that originated from a particular Map object. Aborting a file request is different from canceling a file request: A canceled request doesn't have its callback called, while an aborted request will have its callback called with an error, indicating that the environment is going to be shut down. --- .clang-format | 2 +- gyp/http-curl.gypi | 43 ++++++++++----- gyp/http-nsurl.gypi | 10 ++++ include/mbgl/map/map.hpp | 14 ++--- include/mbgl/storage/default/request.hpp | 8 ++- .../mbgl/storage/default/shared_request_base.hpp | 35 ++++++++---- include/mbgl/storage/default_file_source.hpp | 17 +++--- include/mbgl/storage/file_source.hpp | 12 ++++- platform/darwin/http_request_nsurl.mm | 3 ++ src/mbgl/map/environment.cpp | 39 ++++++++++++++ src/mbgl/map/environment.hpp | 44 +++++++++++++++ src/mbgl/map/map.cpp | 40 +++++++------- src/mbgl/map/raster_tile_data.cpp | 7 ++- src/mbgl/map/raster_tile_data.hpp | 2 +- src/mbgl/map/source.cpp | 47 ++++++++-------- src/mbgl/map/source.hpp | 22 +++----- src/mbgl/map/sprite.cpp | 22 ++++---- src/mbgl/map/sprite.hpp | 7 +-- src/mbgl/map/tile_data.cpp | 15 +++--- src/mbgl/map/tile_data.hpp | 8 +-- src/mbgl/map/vector_tile_data.cpp | 4 +- src/mbgl/map/vector_tile_data.hpp | 7 +-- src/mbgl/storage/default_file_source.cpp | 62 +++++++++++++++++----- src/mbgl/storage/request.cpp | 4 +- src/mbgl/text/glyph_store.cpp | 15 +++--- src/mbgl/text/glyph_store.hpp | 10 ++-- test/storage/cache_response.cpp | 5 +- test/storage/cache_revalidate.cpp | 17 +++--- test/storage/directory_reading.cpp | 7 ++- test/storage/file_reading.cpp | 17 +++--- test/storage/http_cancel.cpp | 14 +++-- test/storage/http_coalescing.cpp | 4 +- test/storage/http_environment.cpp | 54 +++++++++++++++++++ test/storage/http_error.cpp | 18 +++++-- test/storage/http_header_parsing.cpp | 10 ++-- test/storage/http_load.cpp | 6 ++- test/storage/http_noloop.cpp | 5 +- test/storage/http_other_loop.cpp | 5 +- test/storage/http_reading.cpp | 17 ++++-- test/storage/server.js | 7 +++ test/test.gyp | 1 + 41 files changed, 485 insertions(+), 201 deletions(-) create mode 100644 src/mbgl/map/environment.cpp create mode 100644 src/mbgl/map/environment.hpp create mode 100644 test/storage/http_environment.cpp diff --git a/.clang-format b/.clang-format index 86d562e74e..109b562b59 100644 --- a/.clang-format +++ b/.clang-format @@ -2,7 +2,7 @@ Standard: Cpp11 IndentWidth: 4 AccessModifierOffset: -4 UseTab: Never -BinPackParameters: false +BinPackParameters: true AllowShortIfStatementsOnASingleLine: false AllowShortLoopsOnASingleLine: false AllowShortBlocksOnASingleLine: false diff --git a/gyp/http-curl.gypi b/gyp/http-curl.gypi index bfb2054d55..c97ad370b5 100644 --- a/gyp/http-curl.gypi +++ b/gyp/http-curl.gypi @@ -14,6 +14,25 @@ '../include', ], + 'variables': { + 'cflags_cc': [ + '<@(uv_cflags)', + '<@(curl_cflags)', + '<@(boost_cflags)', + ], + 'ldflags': [ + '<@(uv_ldflags)', + '<@(curl_ldflags)', + ], + 'libraries': [ + '<@(uv_static_libs)', + '<@(curl_static_libs)', + ], + 'defines': [ + '-DMBGL_HTTP_CURL' + ], + }, + 'conditions': [ ['host == "android"', { 'variables': { @@ -32,19 +51,17 @@ }], ], - 'variables': { - 'cflags_cc': [ - '<@(uv_cflags)', - '<@(curl_cflags)', - '<@(boost_cflags)', - ], - 'ldflags': [ - '<@(uv_ldflags)', - '<@(curl_ldflags)', - ], - 'libraries': [ - '<@(uv_static_libs)', - '<@(curl_static_libs)', + 'direct_dependent_settings': { + 'conditions': [ + ['OS == "mac"', { + 'xcode_settings': { + 'OTHER_CFLAGS': [ '<@(defines)' ], + 'OTHER_CPLUSPLUSFLAGS': [ '<@(defines)' ], + } + }, { + 'cflags': [ '<@(defines)' ], + 'cflags_cc': [ '<@(defines)' ], + }] ], }, diff --git a/gyp/http-nsurl.gypi b/gyp/http-nsurl.gypi index 4205f59d81..5a079fdeeb 100644 --- a/gyp/http-nsurl.gypi +++ b/gyp/http-nsurl.gypi @@ -25,6 +25,9 @@ 'libraries': [ '<@(uv_static_libs)', ], + 'defines': [ + '-DMBGL_HTTP_NSURL' + ], }, 'xcode_settings': { @@ -32,6 +35,13 @@ 'CLANG_ENABLE_OBJC_ARC': 'NO', }, + 'direct_dependent_settings': { + 'xcode_settings': { + 'OTHER_CFLAGS': [ '<@(defines)' ], + 'OTHER_CPLUSPLUSFLAGS': [ '<@(defines)' ], + } + }, + 'link_settings': { 'libraries': [ '<@(libraries)' ], 'xcode_settings': { diff --git a/include/mbgl/map/map.hpp b/include/mbgl/map/map.hpp index 8711aa3d08..00719aa382 100644 --- a/include/mbgl/map/map.hpp +++ b/include/mbgl/map/map.hpp @@ -36,11 +36,7 @@ class View; class GlyphAtlas; class SpriteAtlas; class LineAtlas; - -struct exception : std::runtime_error { - inline exception(const char *msg) : std::runtime_error(msg) { - } -}; +class Environment; class Map : private util::noncopyable { friend class View; @@ -182,8 +178,8 @@ private: Mode mode = Mode::None; -public: // TODO: make private again - std::unique_ptr loop; + const std::unique_ptr env; + View &view; private: std::unique_ptr workers; @@ -214,12 +210,8 @@ private: // Stores whether the map thread has been stopped already. std::atomic_bool isStopped; - View &view; - -#ifdef DEBUG const std::thread::id mainThread; std::thread::id mapThread; -#endif Transform transform; TransformState state; diff --git a/include/mbgl/storage/default/request.hpp b/include/mbgl/storage/default/request.hpp index 648585f304..b686d1fe90 100644 --- a/include/mbgl/storage/default/request.hpp +++ b/include/mbgl/storage/default/request.hpp @@ -15,13 +15,14 @@ typedef struct uv_loop_s uv_loop_t; namespace mbgl { class Response; +class Environment; class Request : private util::noncopyable { MBGL_STORE_THREAD(tid) public: using Callback = std::function; - Request(const Resource &resource, uv_loop_t *loop, Callback callback); + Request(const Resource &resource, uv_loop_t *loop, const Environment &env, Callback callback); public: // May be called from any thread. @@ -45,6 +46,11 @@ private: public: const Resource resource; + + // The environment ref is used to associate requests with a particular environment. This allows + // us to only terminate requests associated with that environment, e.g. when the map the env + // belongs to is discarded. + const Environment &env; }; } diff --git a/include/mbgl/storage/default/shared_request_base.hpp b/include/mbgl/storage/default/shared_request_base.hpp index 2d56615608..59e38efc2f 100644 --- a/include/mbgl/storage/default/shared_request_base.hpp +++ b/include/mbgl/storage/default/shared_request_base.hpp @@ -4,11 +4,13 @@ #include #include #include +#include #include #include #include #include +#include #include typedef struct uv_loop_s uv_loop_t; @@ -45,20 +47,12 @@ public: observers.insert(request); } - void unsubscribeAll() { - MBGL_VERIFY_THREAD(tid); - - source = nullptr; - observers.clear(); - cancel(); - } - void unsubscribe(Request *request) { MBGL_VERIFY_THREAD(tid); observers.erase(request); - if (observers.empty()) { + if (abandoned()) { // There are no observers anymore. We are initiating cancelation. if (source) { // First, remove this SharedRequestBase from the source. @@ -70,6 +64,29 @@ public: } } + bool abandoned() const { + return observers.empty(); + } + + std::vector removeAllInEnvironment(const Environment &env) { + MBGL_VERIFY_THREAD(tid); + + std::vector result; + + // Removes all Requests in the supplied environment and returns a list + // of them. + util::erase_if(observers, [&](Request *req) -> bool { + if (&req->env == &env) { + result.push_back(req); + return true; + } else { + return false; + } + }); + + return result; + } + protected: virtual ~SharedRequestBase() { MBGL_VERIFY_THREAD(tid); diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp index 86e2414041..14b4db6eff 100644 --- a/include/mbgl/storage/default_file_source.hpp +++ b/include/mbgl/storage/default_file_source.hpp @@ -20,11 +20,14 @@ class DefaultFileSource : public FileSource { public: DefaultFileSource(FileCache *cache, const std::string &root = ""); DefaultFileSource(FileCache *cache, uv_loop_t *loop, const std::string &root = ""); - ~DefaultFileSource(); + ~DefaultFileSource() override; - Request *request(const Resource &resource, uv_loop_t *loop, Callback callback); - void cancel(Request *request); - void request(const Resource &resource, Callback callback); + Request *request(const Resource &resource, uv_loop_t *loop, const Environment &env, + Callback callback) override; + void cancel(Request *request) override; + void request(const Resource &resource, const Environment &env, Callback callback) override; + + void abort(const Environment &env) override; enum class CacheHint : uint8_t { Full, Refresh, No }; void notify(SharedRequestBase *sharedRequest, const std::set &observers, @@ -39,14 +42,16 @@ private: struct RemoveRequestAction; struct ResultAction; struct StopAction; - using Action = - mapbox::util::variant; + struct AbortAction; + using Action = mapbox::util::variant; using Queue = util::AsyncQueue; void process(AddRequestAction &action); void process(RemoveRequestAction &action); void process(ResultAction &action); void process(StopAction &action); + void process(AbortAction &action); SharedRequestBase *find(const Resource &resource); diff --git a/include/mbgl/storage/file_source.hpp b/include/mbgl/storage/file_source.hpp index 8517d6e4a6..30e88c39f6 100644 --- a/include/mbgl/storage/file_source.hpp +++ b/include/mbgl/storage/file_source.hpp @@ -15,6 +15,7 @@ typedef struct uv_loop_s uv_loop_t; namespace mbgl { class Request; +class Environment; class FileSource : private util::noncopyable { protected: @@ -27,12 +28,19 @@ public: // These can be called from any thread. The callback will be invoked in the loop. // You can only cancel a request from the same thread it was created in. - virtual Request *request(const Resource &resource, uv_loop_t *loop, Callback callback) = 0; + virtual Request *request(const Resource &resource, uv_loop_t *loop, const Environment &env, + Callback callback) = 0; virtual void cancel(Request *request) = 0; // These can be called from any thread. The callback will be invoked in an arbitrary other thread. // You cannot cancel these requests. - virtual void request(const Resource &resource, Callback callback) = 0; + virtual void request(const Resource &resource, const Environment &env, Callback callback) = 0; + + // This can be called from any thread. All requests with the environment pointer env should be + // notified as errored. Note that this is /different/ from canceling requests; a canceled + // request's callback is never called, while an aborted request's callback is called with + // a error message. + virtual void abort(const Environment &env) = 0; }; } diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index 83c010f8b8..638b5062d4 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -188,6 +188,7 @@ void HTTPRequestImpl::handleResponse() { } context->removeRequest(request); + request->ptr = nullptr; delete request; request = nullptr; } @@ -206,6 +207,8 @@ void HTTPRequestImpl::cancel() { [task cancel]; [task release]; task = nullptr; + } else { + delete this; } } diff --git a/src/mbgl/map/environment.cpp b/src/mbgl/map/environment.cpp new file mode 100644 index 0000000000..ee13d33ea7 --- /dev/null +++ b/src/mbgl/map/environment.cpp @@ -0,0 +1,39 @@ +#include +#include + +#include + +#include + +namespace mbgl { + +Environment::Environment(FileSource &fs) : fileSource(fs), loop(uv_loop_new()) { +} + +void Environment::setup() { + mapThread = std::this_thread::get_id(); +} + +bool Environment::inMapThread() const { + return std::this_thread::get_id() == mapThread; +} + +void Environment::requestAsync(const Resource &resource, std::function callback) { + fileSource.request(resource, *this, std::move(callback)); +} + +Request *Environment::request(const Resource &resource, std::function callback) { + assert(inMapThread()); + return fileSource.request(resource, loop, *this, std::move(callback)); +} + +void Environment::cancelRequest(Request *req) { + assert(inMapThread()); + fileSource.cancel(req); +} + +void Environment::terminate() { + fileSource.abort(*this); +} + +} diff --git a/src/mbgl/map/environment.hpp b/src/mbgl/map/environment.hpp new file mode 100644 index 0000000000..b68a7b9e2f --- /dev/null +++ b/src/mbgl/map/environment.hpp @@ -0,0 +1,44 @@ +#ifndef MBGL_MAP_MAP_ENVIRONMENT +#define MBGL_MAP_MAP_ENVIRONMENT + +#include +#include + +#include +#include + +typedef struct uv_loop_s uv_loop_t; + +namespace mbgl { + +class FileSource; +class Request; +class Response; +struct Resource; + +class Environment : private util::noncopyable { +public: + Environment(FileSource &); + + void setup(); + + bool inMapThread() const; + + void requestAsync(const Resource &, std::function); + Request *request(const Resource &, std::function); + void cancelRequest(Request *); + + // Request to terminate the environment. + void terminate(); + +private: + FileSource &fileSource; + std::thread::id mapThread; + +public: + uv_loop_t *const loop; +}; + +} + +#endif diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index 518bc5bb96..0cd7d3621e 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -25,6 +26,7 @@ #include #include #include +#include #include #include @@ -57,16 +59,14 @@ const static bool uvVersionCheck = []() { using namespace mbgl; Map::Map(View& view_, FileSource& fileSource_) - : loop(util::make_unique()), + : env(util::make_unique(fileSource_)), view(view_), -#ifdef DEBUG mainThread(std::this_thread::get_id()), mapThread(mainThread), -#endif transform(view_), fileSource(fileSource_), glyphAtlas(util::make_unique(1024, 1024)), - glyphStore(std::make_shared(fileSource)), + glyphStore(std::make_shared(*env)), spriteAtlas(util::make_unique(512, 512)), lineAtlas(util::make_unique(512, 512)), texturePool(std::make_shared()), @@ -92,7 +92,7 @@ Map::~Map() { texturePool.reset(); workers.reset(); - uv_run(**loop, UV_RUN_DEFAULT); + uv_run(env->loop, UV_RUN_DEFAULT); } uv::worker &Map::getWorker() { @@ -112,7 +112,7 @@ void Map::start(bool startPaused) { isStopped = false; // Setup async notifications - asyncTerminate = util::make_unique(**loop, [this]() { + asyncTerminate = util::make_unique(env->loop, [this]() { assert(std::this_thread::get_id() == mapThread); // Remove all of these to make sure they are destructed in the correct thread. @@ -127,7 +127,7 @@ void Map::start(bool startPaused) { asyncTerminate.reset(); }); - asyncRender = util::make_unique(**loop, [this]() { + asyncRender = util::make_unique(env->loop, [this]() { assert(std::this_thread::get_id() == mapThread); if (state.hasSize()) { @@ -207,7 +207,7 @@ void Map::pause(bool waitForPause) { pausing = true; mutexRun.unlock(); - uv_stop(**loop); + uv_stop(env->loop); rerender(); // Needed to ensure uv_stop is seen and uv_run exits, otherwise we deadlock on wait_for_pause if (waitForPause) { @@ -242,12 +242,14 @@ void Map::run() { } if (mode == Mode::Static && !style && styleURL.empty()) { - throw exception("Style is not set"); + throw util::Exception("Style is not set"); } view.activate(); - workers = util::make_unique(**loop, 4, "Tile Worker"); + workers = util::make_unique(env->loop, 4, "Tile Worker"); + + env->setup(); setup(); prepare(); @@ -255,15 +257,15 @@ void Map::run() { if (mode == Mode::Continuous) { terminating = false; while(!terminating) { - uv_run(**loop, UV_RUN_DEFAULT); + uv_run(env->loop, UV_RUN_DEFAULT); checkForPause(); } } else { - uv_run(**loop, UV_RUN_DEFAULT); + uv_run(env->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); + uv_run(env->loop, UV_RUN_ONCE); // If the map rendering wasn't started asynchronously, we perform one render // *after* all events have been processed. @@ -376,7 +378,7 @@ util::ptr Map::getSprite() { const float pixelRatio = state.getPixelRatio(); const std::string &sprite_url = style->getSpriteURL(); if (!sprite || sprite->pixelRatio != pixelRatio) { - sprite = Sprite::Create(sprite_url, pixelRatio, fileSource); + sprite = Sprite::Create(sprite_url, pixelRatio, *env); } return sprite; @@ -621,7 +623,7 @@ void Map::updateSources() { if (source->enabled) { if (!source->source) { source->source = std::make_shared(source->info); - source->source->load(*this, fileSource); + source->source->load(*this, *env); } } else { source->source.reset(); @@ -648,10 +650,8 @@ void Map::updateSources(const util::ptr &group) { void Map::updateTiles() { for (const auto& source : activeSources) { - source->source->update(*this, getWorker(), - style, *glyphAtlas, *glyphStore, - *spriteAtlas, getSprite(), - *texturePool, fileSource, ***loop, [this](){ update(); }); + source->source->update(*this, *env, getWorker(), style, *glyphAtlas, *glyphStore, + *spriteAtlas, getSprite(), *texturePool, [this]() { update(); }); } } @@ -659,7 +659,7 @@ void Map::prepare() { if (!style) { style = std::make_shared