From bba3d9838a512c75a63f5eb9263913ec96011e8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 10 Apr 2015 15:36:32 +0200 Subject: [PATCH 01/13] add Thread and RunLoop Thread is a generic thread management object that starts a thread, creates an object T in that thread and calls its .start() function. When the Thread object is destructed (which must happen in the thread that created it), it'll call the object's .stop() function, and wait for thread termination. The .stop() function must somehow get the thread to terminate. Note that object T's constructor/destructor/start()/stop() must be protected/private, and Thread must be declared as a friend class. All public functions may be called from any thread and are exposed through operator->(). RunLoop encapsulates a libuv runloop and has a facility of dispatching function objects to the thread. It can be used either as a worker thread class by itself, or it can be derived from. This commit converts SQLiteCache to derive from RunLoop and removes the custom code used previously for managing the cache thread. --- android/cpp/native_map_view.cpp | 2 +- bin/render.cpp | 5 +- include/mbgl/android/native_map_view.hpp | 3 +- include/mbgl/storage/default/sqlite_cache.hpp | 42 ++--- include/mbgl/storage/file_cache.hpp | 4 +- include/mbgl/util/run_loop.hpp | 52 ++++++ include/mbgl/util/thread.hpp | 52 ++++++ linux/main.cpp | 5 +- macosx/main.mm | 5 +- platform/default/sqlite_cache.cpp | 157 ++++++------------ platform/ios/MGLMapView.mm | 7 +- src/mbgl/storage/default_file_source.cpp | 6 +- src/mbgl/util/run_loop.cpp | 55 ++++++ src/mbgl/util/uv_detail.hpp | 21 ++- test/storage/cache_response.cpp | 5 +- test/storage/cache_revalidate.cpp | 5 +- test/storage/database.cpp | 35 ++-- 17 files changed, 280 insertions(+), 181 deletions(-) create mode 100644 include/mbgl/util/run_loop.hpp create mode 100644 include/mbgl/util/thread.hpp create mode 100644 src/mbgl/util/run_loop.cpp diff --git a/android/cpp/native_map_view.cpp b/android/cpp/native_map_view.cpp index 39a777bff..340bbae10 100644 --- a/android/cpp/native_map_view.cpp +++ b/android/cpp/native_map_view.cpp @@ -58,7 +58,7 @@ void log_gl_string(GLenum name, const char *label) { NativeMapView::NativeMapView(JNIEnv *env, jobject obj_) : mbgl::View(*this), fileCache(mbgl::android::cachePath + "/mbgl-cache.db"), - fileSource(&fileCache), + fileSource(fileCache), map(*this, fileSource) { mbgl::Log::Debug(mbgl::Event::Android, "NativeMapView::NativeMapView"); diff --git a/bin/render.cpp b/bin/render.cpp index 01f692909..879bf840a 100644 --- a/bin/render.cpp +++ b/bin/render.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include @@ -66,8 +67,8 @@ int main(int argc, char *argv[]) { using namespace mbgl; - mbgl::SQLiteCache cache(cache_file); - mbgl::DefaultFileSource fileSource(&cache); + mbgl::util::Thread cache(cache_file); + mbgl::DefaultFileSource fileSource(cache); // Try to load the token from the environment. if (!token.size()) { diff --git a/include/mbgl/android/native_map_view.hpp b/include/mbgl/android/native_map_view.hpp index 21784f531..4a8d9365b 100644 --- a/include/mbgl/android/native_map_view.hpp +++ b/include/mbgl/android/native_map_view.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -61,7 +62,7 @@ private: ANativeWindow *window = nullptr; - mbgl::SQLiteCache fileCache; + mbgl::util::Thread fileCache; mbgl::DefaultFileSource fileSource; mbgl::Map map; diff --git a/include/mbgl/storage/default/sqlite_cache.hpp b/include/mbgl/storage/default/sqlite_cache.hpp index fe80a41b5..899cc8a89 100644 --- a/include/mbgl/storage/default/sqlite_cache.hpp +++ b/include/mbgl/storage/default/sqlite_cache.hpp @@ -2,50 +2,40 @@ #define MBGL_STORAGE_DEFAULT_SQLITE_CACHE #include +#include #include -#include -typedef struct uv_loop_s uv_loop_t; - -namespace mapbox { namespace util { template class variant; } } namespace mapbox { namespace sqlite { class Database; class Statement; } } namespace mbgl { -namespace util { template class AsyncQueue; } +class SQLiteCache : public FileCache, protected util::RunLoop { + friend class util::Thread; -class SQLiteCache : public FileCache { - struct GetAction; - struct PutAction; - struct RefreshAction; - struct StopAction; - using Action = mapbox::util::variant; - using Queue = util::AsyncQueue; - -public: +private: SQLiteCache(const std::string &path = ":memory:"); ~SQLiteCache(); - void get(const Resource &resource, std::function)> callback); - void put(const Resource &resource, std::shared_ptr response, Hint hint); +public: + // FileCache API + void get(const Resource &resource, Callback callback) override; + void put(const Resource &resource, std::shared_ptr response, Hint hint) override; private: - struct ActionDispatcher; - void process(GetAction &action); - void process(PutAction &action); - void process(RefreshAction &action); - void process(StopAction &action); - void createDatabase(); void createSchema(); + void processGet(const Resource& resource, Callback callback); + void processPut(const Resource& resource, std::shared_ptr response); + void processRefresh(const Resource& resource, int64_t expires); + +private: const std::string path; - uv_loop_t *loop = nullptr; - Queue *queue = nullptr; - std::thread thread; std::unique_ptr<::mapbox::sqlite::Database> db; - std::unique_ptr<::mapbox::sqlite::Statement> getStmt, putStmt, refreshStmt; + std::unique_ptr<::mapbox::sqlite::Statement> getStmt; + std::unique_ptr<::mapbox::sqlite::Statement> putStmt; + std::unique_ptr<::mapbox::sqlite::Statement> refreshStmt; bool schema = false; }; diff --git a/include/mbgl/storage/file_cache.hpp b/include/mbgl/storage/file_cache.hpp index 97e75a5d8..f815d5b8c 100644 --- a/include/mbgl/storage/file_cache.hpp +++ b/include/mbgl/storage/file_cache.hpp @@ -16,9 +16,9 @@ public: virtual ~FileCache() = default; enum class Hint : uint8_t { Full, Refresh, No }; + using Callback = std::function)>; - virtual void get(const Resource &resource, - std::function)> callback) = 0; + virtual void get(const Resource &resource, Callback callback) = 0; virtual void put(const Resource &resource, std::shared_ptr response, Hint hint) = 0; }; diff --git a/include/mbgl/util/run_loop.hpp b/include/mbgl/util/run_loop.hpp new file mode 100644 index 000000000..f0318d202 --- /dev/null +++ b/include/mbgl/util/run_loop.hpp @@ -0,0 +1,52 @@ +#ifndef MBGL_UTIL_RUN_LOOP +#define MBGL_UTIL_RUN_LOOP + +#include +#include +#include + +namespace uv { +class async; +class loop; +} + +namespace mbgl { +namespace util { + +template class Thread; + +class RunLoop { + friend Thread; + +protected: + // These are called by the Thread<> wrapper. + RunLoop(); + ~RunLoop(); + + // Called by the Thread<> wrapper to start the loop. When you implement this + // method in a child class, you *must* call this function as the last action. + void start(); + +protected: + // Called by the Thread<> wrapper to terminate this loop. + void stop(); + +private: + // Invokes function in the run loop. + void process(); + +public: + // Schedules a function to be executed as part of this run loop. + void invoke(std::function fn); + +private: + const std::unique_ptr runloop; + const std::unique_ptr runloopAsync; + std::mutex runloopMutex; + std::queue> runloopQueue; +}; + +} +} + +#endif \ No newline at end of file diff --git a/include/mbgl/util/thread.hpp b/include/mbgl/util/thread.hpp new file mode 100644 index 000000000..943039c5d --- /dev/null +++ b/include/mbgl/util/thread.hpp @@ -0,0 +1,52 @@ +#ifndef MBGL_UTIL_THREAD +#define MBGL_UTIL_THREAD + +#include +#include + +namespace mbgl { +namespace util { + +template +class Thread { +public: + template + Thread(Args&&... args); + Thread(const Thread&) = delete; + Thread(Thread&&) = delete; + Thread& operator=(const Thread&) = delete; + Thread& operator=(Thread&&) = delete; + ~Thread(); + + inline Object* operator->() const { return &object; } + inline operator Object*() const { return &object; } + +private: + std::thread thread; + Object& object; +}; + +template +template +Thread::Thread(Args&&... args) + : object([&]() -> Object& { + std::promise promise; + thread = std::thread([&] { + Object context(::std::forward(args)...); + promise.set_value(context); + context.start(); + }); + return promise.get_future().get(); + }()) { +} + +template +Thread::~Thread() { + object.stop(); + thread.join(); +} + +} +} + +#endif diff --git a/linux/main.cpp b/linux/main.cpp index 4d5474c02..71c2424af 100644 --- a/linux/main.cpp +++ b/linux/main.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -65,8 +66,8 @@ int main(int argc, char *argv[]) { view = mbgl::util::make_unique(); - mbgl::SQLiteCache cache("/tmp/mbgl-cache.db"); - mbgl::DefaultFileSource fileSource(&cache); + mbgl::util::Thread cache("/tmp/mbgl-cache.db"); + mbgl::DefaultFileSource fileSource(cache); mbgl::Map map(*view, fileSource); // Load settings diff --git a/macosx/main.mm b/macosx/main.mm index 087544f7c..fb12282e2 100644 --- a/macosx/main.mm +++ b/macosx/main.mm @@ -6,6 +6,7 @@ #include #include #include +#include #include @@ -103,8 +104,8 @@ const std::string &defaultCacheDatabase() { int main() { GLFWView view; - mbgl::SQLiteCache cache(defaultCacheDatabase()); - mbgl::DefaultFileSource fileSource(&cache); + mbgl::util::Thread cache(defaultCacheDatabase()); + mbgl::DefaultFileSource fileSource(cache); mbgl::Map map(view, fileSource); URLHandler *handler = [[URLHandler alloc] init]; diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index a3114098c..b084a7f2a 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -2,9 +2,6 @@ #include #include -#include -#include -#include #include #include #include @@ -12,10 +9,6 @@ #include "sqlite3.hpp" #include -#include - -#include - namespace mbgl { std::string removeAccessTokenFromURL(const std::string &url) { @@ -67,90 +60,28 @@ std::string unifyMapboxURLs(const std::string &url) { using namespace mapbox::sqlite; -struct SQLiteCache::GetAction { - const Resource resource; - const std::function)> callback; -}; - -struct SQLiteCache::PutAction { - const Resource resource; - const std::shared_ptr response; -}; - -struct SQLiteCache::RefreshAction { - const Resource resource; - const int64_t expires; -}; - -struct SQLiteCache::StopAction { -}; - -struct SQLiteCache::ActionDispatcher { - SQLiteCache &cache; - template void operator()(T &t) { cache.process(t); } -}; - SQLiteCache::SQLiteCache(const std::string& path_) - : path(path_), - loop(uv_loop_new()), - queue(new Queue(loop, [this](Action& action) { - mapbox::util::apply_visitor(ActionDispatcher{ *this }, action); - })), - thread([this]() { + : path(path_) { #ifdef __APPLE__ - pthread_setname_np("SQLite Cache"); + pthread_setname_np("SQLite Cache"); #endif - uv_run(loop, UV_RUN_DEFAULT); - - try { - getStmt.reset(); - putStmt.reset(); - refreshStmt.reset(); - db.reset(); - } catch (mapbox::sqlite::Exception& ex) { - Log::Error(Event::Database, ex.code, ex.what()); - } - }) { } SQLiteCache::~SQLiteCache() { - if (thread.joinable()) { - if (queue) { - queue->send(StopAction{ }); - } - thread.join(); - uv_loop_delete(loop); - } -} - - -void SQLiteCache::get(const Resource &resource, std::function)> callback) { - // Can be called from any thread, but most likely from the file source thread. - // Will try to load the URL from the SQLite database and call the callback when done. - // Note that the callback is probably going to invoked from another thread, so the caller - // must make sure that it can run in that thread. - assert(queue); - queue->send(GetAction{ resource, callback }); -} - -void SQLiteCache::put(const Resource &resource, std::shared_ptr response, Hint hint) { - // Can be called from any thread, but most likely from the file source thread. We are either - // storing a new response or updating the currently stored response, potentially setting a new - // expiry date. - assert(queue); - assert(response); - - if (hint == Hint::Full) { - queue->send(PutAction{ resource, response }); - } else if (hint == Hint::Refresh) { - queue->send(RefreshAction{ resource, response->expires }); + // Deleting these SQLite objects may result in exceptions, but we're in a destructor, so we + // can't throw anything. + try { + getStmt.reset(); + putStmt.reset(); + refreshStmt.reset(); + db.reset(); + } catch (mapbox::sqlite::Exception& ex) { + Log::Error(Event::Database, ex.code, ex.what()); } } void SQLiteCache::createDatabase() { db = util::make_unique(path.c_str(), ReadWrite | Create); - - createSchema(); } void SQLiteCache::createSchema() { @@ -171,7 +102,6 @@ void SQLiteCache::createSchema() { db->exec(sql); schema = true; } catch (mapbox::sqlite::Exception &ex) { - if (ex.code == SQLITE_NOTADB) { Log::Warning(Event::Database, "Trashing invalid database"); db.reset(); @@ -192,7 +122,15 @@ void SQLiteCache::createSchema() { } } -void SQLiteCache::process(GetAction &action) { +void SQLiteCache::get(const Resource &resource, Callback callback) { + // Can be called from any thread, but most likely from the file source thread. + // Will try to load the URL from the SQLite database and call the callback when done. + // Note that the callback is probably going to invoked from another thread, so the caller + // must make sure that it can run in that thread. + invoke(std::bind(&SQLiteCache::processGet, this, resource, callback)); +} + +void SQLiteCache::processGet(const Resource &resource, Callback callback) { try { // This is called in the SQLite event loop. if (!db) { @@ -212,7 +150,7 @@ void SQLiteCache::process(GetAction &action) { getStmt->reset(); } - const std::string unifiedURL = unifyMapboxURLs(action.resource.url); + const std::string unifiedURL = unifyMapboxURLs(resource.url); getStmt->bind(1, unifiedURL.c_str()); if (getStmt->run()) { // There is data. @@ -225,18 +163,29 @@ void SQLiteCache::process(GetAction &action) { if (getStmt->get(5)) { // == compressed response->data = util::decompress(response->data); } - action.callback(std::move(response)); + callback(std::move(response)); } else { // There is no data. - action.callback(nullptr); + callback(nullptr); } } catch (mapbox::sqlite::Exception& ex) { Log::Error(Event::Database, ex.code, ex.what()); - action.callback(nullptr); + callback(nullptr); } } -void SQLiteCache::process(PutAction &action) { +void SQLiteCache::put(const Resource &resource, std::shared_ptr response, Hint hint) { + // Can be called from any thread, but most likely from the file source thread. We are either + // storing a new response or updating the currently stored response, potentially setting a new + // expiry date. + if (hint == Hint::Full) { + invoke(std::bind(&SQLiteCache::processPut, this, resource, response)); + } else if (hint == Hint::Refresh) { + invoke(std::bind(&SQLiteCache::processRefresh, this, resource, response->expires)); + } +} + +void SQLiteCache::processPut(const Resource& resource, std::shared_ptr response) { try { if (!db) { createDatabase(); @@ -255,37 +204,37 @@ void SQLiteCache::process(PutAction &action) { putStmt->reset(); } - const std::string unifiedURL = unifyMapboxURLs(action.resource.url); + const std::string unifiedURL = unifyMapboxURLs(resource.url); putStmt->bind(1 /* url */, unifiedURL.c_str()); - putStmt->bind(2 /* status */, int(action.response->status)); - putStmt->bind(3 /* kind */, int(action.resource.kind)); - putStmt->bind(4 /* modified */, action.response->modified); - putStmt->bind(5 /* etag */, action.response->etag.c_str()); - putStmt->bind(6 /* expires */, action.response->expires); + putStmt->bind(2 /* status */, int(response->status)); + putStmt->bind(3 /* kind */, int(resource.kind)); + putStmt->bind(4 /* modified */, response->modified); + putStmt->bind(5 /* etag */, response->etag.c_str()); + putStmt->bind(6 /* expires */, response->expires); std::string data; - if (action.resource.kind != Resource::Image) { + if (resource.kind != Resource::Image) { // Do not compress images, since they are typically compressed already. - data = util::compress(action.response->data); + data = util::compress(response->data); } - if (!data.empty() && data.size() < action.response->data.size()) { + if (!data.empty() && data.size() < response->data.size()) { // Store the compressed data when it is smaller than the original // uncompressed data. putStmt->bind(7 /* data */, data, false); // do not retain the string internally. putStmt->bind(8 /* compressed */, true); } else { - putStmt->bind(7 /* data */, action.response->data, false); // do not retain the string internally. + putStmt->bind(7 /* data */, response->data, false); // do not retain the string internally. putStmt->bind(8 /* compressed */, false); } putStmt->run(); } catch (mapbox::sqlite::Exception& ex) { Log::Error(Event::Database, ex.code, ex.what()); - } + } } -void SQLiteCache::process(RefreshAction &action) { +void SQLiteCache::processRefresh(const Resource& resource, int64_t expires) { try { if (!db) { createDatabase(); @@ -302,8 +251,8 @@ void SQLiteCache::process(RefreshAction &action) { refreshStmt->reset(); } - const std::string unifiedURL = unifyMapboxURLs(action.resource.url); - refreshStmt->bind(1, int64_t(action.expires)); + const std::string unifiedURL = unifyMapboxURLs(resource.url); + refreshStmt->bind(1, int64_t(expires)); refreshStmt->bind(2, unifiedURL.c_str()); refreshStmt->run(); } catch (mapbox::sqlite::Exception& ex) { @@ -311,10 +260,4 @@ void SQLiteCache::process(RefreshAction &action) { } } -void SQLiteCache::process(StopAction &) { - assert(queue); - queue->stop(); - queue = nullptr; -} - } diff --git a/platform/ios/MGLMapView.mm b/platform/ios/MGLMapView.mm index f70f9f348..662b371b8 100644 --- a/platform/ios/MGLMapView.mm +++ b/platform/ios/MGLMapView.mm @@ -14,6 +14,7 @@ #include #include #include +#include #import "MGLTypes.h" #import "NSString+MGLAdditions.h" @@ -111,7 +112,7 @@ std::chrono::steady_clock::duration secondsAsDuration(float duration) mbgl::Map *mbglMap = nullptr; MBGLView *mbglView = nullptr; -mbgl::SQLiteCache *mbglFileCache = nullptr; +mbgl::util::Thread *mbglFileCache = nullptr; mbgl::DefaultFileSource *mbglFileSource = nullptr; - (instancetype)initWithFrame:(CGRect)frame @@ -269,8 +270,8 @@ mbgl::DefaultFileSource *mbglFileSource = nullptr; // setup mbgl map // mbglView = new MBGLView(self); - mbglFileCache = new mbgl::SQLiteCache(defaultCacheDatabase()); - mbglFileSource = new mbgl::DefaultFileSource(mbglFileCache); + mbglFileCache = new mbgl::util::Thread(defaultCacheDatabase()); + mbglFileSource = new mbgl::DefaultFileSource(*mbglFileCache); mbglMap = new mbgl::Map(*mbglView, *mbglFileSource); mbglView->resize(self.bounds.size.width, self.bounds.size.height, _glView.contentScaleFactor, _glView.drawableWidth, _glView.drawableHeight); diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index ca8d423b1..841a56bce 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -122,11 +122,7 @@ Request *DefaultFileSource::request(const Resource &resource, uv_loop_t *l, cons void DefaultFileSource::request(const Resource &resource, const Environment &env, Callback callback) { - auto req = new Request(resource, nullptr, env, std::move(callback)); - - // This function can be called from any thread. Make sure we're executing the actual call in the - // file source loop by sending it over the queue. It will be processed in processAction(). - queue->send(AddRequestAction{ req }); + request(resource, nullptr, env, std::move(callback)); } void DefaultFileSource::cancel(Request *req) { diff --git a/src/mbgl/util/run_loop.cpp b/src/mbgl/util/run_loop.cpp new file mode 100644 index 000000000..3a7cf597f --- /dev/null +++ b/src/mbgl/util/run_loop.cpp @@ -0,0 +1,55 @@ +#include +#include +#include + +#include + +namespace /* anonymous */ { +inline void critical_section(std::mutex& mutex, std::function fn) { + std::lock_guard lock(mutex); + fn(); +} +} + +namespace mbgl { +namespace util { + +RunLoop::RunLoop() + : runloop(util::make_unique()), + runloopAsync(util::make_unique(runloop->get(), std::bind(&RunLoop::process, this))) { +} + +// Define here since we can't destroy the uv::* objects from just the header file. +RunLoop::~RunLoop() = default; + +void RunLoop::start() { + runloop->run(); +} + +void RunLoop::stop() { + critical_section(runloopMutex, [this] { runloopQueue.push(nullptr); }); + runloopAsync->send(); +} + +void RunLoop::process() { + std::queue> queue; + critical_section(runloopMutex, [this, &queue] { queue.swap(runloopQueue); }); + while (!queue.empty()) { + if (queue.front()) { + queue.front()(); + } else { + runloopAsync->unref(); + } + queue.pop(); + } +} + +void RunLoop::invoke(std::function fn) { + if (fn) { + critical_section(runloopMutex, [this, &fn] { runloopQueue.push(fn); }); + runloopAsync->send(); + } +} + +} +} diff --git a/src/mbgl/util/uv_detail.hpp b/src/mbgl/util/uv_detail.hpp index 9d479da42..6acef1b38 100644 --- a/src/mbgl/util/uv_detail.hpp +++ b/src/mbgl/util/uv_detail.hpp @@ -41,10 +41,19 @@ public: uv_loop_close(l); delete l; #endif - } - inline uv_loop_t *operator*() { return l; } + inline void run() { + uv_run(l, UV_RUN_DEFAULT); + } + + inline uv_loop_t* operator*() { + return l; + } + + inline uv_loop_t* get() { + return l; + } private: uv_loop_t *l = nullptr; @@ -72,6 +81,14 @@ public: } } + inline void ref() { + uv_ref(reinterpret_cast(a.get())); + } + + inline void unref() { + uv_unref(reinterpret_cast(a.get())); + } + private: #if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 static void async_cb(uv_async_t* a, int) { diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index ac0dc4c56..f5d7c4bf3 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -4,14 +4,15 @@ #include #include +#include TEST_F(Storage, CacheResponse) { SCOPED_TEST(CacheResponse); using namespace mbgl; - SQLiteCache cache(":memory:"); - DefaultFileSource fs(&cache, uv_default_loop()); + util::Thread cache(":memory:"); + DefaultFileSource fs(cache, uv_default_loop()); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/cache" }; auto &env = *static_cast(nullptr); diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp index bd32042b9..29f25eb77 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -4,6 +4,7 @@ #include #include +#include TEST_F(Storage, CacheRevalidate) { SCOPED_TEST(CacheRevalidateSame) @@ -12,8 +13,8 @@ TEST_F(Storage, CacheRevalidate) { using namespace mbgl; - SQLiteCache cache(":memory:"); - DefaultFileSource fs(&cache); + util::Thread cache(":memory:"); + DefaultFileSource fs(cache); auto &env = *static_cast(nullptr); diff --git a/test/storage/database.cpp b/test/storage/database.cpp index 1a2b618a5..ffa82ecb9 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include @@ -41,9 +42,9 @@ TEST_F(Storage, DatabaseDoesNotExist) { EXPECT_EQ(1ul, dynamic_cast(observer.get())->count({ EventSeverity::Error, Event::Database, 14, "unable to open database file" })); }); - SQLiteCache cache("test/fixtures/404/cache.db"); + util::Thread cache("test/fixtures/404/cache.db"); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); test.finish(); }); @@ -85,9 +86,9 @@ TEST_F(Storage, DatabaseCreate) { Log::removeObserver(); }); - SQLiteCache cache("test/fixtures/database/cache.db"); + util::Thread cache("test/fixtures/database/cache.db"); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); test.finish(); }); @@ -140,7 +141,8 @@ TEST_F(Storage, DatabaseLockedRead) { deleteFile("test/fixtures/database/locked.db"); FileLock guard("test/fixtures/database/locked.db"); - auto cache = util::make_unique("test/fixtures/database/locked.db"); + + util::Thread cache("test/fixtures/database/locked.db"); std::promise promise; @@ -177,9 +179,6 @@ TEST_F(Storage, DatabaseLockedRead) { // Make sure that we got a no errors Log::removeObserver(); } - - // Explicitly delete the Cache now. - cache.reset(); } @@ -192,7 +191,7 @@ TEST_F(Storage, DatabaseLockedWrite) { deleteFile("test/fixtures/database/locked.db"); FileLock guard("test/fixtures/database/locked.db"); - auto cache = util::make_unique("test/fixtures/database/locked.db"); + util::Thread cache("test/fixtures/database/locked.db"); std::promise promise; @@ -234,9 +233,6 @@ TEST_F(Storage, DatabaseLockedWrite) { // Make sure that we got a no errors Log::removeObserver(); } - - // Explicitly delete the Cache now. - cache.reset(); } @@ -249,7 +245,7 @@ TEST_F(Storage, DatabaseLockedRefresh) { createDir("test/fixtures/database"); deleteFile("test/fixtures/database/locked.db"); - auto cache = util::make_unique("test/fixtures/database/locked.db"); + util::Thread cache("test/fixtures/database/locked.db"); // Then, lock the file and try again. FileLock guard("test/fixtures/database/locked.db"); @@ -293,9 +289,6 @@ TEST_F(Storage, DatabaseLockedRefresh) { auto flo = dynamic_cast(observer.get()); EXPECT_EQ(4ul, flo->count({ EventSeverity::Error, Event::Database, 5, "database is locked" })); } - - // Explicitly delete the Cache now. - cache.reset(); } @@ -307,7 +300,7 @@ TEST_F(Storage, DatabaseDeleted) { createDir("test/fixtures/database"); deleteFile("test/fixtures/database/locked.db"); - auto cache = util::make_unique("test/fixtures/database/locked.db"); + util::Thread cache("test/fixtures/database/locked.db"); std::promise promise; @@ -348,9 +341,6 @@ TEST_F(Storage, DatabaseDeleted) { auto flo = dynamic_cast(observer.get()); EXPECT_EQ(1ul, flo->count({ EventSeverity::Error, Event::Database, 8, "attempt to write a readonly database" })); } - - // Explicitly delete the Cache now. - cache.reset(); } @@ -363,7 +353,7 @@ TEST_F(Storage, DatabaseInvalid) { deleteFile("test/fixtures/database/invalid.db"); writeFile("test/fixtures/database/invalid.db", "this is an invalid file"); - auto cache = util::make_unique("test/fixtures/database/invalid.db"); + util::Thread cache("test/fixtures/database/invalid.db"); std::promise promise; @@ -385,7 +375,4 @@ TEST_F(Storage, DatabaseInvalid) { auto flo = dynamic_cast(observer.get()); EXPECT_EQ(1ul, flo->count({ EventSeverity::Warning, Event::Database, -1, "Trashing invalid database" })); } - - // Explicitly delete the Cache now. - cache.reset(); } From bd531b9e080f3300c6947b0e0e515218bb395f45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 10 Apr 2015 15:52:02 +0200 Subject: [PATCH 02/13] guarantee that Object stays around Adds more documentation, and guarantees that the Object in the managed thread stays around until the Thread<> destructor is called. This might happen if .start() returns before .stop() is called, e.g. when a run loop in Object() terminates prematurely. --- include/mbgl/util/run_loop.hpp | 2 +- include/mbgl/util/thread.hpp | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/include/mbgl/util/run_loop.hpp b/include/mbgl/util/run_loop.hpp index f0318d202..5aefcd1d3 100644 --- a/include/mbgl/util/run_loop.hpp +++ b/include/mbgl/util/run_loop.hpp @@ -49,4 +49,4 @@ private: } } -#endif \ No newline at end of file +#endif diff --git a/include/mbgl/util/thread.hpp b/include/mbgl/util/thread.hpp index 943039c5d..6983b8190 100644 --- a/include/mbgl/util/thread.hpp +++ b/include/mbgl/util/thread.hpp @@ -7,6 +7,14 @@ namespace mbgl { namespace util { +// Manages a thread with Object. + +// Upon creation of this object, it launches a thread, creates an object of type Object in that +// thread, and then calls .start(); on that object. When the Thread<> object is destructed, the +// Object's .stop() function is called, and the destructor waits for thread termination. The +// Thread<> constructor blocks until the thread and the Object are fully created, so after the +// object creation, it's safe to obtain the Object stored in this thread. + template class Thread { public: @@ -23,6 +31,7 @@ public: private: std::thread thread; + std::promise joinable; Object& object; }; @@ -35,6 +44,7 @@ Thread::Thread(Args&&... args) Object context(::std::forward(args)...); promise.set_value(context); context.start(); + joinable.get_future().get(); }); return promise.get_future().get(); }()) { @@ -43,6 +53,7 @@ Thread::Thread(Args&&... args) template Thread::~Thread() { object.stop(); + joinable.set_value(); thread.join(); } From 4c7132337f96486a154e75efe394f5a1c06a77a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 10 Apr 2015 22:37:27 +0200 Subject: [PATCH 03/13] convert DefaultFileSource to use util::Thread<> --- android/cpp/native_map_view.cpp | 4 +- include/mbgl/android/native_map_view.hpp | 2 +- .../mbgl/storage/default/asset_request.hpp | 2 +- include/mbgl/storage/default/http_request.hpp | 2 +- .../storage/default/shared_request_base.hpp | 7 +- include/mbgl/storage/default_file_source.hpp | 45 ++---- include/mbgl/util/run_loop.hpp | 6 +- linux/main.cpp | 4 +- macosx/main.mm | 4 +- platform/darwin/http_request_nsurl.mm | 24 +-- platform/default/asset_request_fs.cpp | 2 +- platform/default/asset_request_zip.cpp | 2 +- platform/default/http_request_curl.cpp | 24 +-- platform/ios/MGLMapView.mm | 6 +- src/mbgl/storage/default_file_source.cpp | 145 ++++-------------- src/mbgl/util/run_loop.cpp | 4 +- test/headless/headless.cpp | 5 +- test/storage/cache_response.cpp | 6 +- test/storage/cache_revalidate.cpp | 14 +- test/storage/directory_reading.cpp | 7 +- test/storage/file_reading.cpp | 19 +-- test/storage/http_cancel.cpp | 15 +- test/storage/http_coalescing.cpp | 5 +- test/storage/http_environment.cpp | 9 +- test/storage/http_error.cpp | 7 +- test/storage/http_header_parsing.cpp | 7 +- test/storage/http_load.cpp | 5 +- test/storage/http_noloop.cpp | 5 +- test/storage/http_other_loop.cpp | 5 +- test/storage/http_reading.cpp | 15 +- 30 files changed, 164 insertions(+), 243 deletions(-) diff --git a/android/cpp/native_map_view.cpp b/android/cpp/native_map_view.cpp index 340bbae10..b51e5cbc6 100644 --- a/android/cpp/native_map_view.cpp +++ b/android/cpp/native_map_view.cpp @@ -59,7 +59,7 @@ NativeMapView::NativeMapView(JNIEnv *env, jobject obj_) : mbgl::View(*this), fileCache(mbgl::android::cachePath + "/mbgl-cache.db"), fileSource(fileCache), - map(*this, fileSource) { + map(*this, *fileSource) { mbgl::Log::Debug(mbgl::Event::Android, "NativeMapView::NativeMapView"); assert(env != nullptr); @@ -145,7 +145,7 @@ void NativeMapView::notify() { mbgl::Map &NativeMapView::getMap() { return map; } -mbgl::DefaultFileSource &NativeMapView::getFileSource() { return fileSource; } +mbgl::DefaultFileSource &NativeMapView::getFileSource() { return *fileSource; } bool NativeMapView::inEmulator() { // Detect if we are in emulator diff --git a/include/mbgl/android/native_map_view.hpp b/include/mbgl/android/native_map_view.hpp index 4a8d9365b..817b6bc8b 100644 --- a/include/mbgl/android/native_map_view.hpp +++ b/include/mbgl/android/native_map_view.hpp @@ -63,7 +63,7 @@ private: ANativeWindow *window = nullptr; mbgl::util::Thread fileCache; - mbgl::DefaultFileSource fileSource; + mbgl::util::Thread fileSource; mbgl::Map map; EGLDisplay display = EGL_NO_DISPLAY; diff --git a/include/mbgl/storage/default/asset_request.hpp b/include/mbgl/storage/default/asset_request.hpp index c582c025f..48f402fef 100644 --- a/include/mbgl/storage/default/asset_request.hpp +++ b/include/mbgl/storage/default/asset_request.hpp @@ -9,7 +9,7 @@ class AssetRequest : public SharedRequestBase { public: AssetRequest(DefaultFileSource *source, const Resource &resource); - void start(uv_loop_t *loop, std::unique_ptr response = nullptr); + void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); private: diff --git a/include/mbgl/storage/default/http_request.hpp b/include/mbgl/storage/default/http_request.hpp index 914e622f1..1cd38a828 100644 --- a/include/mbgl/storage/default/http_request.hpp +++ b/include/mbgl/storage/default/http_request.hpp @@ -9,7 +9,7 @@ class HTTPRequest : public SharedRequestBase { public: HTTPRequest(DefaultFileSource *source, const Resource &resource); - void start(uv_loop_t *loop, std::unique_ptr response = nullptr); + void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); void retryImmediately(); diff --git a/include/mbgl/storage/default/shared_request_base.hpp b/include/mbgl/storage/default/shared_request_base.hpp index 59e38efc2..9c5f87b33 100644 --- a/include/mbgl/storage/default/shared_request_base.hpp +++ b/include/mbgl/storage/default/shared_request_base.hpp @@ -29,15 +29,14 @@ public: SharedRequestBase(DefaultFileSource *source_, const Resource &resource_) : resource(resource_), source(source_) {} - virtual void start(uv_loop_t *loop, std::unique_ptr response = nullptr) = 0; + virtual void start(uv_loop_t *loop, std::shared_ptr response = nullptr) = 0; virtual void cancel() = 0; - void notify(std::unique_ptr response, FileCache::Hint hint) { + void notify(std::shared_ptr response, FileCache::Hint hint) { MBGL_VERIFY_THREAD(tid); if (source) { - source->notify(this, observers, std::shared_ptr(std::move(response)), - hint); + source->notify(this, observers, response, hint); } } diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp index 7aab54f73..f6028217e 100644 --- a/include/mbgl/storage/default_file_source.hpp +++ b/include/mbgl/storage/default_file_source.hpp @@ -3,25 +3,25 @@ #include #include +#include #include #include -#include - -namespace mapbox { namespace util { template class variant; } } namespace mbgl { -namespace util { template class AsyncQueue; } - class SharedRequestBase; -class DefaultFileSource : public FileSource { -public: +class DefaultFileSource : public FileSource, protected util::RunLoop { + friend class util::Thread; + friend class SharedRequestBase; + +private: DefaultFileSource(FileCache *cache, const std::string &root = ""); - DefaultFileSource(FileCache *cache, uv_loop_t *loop, const std::string &root = ""); ~DefaultFileSource() override; +public: + // FileSource API Request *request(const Resource &resource, uv_loop_t *loop, const Environment &env, Callback callback) override; void cancel(Request *request) override; @@ -29,37 +29,22 @@ public: void abort(const Environment &env) override; +private: void notify(SharedRequestBase *sharedRequest, const std::set &observers, std::shared_ptr response, FileCache::Hint hint); + SharedRequestBase *find(const Resource &resource); + + void processAdd(Request* request); + void processCancel(Request* request); + void processResult(const Resource& resource, std::shared_ptr response); + void processAbort(const Environment& env); public: const std::string assetRoot; private: - struct ActionDispatcher; - struct AddRequestAction; - struct RemoveRequestAction; - struct ResultAction; - struct StopAction; - 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); - std::unordered_map pending; - - uv_loop_t *loop = nullptr; FileCache *cache = nullptr; - Queue *queue = nullptr; - std::thread thread; }; } diff --git a/include/mbgl/util/run_loop.hpp b/include/mbgl/util/run_loop.hpp index 5aefcd1d3..9bed417c3 100644 --- a/include/mbgl/util/run_loop.hpp +++ b/include/mbgl/util/run_loop.hpp @@ -1,6 +1,7 @@ #ifndef MBGL_UTIL_RUN_LOOP #define MBGL_UTIL_RUN_LOOP +#include #include #include #include @@ -31,13 +32,16 @@ protected: // Called by the Thread<> wrapper to terminate this loop. void stop(); + // Obtain the underlying loop object in case you want to attach additional listeners. + uv::loop& loop() { return *runloop; }; + private: // Invokes function in the run loop. void process(); public: // Schedules a function to be executed as part of this run loop. - void invoke(std::function fn); + void invoke(std::function&& fn); private: const std::unique_ptr runloop; diff --git a/linux/main.cpp b/linux/main.cpp index 71c2424af..d91aa7dec 100644 --- a/linux/main.cpp +++ b/linux/main.cpp @@ -67,8 +67,8 @@ int main(int argc, char *argv[]) { view = mbgl::util::make_unique(); mbgl::util::Thread cache("/tmp/mbgl-cache.db"); - mbgl::DefaultFileSource fileSource(cache); - mbgl::Map map(*view, fileSource); + mbgl::util::Thread fileSource(cache); + mbgl::Map map(*view, *fileSource); // Load settings mbgl::Settings_JSON settings; diff --git a/macosx/main.mm b/macosx/main.mm index fb12282e2..d0f3282f8 100644 --- a/macosx/main.mm +++ b/macosx/main.mm @@ -105,8 +105,8 @@ int main() { GLFWView view; mbgl::util::Thread cache(defaultCacheDatabase()); - mbgl::DefaultFileSource fileSource(cache); - mbgl::Map map(view, fileSource); + mbgl::util::Thread fileSource(cache); + mbgl::Map map(view, *fileSource); URLHandler *handler = [[URLHandler alloc] init]; [handler setMap:&map]; diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index 7be5f226f..773f83e5d 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -47,7 +47,7 @@ class HTTPNSURLContext; class HTTPRequestImpl { public: - HTTPRequestImpl(HTTPRequest *request, uv_loop_t *loop, std::unique_ptr response); + HTTPRequestImpl(HTTPRequest *request, uv_loop_t *loop, std::shared_ptr response); ~HTTPRequestImpl(); void cancel(); @@ -66,7 +66,7 @@ private: HTTPRequest *request = nullptr; NSURLSessionDataTask *task = nullptr; std::unique_ptr response; - std::unique_ptr existingResponse; + const std::shared_ptr existingResponse; ResponseStatus status = ResponseStatus::PermanentError; uv_async_t *async = nullptr; int attempts = 0; @@ -118,10 +118,10 @@ HTTPNSURLContext::~HTTPNSURLContext() { // ------------------------------------------------------------------------------------------------- HTTPRequestImpl::HTTPRequestImpl(HTTPRequest *request_, uv_loop_t *loop, - std::unique_ptr existingResponse_) + std::shared_ptr existingResponse_) : context(HTTPNSURLContext::Get(loop)), request(request_), - existingResponse(std::move(existingResponse_)), + existingResponse(existingResponse_), async(new uv_async_t) { assert(request); context->addRequest(request); @@ -326,12 +326,12 @@ void HTTPRequestImpl::handleResult(NSData *data, NSURLResponse *res, NSError *er if (responseCode == 304) { if (existingResponse) { - // We're going to reuse the old response object, but need to copy over the new - // expires value (if possible). - std::swap(response, existingResponse); - if (existingResponse->expires) { - response->expires = existingResponse->expires; - } + // We're going to copy over the existing response's data. + response->status = existingResponse->status; + response->message = existingResponse->message; + response->modified = existingResponse->modified; + response->etag = existingResponse->etag; + response->data = existingResponse->data; status = ResponseStatus::NotModified; } else { // This is an unsolicited 304 response and should only happen on malfunctioning @@ -409,11 +409,11 @@ HTTPRequest::~HTTPRequest() { } } -void HTTPRequest::start(uv_loop_t *loop, std::unique_ptr response) { +void HTTPRequest::start(uv_loop_t *loop, std::shared_ptr response) { MBGL_VERIFY_THREAD(tid); assert(!ptr); - ptr = new HTTPRequestImpl(this, loop, std::move(response)); + ptr = new HTTPRequestImpl(this, loop, response); } void HTTPRequest::retryImmediately() { diff --git a/platform/default/asset_request_fs.cpp b/platform/default/asset_request_fs.cpp index 5ebde2d88..10d5f33a3 100644 --- a/platform/default/asset_request_fs.cpp +++ b/platform/default/asset_request_fs.cpp @@ -220,7 +220,7 @@ AssetRequest::~AssetRequest() { } } -void AssetRequest::start(uv_loop_t *loop, std::unique_ptr response) { +void AssetRequest::start(uv_loop_t *loop, std::shared_ptr response) { MBGL_VERIFY_THREAD(tid); // We're ignoring the existing response if any. diff --git a/platform/default/asset_request_zip.cpp b/platform/default/asset_request_zip.cpp index fd27c4b95..85ff0aede 100644 --- a/platform/default/asset_request_zip.cpp +++ b/platform/default/asset_request_zip.cpp @@ -288,7 +288,7 @@ AssetRequest::~AssetRequest() { } } -void AssetRequest::start(uv_loop_t *loop, std::unique_ptr response) { +void AssetRequest::start(uv_loop_t *loop, std::shared_ptr response) { MBGL_VERIFY_THREAD(tid); // We're ignoring the existing response if any. diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index a7ec162aa..707465a3d 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -95,7 +95,7 @@ class HTTPRequestImpl { MBGL_STORE_THREAD(tid) public: - HTTPRequestImpl(HTTPRequest *request, uv_loop_t *loop, std::unique_ptr response); + HTTPRequestImpl(HTTPRequest *request, uv_loop_t *loop, std::shared_ptr response); ~HTTPRequestImpl(); void handleResult(CURLcode code); @@ -123,7 +123,7 @@ private: std::unique_ptr response; // In case of revalidation requests, this will store the old response. - std::unique_ptr existingResponse; + const std::shared_ptr existingResponse; CURL *handle = nullptr; curl_slist *headers = nullptr; @@ -425,10 +425,10 @@ static CURLcode sslctx_function(CURL * /* curl */, void *sslctx, void * /* parm } #endif -HTTPRequestImpl::HTTPRequestImpl(HTTPRequest *request_, uv_loop_t *loop, std::unique_ptr response_) +HTTPRequestImpl::HTTPRequestImpl(HTTPRequest *request_, uv_loop_t *loop, std::shared_ptr response_) : context(HTTPCURLContext::Get(loop)), request(request_), - existingResponse(std::move(response_)), + existingResponse(response_), handle(context->getHandle()) { assert(request); context->addRequest(request); @@ -688,12 +688,12 @@ void HTTPRequestImpl::handleResult(CURLcode code) { if (responseCode == 304) { if (existingResponse) { - // We're going to reuse the old response object, but need to copy over the new - // expires value (if possible). - std::swap(response, existingResponse); - if (existingResponse->expires) { - response->expires = existingResponse->expires; - } + // We're going to copy over the existing response's data. + response->status = existingResponse->status; + response->message = existingResponse->message; + response->modified = existingResponse->modified; + response->etag = existingResponse->etag; + response->data = existingResponse->data; return finish(ResponseStatus::NotModified); } else { // This is an unsolicited 304 response and should only happen on malfunctioning @@ -734,11 +734,11 @@ HTTPRequest::~HTTPRequest() { } } -void HTTPRequest::start(uv_loop_t *loop, std::unique_ptr response) { +void HTTPRequest::start(uv_loop_t *loop, std::shared_ptr response) { MBGL_VERIFY_THREAD(tid); assert(!ptr); - ptr = new HTTPRequestImpl(this, loop, std::move(response)); + ptr = new HTTPRequestImpl(this, loop, response); } void HTTPRequest::retryImmediately() { diff --git a/platform/ios/MGLMapView.mm b/platform/ios/MGLMapView.mm index 662b371b8..8281b6ec5 100644 --- a/platform/ios/MGLMapView.mm +++ b/platform/ios/MGLMapView.mm @@ -113,7 +113,7 @@ std::chrono::steady_clock::duration secondsAsDuration(float duration) mbgl::Map *mbglMap = nullptr; MBGLView *mbglView = nullptr; mbgl::util::Thread *mbglFileCache = nullptr; -mbgl::DefaultFileSource *mbglFileSource = nullptr; +mbgl::util::Thread *mbglFileSource = nullptr; - (instancetype)initWithFrame:(CGRect)frame { @@ -271,8 +271,8 @@ mbgl::DefaultFileSource *mbglFileSource = nullptr; // mbglView = new MBGLView(self); mbglFileCache = new mbgl::util::Thread(defaultCacheDatabase()); - mbglFileSource = new mbgl::DefaultFileSource(*mbglFileCache); - mbglMap = new mbgl::Map(*mbglView, *mbglFileSource); + mbglFileSource = new mbgl::util::Thread(*mbglFileCache); + mbglMap = new mbgl::Map(*mbglView, **mbglFileSource); mbglView->resize(self.bounds.size.width, self.bounds.size.height, _glView.contentScaleFactor, _glView.drawableWidth, _glView.drawableHeight); // Notify map object when network reachability status changes. diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 841a56bce..0ee31b7fe 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -6,10 +6,7 @@ #include #include -#include -#include - -#include +#include #include #include @@ -30,72 +27,15 @@ namespace algo = boost::algorithm; namespace mbgl { -struct DefaultFileSource::ActionDispatcher { - DefaultFileSource &fileSource; - template void operator()(T &t) { fileSource.process(t); } -}; - -struct DefaultFileSource::AddRequestAction { - Request *const request; -}; - -struct DefaultFileSource::RemoveRequestAction { - Request *const request; -}; - -struct DefaultFileSource::ResultAction { - const Resource resource; - std::unique_ptr response; -}; - -struct DefaultFileSource::StopAction { -}; - -struct DefaultFileSource::AbortAction { - const Environment &env; -}; - - -DefaultFileSource::DefaultFileSource(FileCache *cache_, const std::string &root) - : assetRoot(root.empty() ? platform::assetRoot() : root), - loop(uv_loop_new()), - cache(cache_), - queue(new Queue(loop, [this](Action &action) { - mapbox::util::apply_visitor(ActionDispatcher{*this}, action); - })), - thread([this]() { +DefaultFileSource::DefaultFileSource(FileCache* cache_, const std::string& root) + : assetRoot(root.empty() ? platform::assetRoot() : root), cache(cache_) { #ifdef __APPLE__ - pthread_setname_np("FileSource"); + pthread_setname_np("FileSource"); #endif - uv_run(loop, UV_RUN_DEFAULT); - }) { -} - -DefaultFileSource::DefaultFileSource(FileCache *cache_, uv_loop_t *loop_, const std::string &root) - : assetRoot(root.empty() ? platform::assetRoot() : root), - loop(loop_), - cache(cache_), - queue(new Queue(loop, [this](Action &action) { - mapbox::util::apply_visitor(ActionDispatcher{*this}, action); - })) { - // Make sure that the queue doesn't block the loop from exiting. - queue->unref(); } DefaultFileSource::~DefaultFileSource() { MBGL_VERIFY_THREAD(tid); - - if (thread.joinable()) { - if (queue) { - queue->send(StopAction{ }); - } - thread.join(); - uv_loop_delete(loop); - } else { - // Assume that the loop we received is running in the current thread. - StopAction action {}; - process(action); - } } SharedRequestBase *DefaultFileSource::find(const Resource &resource) { @@ -110,18 +50,20 @@ SharedRequestBase *DefaultFileSource::find(const Resource &resource) { return nullptr; } -Request *DefaultFileSource::request(const Resource &resource, uv_loop_t *l, const Environment &env, +Request* DefaultFileSource::request(const Resource& resource, + uv_loop_t* l, + const Environment& env, Callback callback) { auto req = new Request(resource, l, env, std::move(callback)); // This function can be called from any thread. Make sure we're executing the actual call in the - // file source loop by sending it over the queue. It will be processed in processAction(). - queue->send(AddRequestAction{ req }); + // file source loop by sending it over the queue. + invoke([this, req] { processAdd(req); }); + return req; } -void DefaultFileSource::request(const Resource &resource, const Environment &env, - Callback callback) { +void DefaultFileSource::request(const Resource& resource, const Environment& env, Callback callback) { request(resource, nullptr, env, std::move(callback)); } @@ -129,17 +71,16 @@ void DefaultFileSource::cancel(Request *req) { req->cancel(); // This function can be called from any thread. Make sure we're executing the actual call in the - // file source loop by sending it over the queue. It will be processed in processAction(). - queue->send(RemoveRequestAction{ req }); + // file source loop by sending it over the queue. + invoke([this, req] { processCancel(req); }); } void DefaultFileSource::abort(const Environment &env) { - queue->send(AbortAction{ env }); + invoke([this, &env] { processAbort(env); }); } - -void DefaultFileSource::process(AddRequestAction &action) { - const Resource &resource = action.request->resource; +void DefaultFileSource::processAdd(Request* request) { + const Resource &resource = request->resource; // We're adding a new Request. SharedRequestBase *sharedRequest = find(resource); @@ -151,36 +92,32 @@ void DefaultFileSource::process(AddRequestAction &action) { sharedRequest = new HTTPRequest(this, resource); } - // Make sure the loop stays alive when we're not running the file source in it's own thread. - if (!thread.joinable() && pending.empty()) { - queue->ref(); - } - const bool inserted = pending.emplace(resource, sharedRequest).second; assert(inserted); (void (inserted)); // silence unused variable warning on Release builds. // But first, we're going to start querying the database if it exists. if (!cache) { - sharedRequest->start(loop); + sharedRequest->start(loop().get()); } else { // Otherwise, first check the cache for existing data so that we can potentially // revalidate the information without having to redownload everything. cache->get(resource, [this, resource](std::unique_ptr response) { - queue->send(ResultAction { resource, std::move(response) }); + std::shared_ptr sharedResponse = std::move(response); + invoke([this, resource, sharedResponse] { processResult(resource, sharedResponse); }); }); } } - sharedRequest->subscribe(action.request); + sharedRequest->subscribe(request); } -void DefaultFileSource::process(RemoveRequestAction &action) { - SharedRequestBase *sharedRequest = find(action.request->resource); +void DefaultFileSource::processCancel(Request* request) { + SharedRequestBase *sharedRequest = find(request->resource); if (sharedRequest) { // If the number of dependent requests of the SharedRequestBase drops to zero, the // unsubscribe callback triggers the removal of the SharedRequestBase pointer from the list // of pending requests and initiates cancelation. - sharedRequest->unsubscribe(action.request); + sharedRequest->unsubscribe(request); } else { // There is no request for this URL anymore. Likely, the request already completed // before we got around to process the cancelation request. @@ -188,28 +125,28 @@ void DefaultFileSource::process(RemoveRequestAction &action) { // Send a message back to the requesting thread and notify it that this request has been // canceled and is now safe to be deleted. - action.request->destruct(); + request->destruct(); } -void DefaultFileSource::process(ResultAction &action) { - SharedRequestBase *sharedRequest = find(action.resource); +void DefaultFileSource::processResult(const Resource& resource, std::shared_ptr response) { + SharedRequestBase *sharedRequest = find(resource); if (sharedRequest) { - if (action.response) { + if (response) { // This entry was stored in the cache. Now determine if we need to revalidate. const int64_t now = std::chrono::duration_cast( SystemClock::now().time_since_epoch()).count(); - if (action.response->expires > now) { + if (response->expires > now) { // The response is fresh. We're good to notify the caller. - sharedRequest->notify(std::move(action.response), FileCache::Hint::No); + sharedRequest->notify(response, FileCache::Hint::No); sharedRequest->cancel(); return; } else { // The cached response is stale. Now run the real request. - sharedRequest->start(loop, std::move(action.response)); + sharedRequest->start(loop().get(), response); } } else { // There is no response. Now run the real request. - sharedRequest->start(loop); + sharedRequest->start(loop().get()); } } else { // There is no request for this URL anymore. Likely, the request was canceled @@ -217,19 +154,8 @@ void DefaultFileSource::process(ResultAction &action) { } } -// A stop action means the file source is about to be destructed. We need to cancel all requests -// for all environments. -void DefaultFileSource::process(StopAction &) { - // There may not be any pending requests in this file source anymore. You must terminate all - // Map objects before deleting the FileSource. - assert(pending.empty()); - assert(queue); - queue->stop(); - queue = nullptr; -} - // Aborts all requests that are part of the current environment. -void DefaultFileSource::process(AbortAction &action) { +void DefaultFileSource::processAbort(const Environment& env) { // Construct a cancellation response. auto res = util::make_unique(); res->status = Response::Error; @@ -239,7 +165,7 @@ void DefaultFileSource::process(AbortAction &action) { // Iterate through all pending requests and remove them in case they're abandoned. util::erase_if(pending, [&](const std::pair &it) -> bool { // Obtain all pending requests that are in the current environment. - const auto aborted = it.second->removeAllInEnvironment(action.env); + const auto aborted = it.second->removeAllInEnvironment(env); // Notify all observers. for (auto req : aborted) { @@ -274,11 +200,6 @@ void DefaultFileSource::notify(SharedRequestBase *sharedRequest, req->notify(response); } } - - if (!thread.joinable() && pending.empty()) { - // When there are no pending requests, we're going to allow the queue to stop. - queue->unref(); - } } } diff --git a/src/mbgl/util/run_loop.cpp b/src/mbgl/util/run_loop.cpp index 3a7cf597f..dba75fcfc 100644 --- a/src/mbgl/util/run_loop.cpp +++ b/src/mbgl/util/run_loop.cpp @@ -44,9 +44,9 @@ void RunLoop::process() { } } -void RunLoop::invoke(std::function fn) { +void RunLoop::invoke(std::function&& fn) { if (fn) { - critical_section(runloopMutex, [this, &fn] { runloopQueue.push(fn); }); + critical_section(runloopMutex, [this, &fn] { runloopQueue.emplace(std::move(fn)); }); runloopAsync->send(); } } diff --git a/test/headless/headless.cpp b/test/headless/headless.cpp index c47b9349a..76c126433 100644 --- a/test/headless/headless.cpp +++ b/test/headless/headless.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -139,8 +140,8 @@ TEST_P(HeadlessTest, render) { } HeadlessView view(display); - mbgl::DefaultFileSource fileSource(nullptr); - Map map(view, fileSource); + util::Thread fileSource(nullptr); + Map map(view, *fileSource); map.setClasses(classes); map.setStyleJSON(style, "test/suite"); diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index f5d7c4bf3..46083e2f9 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -12,12 +12,12 @@ TEST_F(Storage, CacheResponse) { using namespace mbgl; util::Thread cache(":memory:"); - DefaultFileSource fs(cache, uv_default_loop()); + util::Thread fs(cache); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/cache" }; auto &env = *static_cast(nullptr); - fs.request(resource, uv_default_loop(), env, [&](const Response &res) { + fs->request(resource, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response 1", res.data); EXPECT_LT(0, res.expires); @@ -25,7 +25,7 @@ TEST_F(Storage, CacheResponse) { EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); - fs.request(resource, uv_default_loop(), env, [&, res](const Response &res2) { + fs->request(resource, uv_default_loop(), env, [&, res](const Response &res2) { EXPECT_EQ(res.status, res2.status); EXPECT_EQ(res.data, res2.data); EXPECT_EQ(res.expires, res2.expires); diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp index 29f25eb77..901c151f1 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -14,12 +14,12 @@ TEST_F(Storage, CacheRevalidate) { using namespace mbgl; util::Thread cache(":memory:"); - DefaultFileSource fs(cache); + util::Thread fs(cache); auto &env = *static_cast(nullptr); const Resource revalidateSame { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" }; - fs.request(revalidateSame, uv_default_loop(), env, [&](const Response &res) { + fs->request(revalidateSame, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); @@ -27,7 +27,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("snowfall", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateSame, uv_default_loop(), env, [&, res](const Response &res2) { + fs->request(revalidateSame, uv_default_loop(), env, [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. @@ -43,7 +43,7 @@ TEST_F(Storage, CacheRevalidate) { const Resource revalidateModified{ Resource::Unknown, "http://127.0.0.1:3000/revalidate-modified" }; - fs.request(revalidateModified, uv_default_loop(), env, [&](const Response &res) { + fs->request(revalidateModified, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); @@ -51,7 +51,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateModified, uv_default_loop(), env, [&, res](const Response &res2) { + fs->request(revalidateModified, uv_default_loop(), env, [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. @@ -65,7 +65,7 @@ TEST_F(Storage, CacheRevalidate) { }); const Resource revalidateEtag { Resource::Unknown, "http://127.0.0.1:3000/revalidate-etag" }; - fs.request(revalidateEtag, uv_default_loop(), env, [&](const Response &res) { + fs->request(revalidateEtag, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response 1", res.data); EXPECT_EQ(0, res.expires); @@ -73,7 +73,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("response-1", res.etag); EXPECT_EQ("", res.message); - fs.request(revalidateEtag, uv_default_loop(), env, [&, res](const Response &res2) { + fs->request(revalidateEtag, uv_default_loop(), env, [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response 2", res2.data); EXPECT_EQ(0, res2.expires); diff --git a/test/storage/directory_reading.cpp b/test/storage/directory_reading.cpp index a95564846..4264129b2 100644 --- a/test/storage/directory_reading.cpp +++ b/test/storage/directory_reading.cpp @@ -3,6 +3,7 @@ #include #include +#include TEST_F(Storage, AssetReadDirectory) { SCOPED_TEST(ReadDirectory) @@ -10,14 +11,14 @@ TEST_F(Storage, AssetReadDirectory) { using namespace mbgl; #ifdef MBGL_ASSET_ZIP - DefaultFileSource fs(nullptr, uv_default_loop(), "test/fixtures/storage/assets.zip"); + util::Thread fs(nullptr, "test/fixtures/storage/assets.zip"); #else - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); #endif auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(), + fs->request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(0ul, res.data.size()); diff --git a/test/storage/file_reading.cpp b/test/storage/file_reading.cpp index cca072b27..de834798d 100644 --- a/test/storage/file_reading.cpp +++ b/test/storage/file_reading.cpp @@ -4,6 +4,7 @@ #include #include +#include TEST_F(Storage, AssetEmptyFile) { SCOPED_TEST(EmptyFile) @@ -11,14 +12,14 @@ TEST_F(Storage, AssetEmptyFile) { using namespace mbgl; #ifdef MBGL_ASSET_ZIP - DefaultFileSource fs(nullptr, uv_default_loop(), "test/fixtures/storage/assets.zip"); + util::Thread fs(nullptr, "test/fixtures/storage/assets.zip"); #else - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); #endif auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(), + fs->request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(0ul, res.data.size()); @@ -38,14 +39,14 @@ TEST_F(Storage, AssetNonEmptyFile) { using namespace mbgl; #ifdef MBGL_ASSET_ZIP - DefaultFileSource fs(nullptr, uv_default_loop(), "test/fixtures/storage/assets.zip"); + util::Thread fs(nullptr, "test/fixtures/storage/assets.zip"); #else - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); #endif auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" }, + fs->request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(16ul, res.data.size()); @@ -66,14 +67,14 @@ TEST_F(Storage, AssetNonExistentFile) { using namespace mbgl; #ifdef MBGL_ASSET_ZIP - DefaultFileSource fs(nullptr, uv_default_loop(), "test/fixtures/storage/assets.zip"); + util::Thread fs(nullptr, "test/fixtures/storage/assets.zip"); #else - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); #endif auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" }, + fs->request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(0ul, res.data.size()); diff --git a/test/storage/http_cancel.cpp b/test/storage/http_cancel.cpp index d0dac8ccd..33edbdf7c 100644 --- a/test/storage/http_cancel.cpp +++ b/test/storage/http_cancel.cpp @@ -4,6 +4,7 @@ #include #include +#include #include @@ -12,15 +13,15 @@ TEST_F(Storage, HTTPCancel) { using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); auto req = - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, [&](const Response &) { ADD_FAILURE() << "Callback should not be called"; }); - fs.cancel(req); + fs->cancel(req); HTTPCancel.finish(); uv_run(uv_default_loop(), UV_RUN_DEFAULT); @@ -31,15 +32,15 @@ TEST_F(Storage, HTTPCancelMultiple) { using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; - auto req2 = fs.request(resource, uv_default_loop(), env, [&](const Response &) { + auto req2 = fs->request(resource, uv_default_loop(), env, [&](const Response &) { ADD_FAILURE() << "Callback should not be called"; }); - fs.request(resource, uv_default_loop(), env, [&](const Response &res) { + fs->request(resource, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); @@ -48,7 +49,7 @@ TEST_F(Storage, HTTPCancelMultiple) { EXPECT_EQ("", res.message); HTTPCancelMultiple.finish(); }); - fs.cancel(req2); + fs->cancel(req2); uv_run(uv_default_loop(), UV_RUN_DEFAULT); } diff --git a/test/storage/http_coalescing.cpp b/test/storage/http_coalescing.cpp index 9eaea70e1..5164117f5 100644 --- a/test/storage/http_coalescing.cpp +++ b/test/storage/http_coalescing.cpp @@ -3,6 +3,7 @@ #include #include +#include TEST_F(Storage, HTTPCoalescing) { SCOPED_TEST(HTTPCoalescing) @@ -12,7 +13,7 @@ TEST_F(Storage, HTTPCoalescing) { using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); @@ -43,7 +44,7 @@ TEST_F(Storage, HTTPCoalescing) { const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; for (int i = 0; i < total; i++) { - fs.request(resource, uv_default_loop(), env, complete); + fs->request(resource, uv_default_loop(), env, complete); } uv_run(uv_default_loop(), UV_RUN_DEFAULT); diff --git a/test/storage/http_environment.cpp b/test/storage/http_environment.cpp index efd7b78ad..b08a8071c 100644 --- a/test/storage/http_environment.cpp +++ b/test/storage/http_environment.cpp @@ -4,6 +4,7 @@ #include #include +#include #include @@ -13,7 +14,7 @@ TEST_F(Storage, HTTPCancelEnvironment) { using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); // Create two fake environment pointers. The FileSource implementation treats these as opaque // pointers and doesn't reach into them. @@ -23,7 +24,7 @@ TEST_F(Storage, HTTPCancelEnvironment) { const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/delayed" }; // Environment 1 - fs.request(resource, uv_default_loop(), env1, [&](const Response &res) { + fs->request(resource, uv_default_loop(), env1, [&](const Response &res) { // This environment gets aborted below. This means the request is marked as failing and // will return an error here. EXPECT_EQ(Response::Error, res.status); @@ -36,7 +37,7 @@ TEST_F(Storage, HTTPCancelEnvironment) { }); // Environment 2 - fs.request(resource, uv_default_loop(), env2, [&](const Response &res) { + fs->request(resource, uv_default_loop(), env2, [&](const Response &res) { // The same request as above, but in a different environment which doesn't get aborted. This // means the request should succeed. EXPECT_EQ(Response::Successful, res.status); @@ -48,7 +49,7 @@ TEST_F(Storage, HTTPCancelEnvironment) { HTTPRetainedEnvironment.finish(); }); - fs.abort(env1); + fs->abort(env1); uv_run(uv_default_loop(), UV_RUN_DEFAULT); } diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp index abaeff539..bd5ef9811 100644 --- a/test/storage/http_error.cpp +++ b/test/storage/http_error.cpp @@ -4,6 +4,7 @@ #include #include +#include #include @@ -20,13 +21,13 @@ TEST_F(Storage, HTTPError) { }, 500, 500); uv_unref((uv_handle_t *)&statusChange); - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); auto start = uv_hrtime(); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(), + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(), env, [&](const Response &res) { const auto duration = double(uv_hrtime() - start) / 1e9; EXPECT_LT(1, duration) << "Backoff timer didn't wait 1 second"; @@ -41,7 +42,7 @@ TEST_F(Storage, HTTPError) { HTTPTemporaryError.finish(); }); - fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(), env, + fs->request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(), env, [&](const Response &res) { const auto duration = double(uv_hrtime() - start) / 1e9; // 1.5 seconds == 4 retries, with a 500ms timeout (see above). diff --git a/test/storage/http_header_parsing.cpp b/test/storage/http_header_parsing.cpp index e4d86fcc2..df1fb1fc6 100644 --- a/test/storage/http_header_parsing.cpp +++ b/test/storage/http_header_parsing.cpp @@ -4,6 +4,7 @@ #include #include +#include #include @@ -13,11 +14,11 @@ TEST_F(Storage, HTTPHeaderParsing) { using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test?modified=1420794326&expires=1420797926&etag=foo" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); @@ -32,7 +33,7 @@ TEST_F(Storage, HTTPHeaderParsing) { int64_t now = std::chrono::duration_cast( SystemClock::now().time_since_epoch()).count(); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" }, + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); diff --git a/test/storage/http_load.cpp b/test/storage/http_load.cpp index 2680daf93..b03786723 100644 --- a/test/storage/http_load.cpp +++ b/test/storage/http_load.cpp @@ -3,13 +3,14 @@ #include #include +#include TEST_F(Storage, HTTPLoad) { SCOPED_TEST(HTTPLoad) using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); @@ -19,7 +20,7 @@ TEST_F(Storage, HTTPLoad) { std::function req = [&]() { const auto current = number++; - fs.request({ Resource::Unknown, + fs->request({ Resource::Unknown, std::string("http://127.0.0.1:3000/load/") + std::to_string(current) }, uv_default_loop(), env, [&, current](const Response &res) { EXPECT_EQ(Response::Successful, res.status); diff --git a/test/storage/http_noloop.cpp b/test/storage/http_noloop.cpp index cd4ebeb2c..0f75c106f 100644 --- a/test/storage/http_noloop.cpp +++ b/test/storage/http_noloop.cpp @@ -4,13 +4,14 @@ #include #include +#include TEST_F(Storage, HTTPNoLoop) { SCOPED_TEST(HTTPNoLoop) using namespace mbgl; - DefaultFileSource fs(nullptr); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); @@ -22,7 +23,7 @@ TEST_F(Storage, HTTPNoLoop) { uv::close(as); }); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, nullptr, env, + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, nullptr, env, [&](const Response &res) { EXPECT_NE(uv_thread_self(), mainThread) << "Response was called in the same thread"; EXPECT_EQ(Response::Successful, res.status); diff --git a/test/storage/http_other_loop.cpp b/test/storage/http_other_loop.cpp index 9ad673cf7..11fa2f324 100644 --- a/test/storage/http_other_loop.cpp +++ b/test/storage/http_other_loop.cpp @@ -3,6 +3,7 @@ #include #include +#include TEST_F(Storage, HTTPOtherLoop) { SCOPED_TEST(HTTPOtherLoop) @@ -10,11 +11,11 @@ TEST_F(Storage, HTTPOtherLoop) { using namespace mbgl; // This file source launches a separate thread to do the processing. - DefaultFileSource fs(nullptr); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp index a6a577582..1ba34801c 100644 --- a/test/storage/http_reading.cpp +++ b/test/storage/http_reading.cpp @@ -3,6 +3,7 @@ #include #include +#include TEST_F(Storage, HTTPReading) { SCOPED_TEST(HTTPTest) @@ -10,13 +11,13 @@ TEST_F(Storage, HTTPReading) { using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); const auto mainThread = uv_thread_self(); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Successful, res.status); @@ -28,7 +29,7 @@ TEST_F(Storage, HTTPReading) { HTTPTest.finish(); }); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(), + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Error, res.status); @@ -47,11 +48,11 @@ TEST_F(Storage, HTTPNoCallback) { using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, nullptr); uv_run(uv_default_loop(), UV_RUN_DEFAULT); @@ -64,11 +65,11 @@ TEST_F(Storage, HTTPNoCallbackNoLoop) { using namespace mbgl; - DefaultFileSource fs(nullptr, uv_default_loop()); + util::Thread fs(nullptr); auto &env = *static_cast(nullptr); - fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, env, nullptr); + fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, env, nullptr); uv_run(uv_default_loop(), UV_RUN_DEFAULT); From 00e9cd9e9d53e32a964e4cbcc42fd7ea587c0475 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 10 Apr 2015 22:39:53 +0200 Subject: [PATCH 04/13] OS X needs -framework Foundation for CURL tests --- gyp/platform-osx.gypi | 1 + 1 file changed, 1 insertion(+) diff --git a/gyp/platform-osx.gypi b/gyp/platform-osx.gypi index aecb05859..6d6c35b29 100644 --- a/gyp/platform-osx.gypi +++ b/gyp/platform-osx.gypi @@ -26,6 +26,7 @@ '<@(uv_static_libs)', ], 'ldflags': [ + '-framework Foundation', '-framework ImageIO', '-framework CoreServices', '-framework OpenGL', From 0f79a895023c438e603cf70fb948e04cff05b478 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Fri, 10 Apr 2015 23:19:35 +0200 Subject: [PATCH 05/13] don't shadow variable names --- src/mbgl/storage/default_file_source.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 0ee31b7fe..71b3be850 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -79,8 +79,8 @@ void DefaultFileSource::abort(const Environment &env) { invoke([this, &env] { processAbort(env); }); } -void DefaultFileSource::processAdd(Request* request) { - const Resource &resource = request->resource; +void DefaultFileSource::processAdd(Request* req) { + const Resource &resource = req->resource; // We're adding a new Request. SharedRequestBase *sharedRequest = find(resource); @@ -108,16 +108,16 @@ void DefaultFileSource::processAdd(Request* request) { }); } } - sharedRequest->subscribe(request); + sharedRequest->subscribe(req); } -void DefaultFileSource::processCancel(Request* request) { - SharedRequestBase *sharedRequest = find(request->resource); +void DefaultFileSource::processCancel(Request* req) { + SharedRequestBase *sharedRequest = find(req->resource); if (sharedRequest) { // If the number of dependent requests of the SharedRequestBase drops to zero, the // unsubscribe callback triggers the removal of the SharedRequestBase pointer from the list // of pending requests and initiates cancelation. - sharedRequest->unsubscribe(request); + sharedRequest->unsubscribe(req); } else { // There is no request for this URL anymore. Likely, the request already completed // before we got around to process the cancelation request. @@ -125,7 +125,7 @@ void DefaultFileSource::processCancel(Request* request) { // Send a message back to the requesting thread and notify it that this request has been // canceled and is now safe to be deleted. - request->destruct(); + req->destruct(); } void DefaultFileSource::processResult(const Resource& resource, std::shared_ptr response) { From 278e6bd556865aa8cad688d4c671d19a1f5e6871 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Mon, 13 Apr 2015 12:58:47 +0200 Subject: [PATCH 06/13] use std::tuple<> to forward parameters into the Thread<> object works around GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47226 --- include/mbgl/util/thread.hpp | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/include/mbgl/util/thread.hpp b/include/mbgl/util/thread.hpp index 6983b8190..7c9ba70e1 100644 --- a/include/mbgl/util/thread.hpp +++ b/include/mbgl/util/thread.hpp @@ -4,6 +4,21 @@ #include #include +namespace { + +template <::std::size_t...> +struct index_sequence {}; + +template <::std::size_t N, ::std::size_t... I> +struct integer_sequence : integer_sequence {}; + +template <::std::size_t... I> +struct integer_sequence<0, I...> { + using type = index_sequence; +}; + +} + namespace mbgl { namespace util { @@ -29,6 +44,15 @@ public: inline Object* operator->() const { return &object; } inline operator Object*() const { return &object; } +private: + template + void run(std::promise& promise, P&& params, index_sequence) { + Object context(std::get(std::forward

(params))...); + promise.set_value(context); + context.start(); + joinable.get_future().get(); + } + private: std::thread thread; std::promise joinable; @@ -38,16 +62,16 @@ private: template template Thread::Thread(Args&&... args) - : object([&]() -> Object& { + : object([&](std::tuple&& params) -> Object& { + // Note: We're using std::tuple<> to store the arguments because GCC 4.9 has a bug + // when expanding parameters packs captured in lambdas. std::promise promise; + constexpr auto seq = typename integer_sequence::type(); thread = std::thread([&] { - Object context(::std::forward(args)...); - promise.set_value(context); - context.start(); - joinable.get_future().get(); + run(promise, std::move(params), seq); }); return promise.get_future().get(); - }()) { + }(std::move(std::forward_as_tuple(::std::forward(args)...)))) { } template From 16849a341b72c0633be1e3c89498c883d6efb000 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Mon, 13 Apr 2015 17:57:45 +0200 Subject: [PATCH 07/13] hide Thread<> and separate the Implementation object --- Makefile | 1 + android/cpp/native_map_view.cpp | 6 +- bin/render.cpp | 5 +- include/mbgl/android/native_map_view.hpp | 5 +- .../mbgl/storage/default/asset_request.hpp | 2 +- include/mbgl/storage/default/http_request.hpp | 2 +- .../storage/default/shared_request_base.hpp | 6 +- include/mbgl/storage/default/sqlite_cache.hpp | 32 +++-------- include/mbgl/storage/default_file_source.hpp | 29 ++-------- linux/main.cpp | 7 +-- macosx/main.mm | 7 +-- platform/darwin/http_request_nsurl.mm | 2 +- platform/default/asset_request_fs.cpp | 2 +- platform/default/asset_request_zip.cpp | 2 +- platform/default/http_request_curl.cpp | 2 +- platform/default/sqlite_cache.cpp | 54 ++++++++++++++---- platform/ios/MGLMapView.mm | 11 ++-- src/mbgl/storage/default_file_source.cpp | 29 ++++++---- test/headless/headless.cpp | 5 +- test/storage/cache_response.cpp | 9 ++- test/storage/cache_revalidate.cpp | 17 +++--- test/storage/database.cpp | 55 +++++++++---------- test/storage/directory_reading.cpp | 7 +-- test/storage/file_reading.cpp | 19 +++---- test/storage/http_cancel.cpp | 15 +++-- test/storage/http_coalescing.cpp | 5 +- test/storage/http_environment.cpp | 9 ++- test/storage/http_error.cpp | 7 +-- test/storage/http_header_parsing.cpp | 7 +-- test/storage/http_load.cpp | 5 +- test/storage/http_noloop.cpp | 5 +- test/storage/http_other_loop.cpp | 5 +- test/storage/http_reading.cpp | 28 ++++++---- 33 files changed, 196 insertions(+), 206 deletions(-) diff --git a/Makefile b/Makefile index e0d5102d5..434645c81 100644 --- a/Makefile +++ b/Makefile @@ -53,6 +53,7 @@ Xcode/mbgl: config/$(HOST).gypi styles/styles SMCalloutView Makefile/test: test/test.gyp config/$(HOST).gypi styles/styles SMCalloutView deps/run_gyp test/test.gyp $(CONFIG_$(HOST)) $(LIBS_$(HOST)) --generator-output=./build/$(HOST) -f make +.PHONY: test test: Makefile/test $(MAKE) -C build/$(HOST) BUILDTYPE=$(BUILDTYPE) test diff --git a/android/cpp/native_map_view.cpp b/android/cpp/native_map_view.cpp index b51e5cbc6..39a777bff 100644 --- a/android/cpp/native_map_view.cpp +++ b/android/cpp/native_map_view.cpp @@ -58,8 +58,8 @@ void log_gl_string(GLenum name, const char *label) { NativeMapView::NativeMapView(JNIEnv *env, jobject obj_) : mbgl::View(*this), fileCache(mbgl::android::cachePath + "/mbgl-cache.db"), - fileSource(fileCache), - map(*this, *fileSource) { + fileSource(&fileCache), + map(*this, fileSource) { mbgl::Log::Debug(mbgl::Event::Android, "NativeMapView::NativeMapView"); assert(env != nullptr); @@ -145,7 +145,7 @@ void NativeMapView::notify() { mbgl::Map &NativeMapView::getMap() { return map; } -mbgl::DefaultFileSource &NativeMapView::getFileSource() { return *fileSource; } +mbgl::DefaultFileSource &NativeMapView::getFileSource() { return fileSource; } bool NativeMapView::inEmulator() { // Detect if we are in emulator diff --git a/bin/render.cpp b/bin/render.cpp index 879bf840a..01f692909 100644 --- a/bin/render.cpp +++ b/bin/render.cpp @@ -2,7 +2,6 @@ #include #include #include -#include #include #include @@ -67,8 +66,8 @@ int main(int argc, char *argv[]) { using namespace mbgl; - mbgl::util::Thread cache(cache_file); - mbgl::DefaultFileSource fileSource(cache); + mbgl::SQLiteCache cache(cache_file); + mbgl::DefaultFileSource fileSource(&cache); // Try to load the token from the environment. if (!token.size()) { diff --git a/include/mbgl/android/native_map_view.hpp b/include/mbgl/android/native_map_view.hpp index 817b6bc8b..21784f531 100644 --- a/include/mbgl/android/native_map_view.hpp +++ b/include/mbgl/android/native_map_view.hpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include @@ -62,8 +61,8 @@ private: ANativeWindow *window = nullptr; - mbgl::util::Thread fileCache; - mbgl::util::Thread fileSource; + mbgl::SQLiteCache fileCache; + mbgl::DefaultFileSource fileSource; mbgl::Map map; EGLDisplay display = EGL_NO_DISPLAY; diff --git a/include/mbgl/storage/default/asset_request.hpp b/include/mbgl/storage/default/asset_request.hpp index 48f402fef..48d421c3b 100644 --- a/include/mbgl/storage/default/asset_request.hpp +++ b/include/mbgl/storage/default/asset_request.hpp @@ -7,7 +7,7 @@ namespace mbgl { class AssetRequest : public SharedRequestBase { public: - AssetRequest(DefaultFileSource *source, const Resource &resource); + AssetRequest(DefaultFileSource::Impl *source, const Resource &resource); void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); diff --git a/include/mbgl/storage/default/http_request.hpp b/include/mbgl/storage/default/http_request.hpp index 1cd38a828..54e9a77ef 100644 --- a/include/mbgl/storage/default/http_request.hpp +++ b/include/mbgl/storage/default/http_request.hpp @@ -7,7 +7,7 @@ namespace mbgl { class HTTPRequest : public SharedRequestBase { public: - HTTPRequest(DefaultFileSource *source, const Resource &resource); + HTTPRequest(DefaultFileSource::Impl *source, const Resource &resource); void start(uv_loop_t *loop, std::shared_ptr response = nullptr); void cancel(); diff --git a/include/mbgl/storage/default/shared_request_base.hpp b/include/mbgl/storage/default/shared_request_base.hpp index 9c5f87b33..efc78f603 100644 --- a/include/mbgl/storage/default/shared_request_base.hpp +++ b/include/mbgl/storage/default/shared_request_base.hpp @@ -3,7 +3,7 @@ #include #include -#include +#include #include #include #include @@ -26,7 +26,7 @@ protected: MBGL_STORE_THREAD(tid) public: - SharedRequestBase(DefaultFileSource *source_, const Resource &resource_) + SharedRequestBase(DefaultFileSource::Impl *source_, const Resource &resource_) : resource(resource_), source(source_) {} virtual void start(uv_loop_t *loop, std::shared_ptr response = nullptr) = 0; @@ -95,7 +95,7 @@ public: const Resource resource; protected: - DefaultFileSource *source = nullptr; + DefaultFileSource::Impl *source = nullptr; private: std::set observers; diff --git a/include/mbgl/storage/default/sqlite_cache.hpp b/include/mbgl/storage/default/sqlite_cache.hpp index 899cc8a89..db672b222 100644 --- a/include/mbgl/storage/default/sqlite_cache.hpp +++ b/include/mbgl/storage/default/sqlite_cache.hpp @@ -2,41 +2,27 @@ #define MBGL_STORAGE_DEFAULT_SQLITE_CACHE #include -#include #include -namespace mapbox { namespace sqlite { class Database; class Statement; } } - namespace mbgl { -class SQLiteCache : public FileCache, protected util::RunLoop { - friend class util::Thread; - -private: - SQLiteCache(const std::string &path = ":memory:"); - ~SQLiteCache(); +namespace util { +template class Thread; +} +class SQLiteCache : public FileCache { public: + SQLiteCache(const std::string &path = ":memory:"); + ~SQLiteCache() override; + // FileCache API void get(const Resource &resource, Callback callback) override; void put(const Resource &resource, std::shared_ptr response, Hint hint) override; private: - void createDatabase(); - void createSchema(); - - void processGet(const Resource& resource, Callback callback); - void processPut(const Resource& resource, std::shared_ptr response); - void processRefresh(const Resource& resource, int64_t expires); - -private: - const std::string path; - std::unique_ptr<::mapbox::sqlite::Database> db; - std::unique_ptr<::mapbox::sqlite::Statement> getStmt; - std::unique_ptr<::mapbox::sqlite::Statement> putStmt; - std::unique_ptr<::mapbox::sqlite::Statement> refreshStmt; - bool schema = false; + class Thread; + const std::unique_ptr> thread; }; } diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp index f6028217e..372dee909 100644 --- a/include/mbgl/storage/default_file_source.hpp +++ b/include/mbgl/storage/default_file_source.hpp @@ -5,22 +5,13 @@ #include #include -#include -#include - namespace mbgl { -class SharedRequestBase; - -class DefaultFileSource : public FileSource, protected util::RunLoop { - friend class util::Thread; - friend class SharedRequestBase; - -private: +class DefaultFileSource : public FileSource { +public: DefaultFileSource(FileCache *cache, const std::string &root = ""); ~DefaultFileSource() override; -public: // FileSource API Request *request(const Resource &resource, uv_loop_t *loop, const Environment &env, Callback callback) override; @@ -29,22 +20,10 @@ public: void abort(const Environment &env) override; -private: - void notify(SharedRequestBase *sharedRequest, const std::set &observers, - std::shared_ptr response, FileCache::Hint hint); - SharedRequestBase *find(const Resource &resource); - - void processAdd(Request* request); - void processCancel(Request* request); - void processResult(const Resource& resource, std::shared_ptr response); - void processAbort(const Environment& env); - public: - const std::string assetRoot; - + class Impl; private: - std::unordered_map pending; - FileCache *cache = nullptr; + const std::unique_ptr> thread; }; } diff --git a/linux/main.cpp b/linux/main.cpp index d91aa7dec..4d5474c02 100644 --- a/linux/main.cpp +++ b/linux/main.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include @@ -66,9 +65,9 @@ int main(int argc, char *argv[]) { view = mbgl::util::make_unique(); - mbgl::util::Thread cache("/tmp/mbgl-cache.db"); - mbgl::util::Thread fileSource(cache); - mbgl::Map map(*view, *fileSource); + mbgl::SQLiteCache cache("/tmp/mbgl-cache.db"); + mbgl::DefaultFileSource fileSource(&cache); + mbgl::Map map(*view, fileSource); // Load settings mbgl::Settings_JSON settings; diff --git a/macosx/main.mm b/macosx/main.mm index d0f3282f8..087544f7c 100644 --- a/macosx/main.mm +++ b/macosx/main.mm @@ -6,7 +6,6 @@ #include #include #include -#include #include @@ -104,9 +103,9 @@ const std::string &defaultCacheDatabase() { int main() { GLFWView view; - mbgl::util::Thread cache(defaultCacheDatabase()); - mbgl::util::Thread fileSource(cache); - mbgl::Map map(view, *fileSource); + mbgl::SQLiteCache cache(defaultCacheDatabase()); + mbgl::DefaultFileSource fileSource(&cache); + mbgl::Map map(view, fileSource); URLHandler *handler = [[URLHandler alloc] init]; [handler setMap:&map]; diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index 773f83e5d..bd2823476 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -397,7 +397,7 @@ void HTTPRequestImpl::restart(uv_timer_t *timer, int) { // ------------------------------------------------------------------------------------------------- -HTTPRequest::HTTPRequest(DefaultFileSource *source, const Resource &resource) +HTTPRequest::HTTPRequest(DefaultFileSource::Impl *source, const Resource &resource) : SharedRequestBase(source, resource) { } diff --git a/platform/default/asset_request_fs.cpp b/platform/default/asset_request_fs.cpp index 10d5f33a3..d2b07e0a2 100644 --- a/platform/default/asset_request_fs.cpp +++ b/platform/default/asset_request_fs.cpp @@ -207,7 +207,7 @@ void AssetRequestImpl::cleanup(uv_fs_t *req) { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(DefaultFileSource *source_, const Resource &resource_) +AssetRequest::AssetRequest(DefaultFileSource::Impl *source_, const Resource &resource_) : SharedRequestBase(source_, resource_) { assert(algo::starts_with(resource.url, "asset://")); } diff --git a/platform/default/asset_request_zip.cpp b/platform/default/asset_request_zip.cpp index 85ff0aede..10a73b99a 100644 --- a/platform/default/asset_request_zip.cpp +++ b/platform/default/asset_request_zip.cpp @@ -275,7 +275,7 @@ void AssetRequestImpl::cancel() { // ------------------------------------------------------------------------------------------------- -AssetRequest::AssetRequest(DefaultFileSource *source_, const Resource &resource_) +AssetRequest::AssetRequest(DefaultFileSource::Impl *source_, const Resource &resource_) : SharedRequestBase(source_, resource_) { assert(algo::starts_with(resource.url, "asset://")); } diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index 707465a3d..e13ccbfb2 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -722,7 +722,7 @@ void HTTPRequestImpl::handleResult(CURLcode code) { // ------------------------------------------------------------------------------------------------- -HTTPRequest::HTTPRequest(DefaultFileSource *source_, const Resource &resource_) +HTTPRequest::HTTPRequest(DefaultFileSource::Impl *source_, const Resource &resource_) : SharedRequestBase(source_, resource_) { } diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index b084a7f2a..94a8f072c 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -4,6 +4,8 @@ #include #include +#include +#include #include #include "sqlite3.hpp" @@ -11,6 +13,33 @@ namespace mbgl { +class SQLiteCache::Thread : public util::RunLoop { + friend class util::Thread; + +public: + Thread(const std::string &path = ":memory:"); + ~Thread(); + +public: + void processGet(const Resource& resource, Callback callback); + void processPut(const Resource& resource, std::shared_ptr response); + void processRefresh(const Resource& resource, int64_t expires); + +private: + void createDatabase(); + void createSchema(); + +private: + const std::string path; + std::unique_ptr<::mapbox::sqlite::Database> db; + std::unique_ptr<::mapbox::sqlite::Statement> getStmt; + std::unique_ptr<::mapbox::sqlite::Statement> putStmt; + std::unique_ptr<::mapbox::sqlite::Statement> refreshStmt; + bool schema = false; +}; + + + std::string removeAccessTokenFromURL(const std::string &url) { const size_t token_start = url.find("access_token="); // Ensure that token exists, isn't at the front and is preceded by either & or ?. @@ -60,14 +89,19 @@ std::string unifyMapboxURLs(const std::string &url) { using namespace mapbox::sqlite; -SQLiteCache::SQLiteCache(const std::string& path_) +SQLiteCache::SQLiteCache(const std::string& path_) : thread(util::make_unique>(path_)) { +} + +SQLiteCache::~SQLiteCache() = default; + +SQLiteCache::Thread::Thread(const std::string& path_) : path(path_) { #ifdef __APPLE__ pthread_setname_np("SQLite Cache"); #endif } -SQLiteCache::~SQLiteCache() { +SQLiteCache::Thread::~Thread() { // Deleting these SQLite objects may result in exceptions, but we're in a destructor, so we // can't throw anything. try { @@ -80,11 +114,11 @@ SQLiteCache::~SQLiteCache() { } } -void SQLiteCache::createDatabase() { +void SQLiteCache::Thread::createDatabase() { db = util::make_unique(path.c_str(), ReadWrite | Create); } -void SQLiteCache::createSchema() { +void SQLiteCache::Thread::createSchema() { constexpr const char *const sql = "" "CREATE TABLE IF NOT EXISTS `http_cache` (" " `url` TEXT PRIMARY KEY NOT NULL," @@ -127,10 +161,10 @@ void SQLiteCache::get(const Resource &resource, Callback callback) { // Will try to load the URL from the SQLite database and call the callback when done. // Note that the callback is probably going to invoked from another thread, so the caller // must make sure that it can run in that thread. - invoke(std::bind(&SQLiteCache::processGet, this, resource, callback)); + (*thread)->invoke([=] { (*thread)->processGet(resource, callback); }); } -void SQLiteCache::processGet(const Resource &resource, Callback callback) { +void SQLiteCache::Thread::processGet(const Resource &resource, Callback callback) { try { // This is called in the SQLite event loop. if (!db) { @@ -179,13 +213,13 @@ void SQLiteCache::put(const Resource &resource, std::shared_ptr // storing a new response or updating the currently stored response, potentially setting a new // expiry date. if (hint == Hint::Full) { - invoke(std::bind(&SQLiteCache::processPut, this, resource, response)); + (*thread)->invoke([=] { (*thread)->processPut(resource, response); }); } else if (hint == Hint::Refresh) { - invoke(std::bind(&SQLiteCache::processRefresh, this, resource, response->expires)); + (*thread)->invoke([=] { (*thread)->processRefresh(resource, response->expires); }); } } -void SQLiteCache::processPut(const Resource& resource, std::shared_ptr response) { +void SQLiteCache::Thread::processPut(const Resource& resource, std::shared_ptr response) { try { if (!db) { createDatabase(); @@ -234,7 +268,7 @@ void SQLiteCache::processPut(const Resource& resource, std::shared_ptr #include #include -#include #import "MGLTypes.h" #import "NSString+MGLAdditions.h" @@ -112,8 +111,8 @@ std::chrono::steady_clock::duration secondsAsDuration(float duration) mbgl::Map *mbglMap = nullptr; MBGLView *mbglView = nullptr; -mbgl::util::Thread *mbglFileCache = nullptr; -mbgl::util::Thread *mbglFileSource = nullptr; +mbgl::SQLiteCache *mbglFileCache = nullptr; +mbgl::DefaultFileSource *mbglFileSource = nullptr; - (instancetype)initWithFrame:(CGRect)frame { @@ -270,9 +269,9 @@ mbgl::util::Thread *mbglFileSource = nullptr; // setup mbgl map // mbglView = new MBGLView(self); - mbglFileCache = new mbgl::util::Thread(defaultCacheDatabase()); - mbglFileSource = new mbgl::util::Thread(*mbglFileCache); - mbglMap = new mbgl::Map(*mbglView, **mbglFileSource); + mbglFileCache = new mbgl::SQLiteCache(defaultCacheDatabase()); + mbglFileSource = new mbgl::DefaultFileSource(mbglFileCache); + mbglMap = new mbgl::Map(*mbglView, *mbglFileSource); mbglView->resize(self.bounds.size.width, self.bounds.size.height, _glView.contentScaleFactor, _glView.drawableWidth, _glView.drawableHeight); // Notify map object when network reachability status changes. diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index 71b3be850..ee4042459 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include #include @@ -8,6 +8,8 @@ #include #include +#include +#include #include #pragma GCC diagnostic push @@ -18,7 +20,6 @@ #include #pragma GCC diagnostic pop -#include #include #include @@ -27,18 +28,22 @@ namespace algo = boost::algorithm; namespace mbgl { -DefaultFileSource::DefaultFileSource(FileCache* cache_, const std::string& root) +DefaultFileSource::Impl::Impl(FileCache* cache_, const std::string& root) : assetRoot(root.empty() ? platform::assetRoot() : root), cache(cache_) { #ifdef __APPLE__ pthread_setname_np("FileSource"); #endif } +DefaultFileSource::DefaultFileSource(FileCache* cache, const std::string& root) + : thread(util::make_unique>(cache, root)) { +} + DefaultFileSource::~DefaultFileSource() { MBGL_VERIFY_THREAD(tid); } -SharedRequestBase *DefaultFileSource::find(const Resource &resource) { +SharedRequestBase *DefaultFileSource::Impl::find(const Resource &resource) { // We're using a set of pointers here instead of a map between url and SharedRequestBase because // we need to find the requests both by pointer and by URL. Given that the number of requests // is generally very small (typically < 10 at a time), hashing by URL incurs too much overhead @@ -58,7 +63,7 @@ Request* DefaultFileSource::request(const Resource& resource, // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. - invoke([this, req] { processAdd(req); }); + (*thread)->invoke([=] { (*thread)->processAdd(req); }); return req; } @@ -72,14 +77,14 @@ void DefaultFileSource::cancel(Request *req) { // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. - invoke([this, req] { processCancel(req); }); + (*thread)->invoke([=] { (*thread)->processCancel(req); }); } void DefaultFileSource::abort(const Environment &env) { - invoke([this, &env] { processAbort(env); }); + (*thread)->invoke([=, &env] { (*thread)->processAbort(env); }); } -void DefaultFileSource::processAdd(Request* req) { +void DefaultFileSource::Impl::processAdd(Request* req) { const Resource &resource = req->resource; // We're adding a new Request. @@ -111,7 +116,7 @@ void DefaultFileSource::processAdd(Request* req) { sharedRequest->subscribe(req); } -void DefaultFileSource::processCancel(Request* req) { +void DefaultFileSource::Impl::processCancel(Request* req) { SharedRequestBase *sharedRequest = find(req->resource); if (sharedRequest) { // If the number of dependent requests of the SharedRequestBase drops to zero, the @@ -128,7 +133,7 @@ void DefaultFileSource::processCancel(Request* req) { req->destruct(); } -void DefaultFileSource::processResult(const Resource& resource, std::shared_ptr response) { +void DefaultFileSource::Impl::processResult(const Resource& resource, std::shared_ptr response) { SharedRequestBase *sharedRequest = find(resource); if (sharedRequest) { if (response) { @@ -155,7 +160,7 @@ void DefaultFileSource::processResult(const Resource& resource, std::shared_ptr< } // Aborts all requests that are part of the current environment. -void DefaultFileSource::processAbort(const Environment& env) { +void DefaultFileSource::Impl::processAbort(const Environment& env) { // Construct a cancellation response. auto res = util::make_unique(); res->status = Response::Error; @@ -182,7 +187,7 @@ void DefaultFileSource::processAbort(const Environment& env) { }); } -void DefaultFileSource::notify(SharedRequestBase *sharedRequest, +void DefaultFileSource::Impl::notify(SharedRequestBase *sharedRequest, const std::set &observers, std::shared_ptr response, FileCache::Hint hint) { // First, remove the request, since it might be destructed at any point now. diff --git a/test/headless/headless.cpp b/test/headless/headless.cpp index 76c126433..f4c8accf6 100644 --- a/test/headless/headless.cpp +++ b/test/headless/headless.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include @@ -140,8 +139,8 @@ TEST_P(HeadlessTest, render) { } HeadlessView view(display); - util::Thread fileSource(nullptr); - Map map(view, *fileSource); + DefaultFileSource fileSource(nullptr); + Map map(view, fileSource); map.setClasses(classes); map.setStyleJSON(style, "test/suite"); diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index 46083e2f9..e37899a86 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -4,20 +4,19 @@ #include #include -#include TEST_F(Storage, CacheResponse) { SCOPED_TEST(CacheResponse); using namespace mbgl; - util::Thread cache(":memory:"); - util::Thread fs(cache); + SQLiteCache cache(":memory:"); + DefaultFileSource fs(&cache); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/cache" }; auto &env = *static_cast(nullptr); - fs->request(resource, uv_default_loop(), env, [&](const Response &res) { + fs.request(resource, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response 1", res.data); EXPECT_LT(0, res.expires); @@ -25,7 +24,7 @@ TEST_F(Storage, CacheResponse) { EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); - fs->request(resource, uv_default_loop(), env, [&, res](const Response &res2) { + fs.request(resource, uv_default_loop(), env, [&, res](const Response &res2) { EXPECT_EQ(res.status, res2.status); EXPECT_EQ(res.data, res2.data); EXPECT_EQ(res.expires, res2.expires); diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp index 901c151f1..bd32042b9 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -4,7 +4,6 @@ #include #include -#include TEST_F(Storage, CacheRevalidate) { SCOPED_TEST(CacheRevalidateSame) @@ -13,13 +12,13 @@ TEST_F(Storage, CacheRevalidate) { using namespace mbgl; - util::Thread cache(":memory:"); - util::Thread fs(cache); + SQLiteCache cache(":memory:"); + DefaultFileSource fs(&cache); auto &env = *static_cast(nullptr); const Resource revalidateSame { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" }; - fs->request(revalidateSame, uv_default_loop(), env, [&](const Response &res) { + fs.request(revalidateSame, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); @@ -27,7 +26,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("snowfall", res.etag); EXPECT_EQ("", res.message); - fs->request(revalidateSame, uv_default_loop(), env, [&, res](const Response &res2) { + fs.request(revalidateSame, uv_default_loop(), env, [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. @@ -43,7 +42,7 @@ TEST_F(Storage, CacheRevalidate) { const Resource revalidateModified{ Resource::Unknown, "http://127.0.0.1:3000/revalidate-modified" }; - fs->request(revalidateModified, uv_default_loop(), env, [&](const Response &res) { + fs.request(revalidateModified, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response", res.data); EXPECT_EQ(0, res.expires); @@ -51,7 +50,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("", res.etag); EXPECT_EQ("", res.message); - fs->request(revalidateModified, uv_default_loop(), env, [&, res](const Response &res2) { + fs.request(revalidateModified, uv_default_loop(), env, [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response", res2.data); // We use this to indicate that a 304 reply came back. @@ -65,7 +64,7 @@ TEST_F(Storage, CacheRevalidate) { }); const Resource revalidateEtag { Resource::Unknown, "http://127.0.0.1:3000/revalidate-etag" }; - fs->request(revalidateEtag, uv_default_loop(), env, [&](const Response &res) { + fs.request(revalidateEtag, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Response 1", res.data); EXPECT_EQ(0, res.expires); @@ -73,7 +72,7 @@ TEST_F(Storage, CacheRevalidate) { EXPECT_EQ("response-1", res.etag); EXPECT_EQ("", res.message); - fs->request(revalidateEtag, uv_default_loop(), env, [&, res](const Response &res2) { + fs.request(revalidateEtag, uv_default_loop(), env, [&, res](const Response &res2) { EXPECT_EQ(Response::Successful, res2.status); EXPECT_EQ("Response 2", res2.data); EXPECT_EQ(0, res2.expires); diff --git a/test/storage/database.cpp b/test/storage/database.cpp index ffa82ecb9..02b25d696 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include @@ -37,14 +36,14 @@ TEST_F(Storage, DatabaseDoesNotExist) { Log::setObserver(util::make_unique()); + SQLiteCache cache("test/fixtures/404/cache.db"); + ScopedTest test([&] { auto observer = Log::removeObserver(); EXPECT_EQ(1ul, dynamic_cast(observer.get())->count({ EventSeverity::Error, Event::Database, 14, "unable to open database file" })); }); - util::Thread cache("test/fixtures/404/cache.db"); - - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); test.finish(); }); @@ -82,13 +81,13 @@ TEST_F(Storage, DatabaseCreate) { Log::setObserver(util::make_unique()); + SQLiteCache cache("test/fixtures/database/cache.db"); + ScopedTest test([&] { Log::removeObserver(); }); - util::Thread cache("test/fixtures/database/cache.db"); - - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); test.finish(); }); @@ -142,7 +141,7 @@ TEST_F(Storage, DatabaseLockedRead) { FileLock guard("test/fixtures/database/locked.db"); - util::Thread cache("test/fixtures/database/locked.db"); + SQLiteCache cache("test/fixtures/database/locked.db"); std::promise promise; @@ -150,7 +149,7 @@ TEST_F(Storage, DatabaseLockedRead) { // First request should fail. Log::setObserver(util::make_unique()); promise = {}; - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); promise.set_value(); }); @@ -170,7 +169,7 @@ TEST_F(Storage, DatabaseLockedRead) { Log::setObserver(util::make_unique()); promise = {}; - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); promise.set_value(); }); @@ -191,7 +190,7 @@ TEST_F(Storage, DatabaseLockedWrite) { deleteFile("test/fixtures/database/locked.db"); FileLock guard("test/fixtures/database/locked.db"); - util::Thread cache("test/fixtures/database/locked.db"); + SQLiteCache cache("test/fixtures/database/locked.db"); std::promise promise; @@ -200,8 +199,8 @@ TEST_F(Storage, DatabaseLockedWrite) { Log::setObserver(util::make_unique()); promise = {}; auto response = std::make_shared(); - cache->put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); promise.set_value(); }); @@ -222,8 +221,8 @@ TEST_F(Storage, DatabaseLockedWrite) { auto response = std::make_shared(); response->data = "Demo"; - cache->put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_NE(nullptr, res.get()); EXPECT_EQ("Demo", res->data); promise.set_value(); @@ -245,7 +244,7 @@ TEST_F(Storage, DatabaseLockedRefresh) { createDir("test/fixtures/database"); deleteFile("test/fixtures/database/locked.db"); - util::Thread cache("test/fixtures/database/locked.db"); + SQLiteCache cache("test/fixtures/database/locked.db"); // Then, lock the file and try again. FileLock guard("test/fixtures/database/locked.db"); @@ -258,8 +257,8 @@ TEST_F(Storage, DatabaseLockedRefresh) { promise = {}; auto response = std::make_shared(); response->data = "Demo"; - cache->put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); promise.set_value(); }); @@ -277,8 +276,8 @@ TEST_F(Storage, DatabaseLockedRefresh) { auto response = std::make_shared(); response->data = "Demo"; - cache->put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Refresh); - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Refresh); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_EQ(nullptr, res.get()); promise.set_value(); }); @@ -300,7 +299,7 @@ TEST_F(Storage, DatabaseDeleted) { createDir("test/fixtures/database"); deleteFile("test/fixtures/database/locked.db"); - util::Thread cache("test/fixtures/database/locked.db"); + SQLiteCache cache("test/fixtures/database/locked.db"); std::promise promise; @@ -310,8 +309,8 @@ TEST_F(Storage, DatabaseDeleted) { promise = {}; auto response = std::make_shared(); response->data = "Demo"; - cache->put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_NE(nullptr, res.get()); EXPECT_EQ("Demo", res->data); promise.set_value(); @@ -329,8 +328,8 @@ TEST_F(Storage, DatabaseDeleted) { promise = {}; auto response = std::make_shared(); response->data = "Demo"; - cache->put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_NE(nullptr, res.get()); EXPECT_EQ("Demo", res->data); promise.set_value(); @@ -353,7 +352,7 @@ TEST_F(Storage, DatabaseInvalid) { deleteFile("test/fixtures/database/invalid.db"); writeFile("test/fixtures/database/invalid.db", "this is an invalid file"); - util::Thread cache("test/fixtures/database/invalid.db"); + SQLiteCache cache("test/fixtures/database/invalid.db"); std::promise promise; @@ -363,8 +362,8 @@ TEST_F(Storage, DatabaseInvalid) { promise = {}; auto response = std::make_shared(); response->data = "Demo"; - cache->put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache->get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { EXPECT_NE(nullptr, res.get()); EXPECT_EQ("Demo", res->data); promise.set_value(); diff --git a/test/storage/directory_reading.cpp b/test/storage/directory_reading.cpp index 4264129b2..ccae4177c 100644 --- a/test/storage/directory_reading.cpp +++ b/test/storage/directory_reading.cpp @@ -3,7 +3,6 @@ #include #include -#include TEST_F(Storage, AssetReadDirectory) { SCOPED_TEST(ReadDirectory) @@ -11,14 +10,14 @@ TEST_F(Storage, AssetReadDirectory) { using namespace mbgl; #ifdef MBGL_ASSET_ZIP - util::Thread fs(nullptr, "test/fixtures/storage/assets.zip"); + DefaultFileSource fs(nullptr, "test/fixtures/storage/assets.zip"); #else - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); #endif auto &env = *static_cast(nullptr); - fs->request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(), + fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(0ul, res.data.size()); diff --git a/test/storage/file_reading.cpp b/test/storage/file_reading.cpp index de834798d..01db75d5c 100644 --- a/test/storage/file_reading.cpp +++ b/test/storage/file_reading.cpp @@ -4,7 +4,6 @@ #include #include -#include TEST_F(Storage, AssetEmptyFile) { SCOPED_TEST(EmptyFile) @@ -12,14 +11,14 @@ TEST_F(Storage, AssetEmptyFile) { using namespace mbgl; #ifdef MBGL_ASSET_ZIP - util::Thread fs(nullptr, "test/fixtures/storage/assets.zip"); + DefaultFileSource fs(nullptr, "test/fixtures/storage/assets.zip"); #else - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); #endif auto &env = *static_cast(nullptr); - fs->request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(), + fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/empty" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(0ul, res.data.size()); @@ -39,14 +38,14 @@ TEST_F(Storage, AssetNonEmptyFile) { using namespace mbgl; #ifdef MBGL_ASSET_ZIP - util::Thread fs(nullptr, "test/fixtures/storage/assets.zip"); + DefaultFileSource fs(nullptr, "test/fixtures/storage/assets.zip"); #else - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); #endif auto &env = *static_cast(nullptr); - fs->request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" }, + fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/nonempty" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ(16ul, res.data.size()); @@ -67,14 +66,14 @@ TEST_F(Storage, AssetNonExistentFile) { using namespace mbgl; #ifdef MBGL_ASSET_ZIP - util::Thread fs(nullptr, "test/fixtures/storage/assets.zip"); + DefaultFileSource fs(nullptr, "test/fixtures/storage/assets.zip"); #else - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); #endif auto &env = *static_cast(nullptr); - fs->request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" }, + fs.request({ Resource::Unknown, "asset://TEST_DATA/fixtures/storage/does_not_exist" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Error, res.status); EXPECT_EQ(0ul, res.data.size()); diff --git a/test/storage/http_cancel.cpp b/test/storage/http_cancel.cpp index 33edbdf7c..80efb3977 100644 --- a/test/storage/http_cancel.cpp +++ b/test/storage/http_cancel.cpp @@ -4,7 +4,6 @@ #include #include -#include #include @@ -13,15 +12,15 @@ TEST_F(Storage, HTTPCancel) { using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); auto req = - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, [&](const Response &) { ADD_FAILURE() << "Callback should not be called"; }); - fs->cancel(req); + fs.cancel(req); HTTPCancel.finish(); uv_run(uv_default_loop(), UV_RUN_DEFAULT); @@ -32,15 +31,15 @@ TEST_F(Storage, HTTPCancelMultiple) { using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; - auto req2 = fs->request(resource, uv_default_loop(), env, [&](const Response &) { + auto req2 = fs.request(resource, uv_default_loop(), env, [&](const Response &) { ADD_FAILURE() << "Callback should not be called"; }); - fs->request(resource, uv_default_loop(), env, [&](const Response &res) { + fs.request(resource, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); EXPECT_EQ(0, res.expires); @@ -49,7 +48,7 @@ TEST_F(Storage, HTTPCancelMultiple) { EXPECT_EQ("", res.message); HTTPCancelMultiple.finish(); }); - fs->cancel(req2); + fs.cancel(req2); uv_run(uv_default_loop(), UV_RUN_DEFAULT); } diff --git a/test/storage/http_coalescing.cpp b/test/storage/http_coalescing.cpp index 5164117f5..28fa4415b 100644 --- a/test/storage/http_coalescing.cpp +++ b/test/storage/http_coalescing.cpp @@ -3,7 +3,6 @@ #include #include -#include TEST_F(Storage, HTTPCoalescing) { SCOPED_TEST(HTTPCoalescing) @@ -13,7 +12,7 @@ TEST_F(Storage, HTTPCoalescing) { using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); @@ -44,7 +43,7 @@ TEST_F(Storage, HTTPCoalescing) { const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/test" }; for (int i = 0; i < total; i++) { - fs->request(resource, uv_default_loop(), env, complete); + fs.request(resource, uv_default_loop(), env, complete); } uv_run(uv_default_loop(), UV_RUN_DEFAULT); diff --git a/test/storage/http_environment.cpp b/test/storage/http_environment.cpp index b08a8071c..41a9d43d5 100644 --- a/test/storage/http_environment.cpp +++ b/test/storage/http_environment.cpp @@ -4,7 +4,6 @@ #include #include -#include #include @@ -14,7 +13,7 @@ TEST_F(Storage, HTTPCancelEnvironment) { using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); // Create two fake environment pointers. The FileSource implementation treats these as opaque // pointers and doesn't reach into them. @@ -24,7 +23,7 @@ TEST_F(Storage, HTTPCancelEnvironment) { const Resource resource { Resource::Unknown, "http://127.0.0.1:3000/delayed" }; // Environment 1 - fs->request(resource, uv_default_loop(), env1, [&](const Response &res) { + fs.request(resource, uv_default_loop(), env1, [&](const Response &res) { // This environment gets aborted below. This means the request is marked as failing and // will return an error here. EXPECT_EQ(Response::Error, res.status); @@ -37,7 +36,7 @@ TEST_F(Storage, HTTPCancelEnvironment) { }); // Environment 2 - fs->request(resource, uv_default_loop(), env2, [&](const Response &res) { + fs.request(resource, uv_default_loop(), env2, [&](const Response &res) { // The same request as above, but in a different environment which doesn't get aborted. This // means the request should succeed. EXPECT_EQ(Response::Successful, res.status); @@ -49,7 +48,7 @@ TEST_F(Storage, HTTPCancelEnvironment) { HTTPRetainedEnvironment.finish(); }); - fs->abort(env1); + fs.abort(env1); uv_run(uv_default_loop(), UV_RUN_DEFAULT); } diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp index bd5ef9811..e5728d97b 100644 --- a/test/storage/http_error.cpp +++ b/test/storage/http_error.cpp @@ -4,7 +4,6 @@ #include #include -#include #include @@ -21,13 +20,13 @@ TEST_F(Storage, HTTPError) { }, 500, 500); uv_unref((uv_handle_t *)&statusChange); - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); auto start = uv_hrtime(); - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(), + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, uv_default_loop(), env, [&](const Response &res) { const auto duration = double(uv_hrtime() - start) / 1e9; EXPECT_LT(1, duration) << "Backoff timer didn't wait 1 second"; @@ -42,7 +41,7 @@ TEST_F(Storage, HTTPError) { HTTPTemporaryError.finish(); }); - fs->request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3001/" }, uv_default_loop(), env, [&](const Response &res) { const auto duration = double(uv_hrtime() - start) / 1e9; // 1.5 seconds == 4 retries, with a 500ms timeout (see above). diff --git a/test/storage/http_header_parsing.cpp b/test/storage/http_header_parsing.cpp index df1fb1fc6..1561660b6 100644 --- a/test/storage/http_header_parsing.cpp +++ b/test/storage/http_header_parsing.cpp @@ -4,7 +4,6 @@ #include #include -#include #include @@ -14,11 +13,11 @@ TEST_F(Storage, HTTPHeaderParsing) { using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); - fs->request({ Resource::Unknown, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?modified=1420794326&expires=1420797926&etag=foo" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); @@ -33,7 +32,7 @@ TEST_F(Storage, HTTPHeaderParsing) { int64_t now = std::chrono::duration_cast( SystemClock::now().time_since_epoch()).count(); - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" }, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test?cachecontrol=max-age=120" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); diff --git a/test/storage/http_load.cpp b/test/storage/http_load.cpp index b03786723..c4069eba3 100644 --- a/test/storage/http_load.cpp +++ b/test/storage/http_load.cpp @@ -3,14 +3,13 @@ #include #include -#include TEST_F(Storage, HTTPLoad) { SCOPED_TEST(HTTPLoad) using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); @@ -20,7 +19,7 @@ TEST_F(Storage, HTTPLoad) { std::function req = [&]() { const auto current = number++; - fs->request({ Resource::Unknown, + fs.request({ Resource::Unknown, std::string("http://127.0.0.1:3000/load/") + std::to_string(current) }, uv_default_loop(), env, [&, current](const Response &res) { EXPECT_EQ(Response::Successful, res.status); diff --git a/test/storage/http_noloop.cpp b/test/storage/http_noloop.cpp index 0f75c106f..cd4ebeb2c 100644 --- a/test/storage/http_noloop.cpp +++ b/test/storage/http_noloop.cpp @@ -4,14 +4,13 @@ #include #include -#include TEST_F(Storage, HTTPNoLoop) { SCOPED_TEST(HTTPNoLoop) using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); @@ -23,7 +22,7 @@ TEST_F(Storage, HTTPNoLoop) { uv::close(as); }); - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, nullptr, env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/temporary-error" }, nullptr, env, [&](const Response &res) { EXPECT_NE(uv_thread_self(), mainThread) << "Response was called in the same thread"; EXPECT_EQ(Response::Successful, res.status); diff --git a/test/storage/http_other_loop.cpp b/test/storage/http_other_loop.cpp index 11fa2f324..9ad673cf7 100644 --- a/test/storage/http_other_loop.cpp +++ b/test/storage/http_other_loop.cpp @@ -3,7 +3,6 @@ #include #include -#include TEST_F(Storage, HTTPOtherLoop) { SCOPED_TEST(HTTPOtherLoop) @@ -11,11 +10,11 @@ TEST_F(Storage, HTTPOtherLoop) { using namespace mbgl; // This file source launches a separate thread to do the processing. - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(Response::Successful, res.status); EXPECT_EQ("Hello World!", res.data); diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp index 1ba34801c..350a8eaa4 100644 --- a/test/storage/http_reading.cpp +++ b/test/storage/http_reading.cpp @@ -3,7 +3,8 @@ #include #include -#include + +#include TEST_F(Storage, HTTPReading) { SCOPED_TEST(HTTPTest) @@ -11,13 +12,13 @@ TEST_F(Storage, HTTPReading) { using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); const auto mainThread = uv_thread_self(); - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Successful, res.status); @@ -29,7 +30,7 @@ TEST_F(Storage, HTTPReading) { HTTPTest.finish(); }); - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(), + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/doesnotexist" }, uv_default_loop(), env, [&](const Response &res) { EXPECT_EQ(uv_thread_self(), mainThread); EXPECT_EQ(Response::Error, res.status); @@ -48,11 +49,11 @@ TEST_F(Storage, HTTPNoCallback) { using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); auto &env = *static_cast(nullptr); - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, uv_default_loop(), env, nullptr); uv_run(uv_default_loop(), UV_RUN_DEFAULT); @@ -60,18 +61,21 @@ TEST_F(Storage, HTTPNoCallback) { HTTPTest.finish(); } -TEST_F(Storage, HTTPNoCallbackNoLoop) { - SCOPED_TEST(HTTPTest) - +TEST_F(Storage, HTTPCallbackNotOnLoop) { using namespace mbgl; - util::Thread fs(nullptr); + DefaultFileSource fs(nullptr); + + SCOPED_TEST(HTTPTest) auto &env = *static_cast(nullptr); - fs->request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, env, nullptr); + std::promise promise; + fs.request({ Resource::Unknown, "http://127.0.0.1:3000/test" }, env, [&] (const Response &) { + promise.set_value(); + }); - uv_run(uv_default_loop(), UV_RUN_DEFAULT); + promise.get_future().get(); HTTPTest.finish(); } From 452d12e6e653d61c2182f4464d4365f0b96c9739 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Mon, 13 Apr 2015 18:28:18 +0200 Subject: [PATCH 08/13] make implementation files private headers --- bin/render.cpp | 2 +- gyp/asset-fs.gypi | 1 + gyp/asset-zip.gypi | 1 + gyp/http-curl.gypi | 1 + gyp/http-nsurl.gypi | 1 + include/mbgl/android/native_map_view.hpp | 2 +- include/mbgl/storage/default_file_source.hpp | 5 ++- .../storage/{default => }/sqlite_cache.hpp | 0 linux/main.cpp | 2 +- macosx/main.mm | 2 +- platform/darwin/http_request_nsurl.mm | 4 +- platform/default/asset_request_fs.cpp | 2 +- platform/default/asset_request_zip.cpp | 4 +- platform/default/http_request_curl.cpp | 4 +- platform/default/sqlite_cache.cpp | 4 +- platform/ios/MGLMapView.mm | 2 +- .../mbgl/storage}/asset_request.hpp | 0 src/mbgl/storage/default_file_source.cpp | 7 ++-- src/mbgl/storage/default_file_source_impl.hpp | 40 +++++++++++++++++++ .../mbgl/storage}/http_context.hpp | 0 .../mbgl/storage}/http_request.hpp | 0 src/mbgl/storage/request.cpp | 2 +- .../default => src/mbgl/storage}/request.hpp | 0 .../mbgl/storage}/shared_request_base.hpp | 2 +- .../mbgl/storage}/thread_context.hpp | 0 {include => src}/mbgl/util/run_loop.hpp | 0 {include => src}/mbgl/util/thread.hpp | 0 test/storage/cache_response.cpp | 2 +- test/storage/cache_revalidate.cpp | 2 +- test/storage/database.cpp | 2 +- 30 files changed, 70 insertions(+), 24 deletions(-) rename include/mbgl/storage/{default => }/sqlite_cache.hpp (100%) rename {include/mbgl/storage/default => src/mbgl/storage}/asset_request.hpp (100%) create mode 100644 src/mbgl/storage/default_file_source_impl.hpp rename {include/mbgl/storage/default => src/mbgl/storage}/http_context.hpp (100%) rename {include/mbgl/storage/default => src/mbgl/storage}/http_request.hpp (100%) rename {include/mbgl/storage/default => src/mbgl/storage}/request.hpp (100%) rename {include/mbgl/storage/default => src/mbgl/storage}/shared_request_base.hpp (98%) rename {include/mbgl/storage/default => src/mbgl/storage}/thread_context.hpp (100%) rename {include => src}/mbgl/util/run_loop.hpp (100%) rename {include => src}/mbgl/util/thread.hpp (100%) diff --git a/bin/render.cpp b/bin/render.cpp index 01f692909..440256b38 100644 --- a/bin/render.cpp +++ b/bin/render.cpp @@ -7,7 +7,7 @@ #include #include #include -#include +#include #pragma GCC diagnostic push #ifndef __clang__ diff --git a/gyp/asset-fs.gypi b/gyp/asset-fs.gypi index f8ec0e355..f7fbea1c7 100644 --- a/gyp/asset-fs.gypi +++ b/gyp/asset-fs.gypi @@ -12,6 +12,7 @@ 'include_dirs': [ '../include', + '../src', ], 'variables': { diff --git a/gyp/asset-zip.gypi b/gyp/asset-zip.gypi index 5c57aa18b..8a195649f 100644 --- a/gyp/asset-zip.gypi +++ b/gyp/asset-zip.gypi @@ -13,6 +13,7 @@ 'include_dirs': [ '../include', + '../src', ], 'variables': { diff --git a/gyp/http-curl.gypi b/gyp/http-curl.gypi index c97ad370b..b7c583235 100644 --- a/gyp/http-curl.gypi +++ b/gyp/http-curl.gypi @@ -12,6 +12,7 @@ 'include_dirs': [ '../include', + '../src', ], 'variables': { diff --git a/gyp/http-nsurl.gypi b/gyp/http-nsurl.gypi index 5a079fdee..a64c8508e 100644 --- a/gyp/http-nsurl.gypi +++ b/gyp/http-nsurl.gypi @@ -12,6 +12,7 @@ 'include_dirs': [ '../include', + '../src', ], 'variables': { diff --git a/include/mbgl/android/native_map_view.hpp b/include/mbgl/android/native_map_view.hpp index 21784f531..14ccaba3f 100644 --- a/include/mbgl/android/native_map_view.hpp +++ b/include/mbgl/android/native_map_view.hpp @@ -4,7 +4,7 @@ #include #include #include -#include +#include #include #include diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp index 372dee909..f39374716 100644 --- a/include/mbgl/storage/default_file_source.hpp +++ b/include/mbgl/storage/default_file_source.hpp @@ -3,10 +3,13 @@ #include #include -#include namespace mbgl { +namespace util { +template class Thread; +} + class DefaultFileSource : public FileSource { public: DefaultFileSource(FileCache *cache, const std::string &root = ""); diff --git a/include/mbgl/storage/default/sqlite_cache.hpp b/include/mbgl/storage/sqlite_cache.hpp similarity index 100% rename from include/mbgl/storage/default/sqlite_cache.hpp rename to include/mbgl/storage/sqlite_cache.hpp diff --git a/linux/main.cpp b/linux/main.cpp index 4d5474c02..6afa3f7f6 100644 --- a/linux/main.cpp +++ b/linux/main.cpp @@ -6,7 +6,7 @@ #include #include #include -#include +#include #include #include diff --git a/macosx/main.mm b/macosx/main.mm index 087544f7c..add763189 100644 --- a/macosx/main.mm +++ b/macosx/main.mm @@ -4,7 +4,7 @@ #include #include #include -#include +#include #include #include diff --git a/platform/darwin/http_request_nsurl.mm b/platform/darwin/http_request_nsurl.mm index bd2823476..0fd4a4e8f 100644 --- a/platform/darwin/http_request_nsurl.mm +++ b/platform/darwin/http_request_nsurl.mm @@ -1,5 +1,5 @@ -#include -#include +#include +#include #include #include diff --git a/platform/default/asset_request_fs.cpp b/platform/default/asset_request_fs.cpp index d2b07e0a2..a7d813b72 100644 --- a/platform/default/asset_request_fs.cpp +++ b/platform/default/asset_request_fs.cpp @@ -1,4 +1,4 @@ -#include +#include #include #include #include diff --git a/platform/default/asset_request_zip.cpp b/platform/default/asset_request_zip.cpp index 10a73b99a..22d426c76 100644 --- a/platform/default/asset_request_zip.cpp +++ b/platform/default/asset_request_zip.cpp @@ -1,5 +1,5 @@ -#include -#include +#include +#include #include #include #include diff --git a/platform/default/http_request_curl.cpp b/platform/default/http_request_curl.cpp index e13ccbfb2..1672c9cdb 100644 --- a/platform/default/http_request_curl.cpp +++ b/platform/default/http_request_curl.cpp @@ -1,5 +1,5 @@ -#include -#include +#include +#include #include #include #include diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index 94a8f072c..fcd6c09be 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -1,5 +1,5 @@ -#include -#include +#include +#include #include #include diff --git a/platform/ios/MGLMapView.mm b/platform/ios/MGLMapView.mm index f70f9f348..68fae64fa 100644 --- a/platform/ios/MGLMapView.mm +++ b/platform/ios/MGLMapView.mm @@ -11,7 +11,7 @@ #include #include #include -#include +#include #include #include diff --git a/include/mbgl/storage/default/asset_request.hpp b/src/mbgl/storage/asset_request.hpp similarity index 100% rename from include/mbgl/storage/default/asset_request.hpp rename to src/mbgl/storage/asset_request.hpp diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index ee4042459..b28e09641 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -1,7 +1,7 @@ #include -#include -#include -#include +#include +#include +#include #include #include @@ -9,7 +9,6 @@ #include #include #include -#include #include #pragma GCC diagnostic push diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp new file mode 100644 index 000000000..82a3b9a1a --- /dev/null +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -0,0 +1,40 @@ +#ifndef MBGL_STORAGE_DEFAULT_DEFAULT_FILE_SOURCE_IMPL +#define MBGL_STORAGE_DEFAULT_DEFAULT_FILE_SOURCE_IMPL + +#include +#include + +#include +#include + +namespace mbgl { + +class SharedRequestBase; + +class DefaultFileSource::Impl : public util::RunLoop { + friend class util::Thread; + +public: + Impl(FileCache *cache, const std::string &root = ""); + +public: + void notify(SharedRequestBase *sharedRequest, const std::set &observers, + std::shared_ptr response, FileCache::Hint hint); + SharedRequestBase *find(const Resource &resource); + + void processAdd(Request* request); + void processCancel(Request* request); + void processResult(const Resource& resource, std::shared_ptr response); + void processAbort(const Environment& env); + +public: + const std::string assetRoot; + +private: + std::unordered_map pending; + FileCache *cache = nullptr; +}; + +} + +#endif diff --git a/include/mbgl/storage/default/http_context.hpp b/src/mbgl/storage/http_context.hpp similarity index 100% rename from include/mbgl/storage/default/http_context.hpp rename to src/mbgl/storage/http_context.hpp diff --git a/include/mbgl/storage/default/http_request.hpp b/src/mbgl/storage/http_request.hpp similarity index 100% rename from include/mbgl/storage/default/http_request.hpp rename to src/mbgl/storage/http_request.hpp diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index ed7f625e8..ea80e5950 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -1,4 +1,4 @@ -#include +#include #include diff --git a/include/mbgl/storage/default/request.hpp b/src/mbgl/storage/request.hpp similarity index 100% rename from include/mbgl/storage/default/request.hpp rename to src/mbgl/storage/request.hpp diff --git a/include/mbgl/storage/default/shared_request_base.hpp b/src/mbgl/storage/shared_request_base.hpp similarity index 98% rename from include/mbgl/storage/default/shared_request_base.hpp rename to src/mbgl/storage/shared_request_base.hpp index efc78f603..d7ed00264 100644 --- a/include/mbgl/storage/default/shared_request_base.hpp +++ b/src/mbgl/storage/shared_request_base.hpp @@ -4,7 +4,7 @@ #include #include #include -#include +#include #include #include diff --git a/include/mbgl/storage/default/thread_context.hpp b/src/mbgl/storage/thread_context.hpp similarity index 100% rename from include/mbgl/storage/default/thread_context.hpp rename to src/mbgl/storage/thread_context.hpp diff --git a/include/mbgl/util/run_loop.hpp b/src/mbgl/util/run_loop.hpp similarity index 100% rename from include/mbgl/util/run_loop.hpp rename to src/mbgl/util/run_loop.hpp diff --git a/include/mbgl/util/thread.hpp b/src/mbgl/util/thread.hpp similarity index 100% rename from include/mbgl/util/thread.hpp rename to src/mbgl/util/thread.hpp diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index e37899a86..d5dd8b36e 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -3,7 +3,7 @@ #include #include -#include +#include TEST_F(Storage, CacheResponse) { SCOPED_TEST(CacheResponse); diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp index bd32042b9..ceb6b2969 100644 --- a/test/storage/cache_revalidate.cpp +++ b/test/storage/cache_revalidate.cpp @@ -3,7 +3,7 @@ #include #include -#include +#include TEST_F(Storage, CacheRevalidate) { SCOPED_TEST(CacheRevalidateSame) diff --git a/test/storage/database.cpp b/test/storage/database.cpp index 02b25d696..5498dd389 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -3,7 +3,7 @@ #include -#include +#include #include #include #include From 352ff64e06982bf215f0b818fa57c2fc0a7a8cd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Mon, 13 Apr 2015 18:32:59 +0200 Subject: [PATCH 09/13] rename SQLiteCache::Thread to SQLiteCache::Impl --- include/mbgl/storage/sqlite_cache.hpp | 4 ++-- platform/default/sqlite_cache.cpp | 24 ++++++++++++------------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/mbgl/storage/sqlite_cache.hpp b/include/mbgl/storage/sqlite_cache.hpp index db672b222..b216f74d7 100644 --- a/include/mbgl/storage/sqlite_cache.hpp +++ b/include/mbgl/storage/sqlite_cache.hpp @@ -21,8 +21,8 @@ public: void put(const Resource &resource, std::shared_ptr response, Hint hint) override; private: - class Thread; - const std::unique_ptr> thread; + class Impl; + const std::unique_ptr> thread; }; } diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index fcd6c09be..8060c73bd 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -13,12 +13,12 @@ namespace mbgl { -class SQLiteCache::Thread : public util::RunLoop { - friend class util::Thread; +class SQLiteCache::Impl : public util::RunLoop { + friend class util::Thread; public: - Thread(const std::string &path = ":memory:"); - ~Thread(); + Impl(const std::string &path = ":memory:"); + ~Impl(); public: void processGet(const Resource& resource, Callback callback); @@ -89,19 +89,19 @@ std::string unifyMapboxURLs(const std::string &url) { using namespace mapbox::sqlite; -SQLiteCache::SQLiteCache(const std::string& path_) : thread(util::make_unique>(path_)) { +SQLiteCache::SQLiteCache(const std::string& path_) : thread(util::make_unique>(path_)) { } SQLiteCache::~SQLiteCache() = default; -SQLiteCache::Thread::Thread(const std::string& path_) +SQLiteCache::Impl::Impl(const std::string& path_) : path(path_) { #ifdef __APPLE__ pthread_setname_np("SQLite Cache"); #endif } -SQLiteCache::Thread::~Thread() { +SQLiteCache::Impl::~Impl() { // Deleting these SQLite objects may result in exceptions, but we're in a destructor, so we // can't throw anything. try { @@ -114,11 +114,11 @@ SQLiteCache::Thread::~Thread() { } } -void SQLiteCache::Thread::createDatabase() { +void SQLiteCache::Impl::createDatabase() { db = util::make_unique(path.c_str(), ReadWrite | Create); } -void SQLiteCache::Thread::createSchema() { +void SQLiteCache::Impl::createSchema() { constexpr const char *const sql = "" "CREATE TABLE IF NOT EXISTS `http_cache` (" " `url` TEXT PRIMARY KEY NOT NULL," @@ -164,7 +164,7 @@ void SQLiteCache::get(const Resource &resource, Callback callback) { (*thread)->invoke([=] { (*thread)->processGet(resource, callback); }); } -void SQLiteCache::Thread::processGet(const Resource &resource, Callback callback) { +void SQLiteCache::Impl::processGet(const Resource &resource, Callback callback) { try { // This is called in the SQLite event loop. if (!db) { @@ -219,7 +219,7 @@ void SQLiteCache::put(const Resource &resource, std::shared_ptr } } -void SQLiteCache::Thread::processPut(const Resource& resource, std::shared_ptr response) { +void SQLiteCache::Impl::processPut(const Resource& resource, std::shared_ptr response) { try { if (!db) { createDatabase(); @@ -268,7 +268,7 @@ void SQLiteCache::Thread::processPut(const Resource& resource, std::shared_ptr Date: Mon, 13 Apr 2015 18:38:39 +0200 Subject: [PATCH 10/13] separate SQLiteCache::Impl header from implementation file --- platform/default/sqlite_cache.cpp | 29 +---------------- platform/default/sqlite_cache_impl.hpp | 44 ++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 28 deletions(-) create mode 100644 platform/default/sqlite_cache_impl.hpp diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index 8060c73bd..958cdc1ac 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -1,4 +1,4 @@ -#include +#include "sqlite_cache_impl.hpp" #include #include @@ -13,33 +13,6 @@ namespace mbgl { -class SQLiteCache::Impl : public util::RunLoop { - friend class util::Thread; - -public: - Impl(const std::string &path = ":memory:"); - ~Impl(); - -public: - void processGet(const Resource& resource, Callback callback); - void processPut(const Resource& resource, std::shared_ptr response); - void processRefresh(const Resource& resource, int64_t expires); - -private: - void createDatabase(); - void createSchema(); - -private: - const std::string path; - std::unique_ptr<::mapbox::sqlite::Database> db; - std::unique_ptr<::mapbox::sqlite::Statement> getStmt; - std::unique_ptr<::mapbox::sqlite::Statement> putStmt; - std::unique_ptr<::mapbox::sqlite::Statement> refreshStmt; - bool schema = false; -}; - - - std::string removeAccessTokenFromURL(const std::string &url) { const size_t token_start = url.find("access_token="); // Ensure that token exists, isn't at the front and is preceded by either & or ?. diff --git a/platform/default/sqlite_cache_impl.hpp b/platform/default/sqlite_cache_impl.hpp new file mode 100644 index 000000000..a194f9e78 --- /dev/null +++ b/platform/default/sqlite_cache_impl.hpp @@ -0,0 +1,44 @@ +#ifndef MBGL_STORAGE_DEFAULT_SQLITE_CACHE_IMPL +#define MBGL_STORAGE_DEFAULT_SQLITE_CACHE_IMPL + +#include +#include + +namespace mapbox { +namespace sqlite { +class Database; +class Statement; +} +} + +namespace mbgl { + +class SQLiteCache::Impl : public util::RunLoop { + friend class util::Thread; + +public: + Impl(const std::string &path = ":memory:"); + ~Impl(); + +public: + void processGet(const Resource& resource, Callback callback); + void processPut(const Resource& resource, std::shared_ptr response); + void processRefresh(const Resource& resource, int64_t expires); + +private: + void createDatabase(); + void createSchema(); + +private: + const std::string path; + std::unique_ptr<::mapbox::sqlite::Database> db; + std::unique_ptr<::mapbox::sqlite::Statement> getStmt; + std::unique_ptr<::mapbox::sqlite::Statement> putStmt; + std::unique_ptr<::mapbox::sqlite::Statement> refreshStmt; + bool schema = false; +}; + + +} + +#endif From 1eaf6b8c18d52fb060bb70a76a516c8a7c56a3fa Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Mon, 13 Apr 2015 16:41:37 -0700 Subject: [PATCH 11/13] Stricter Thread<> interface Implements a Thread<>::invoke interface that only permits calling member functions with bound arguments. Also introduce invokeWithResult, which accepts a callback and invokes it, passing the result, on the invoking thread's RunLoop. This closes the loop of two evented threads doing message passing. --- platform/default/sqlite_cache.cpp | 21 ++-- platform/default/sqlite_cache_impl.hpp | 9 +- src/mbgl/storage/default_file_source.cpp | 27 ++--- src/mbgl/storage/default_file_source_impl.hpp | 12 +- src/mbgl/util/run_loop.cpp | 58 ++++------ src/mbgl/util/run_loop.hpp | 106 +++++++++++++----- src/mbgl/util/thread.hpp | 88 ++++++++++----- src/mbgl/util/uv.cpp | 22 ++++ src/mbgl/util/uv_detail.hpp | 31 +++++ 9 files changed, 237 insertions(+), 137 deletions(-) diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index 958cdc1ac..e73618f2b 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #include #include "sqlite3.hpp" @@ -62,16 +61,14 @@ std::string unifyMapboxURLs(const std::string &url) { using namespace mapbox::sqlite; -SQLiteCache::SQLiteCache(const std::string& path_) : thread(util::make_unique>(path_)) { +SQLiteCache::SQLiteCache(const std::string& path_) + : thread(util::make_unique>("SQLite Cache", path_)) { } SQLiteCache::~SQLiteCache() = default; SQLiteCache::Impl::Impl(const std::string& path_) : path(path_) { -#ifdef __APPLE__ - pthread_setname_np("SQLite Cache"); -#endif } SQLiteCache::Impl::~Impl() { @@ -134,10 +131,10 @@ void SQLiteCache::get(const Resource &resource, Callback callback) { // Will try to load the URL from the SQLite database and call the callback when done. // Note that the callback is probably going to invoked from another thread, so the caller // must make sure that it can run in that thread. - (*thread)->invoke([=] { (*thread)->processGet(resource, callback); }); + thread->invokeWithResult(&Impl::processGet, callback, resource); } -void SQLiteCache::Impl::processGet(const Resource &resource, Callback callback) { +std::unique_ptr SQLiteCache::Impl::processGet(const Resource &resource) { try { // This is called in the SQLite event loop. if (!db) { @@ -170,14 +167,14 @@ void SQLiteCache::Impl::processGet(const Resource &resource, Callback callback) if (getStmt->get(5)) { // == compressed response->data = util::decompress(response->data); } - callback(std::move(response)); + return std::move(response); } else { // There is no data. - callback(nullptr); + return nullptr; } } catch (mapbox::sqlite::Exception& ex) { Log::Error(Event::Database, ex.code, ex.what()); - callback(nullptr); + return nullptr; } } @@ -186,9 +183,9 @@ void SQLiteCache::put(const Resource &resource, std::shared_ptr // storing a new response or updating the currently stored response, potentially setting a new // expiry date. if (hint == Hint::Full) { - (*thread)->invoke([=] { (*thread)->processPut(resource, response); }); + thread->invoke(&Impl::processPut, resource, std::move(response)); } else if (hint == Hint::Refresh) { - (*thread)->invoke([=] { (*thread)->processRefresh(resource, response->expires); }); + thread->invoke(&Impl::processRefresh, resource, int64_t(response->expires)); } } diff --git a/platform/default/sqlite_cache_impl.hpp b/platform/default/sqlite_cache_impl.hpp index a194f9e78..b13c7c2cd 100644 --- a/platform/default/sqlite_cache_impl.hpp +++ b/platform/default/sqlite_cache_impl.hpp @@ -2,7 +2,6 @@ #define MBGL_STORAGE_DEFAULT_SQLITE_CACHE_IMPL #include -#include namespace mapbox { namespace sqlite { @@ -13,15 +12,12 @@ class Statement; namespace mbgl { -class SQLiteCache::Impl : public util::RunLoop { - friend class util::Thread; - +class SQLiteCache::Impl { public: Impl(const std::string &path = ":memory:"); ~Impl(); -public: - void processGet(const Resource& resource, Callback callback); + std::unique_ptr processGet(const Resource&); void processPut(const Resource& resource, std::shared_ptr response); void processRefresh(const Resource& resource, int64_t expires); @@ -29,7 +25,6 @@ private: void createDatabase(); void createSchema(); -private: const std::string path; std::unique_ptr<::mapbox::sqlite::Database> db; std::unique_ptr<::mapbox::sqlite::Statement> getStmt; diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index b28e09641..d866e2353 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #pragma GCC diagnostic push #ifndef __clang__ @@ -29,13 +30,10 @@ namespace mbgl { DefaultFileSource::Impl::Impl(FileCache* cache_, const std::string& root) : assetRoot(root.empty() ? platform::assetRoot() : root), cache(cache_) { -#ifdef __APPLE__ - pthread_setname_np("FileSource"); -#endif } DefaultFileSource::DefaultFileSource(FileCache* cache, const std::string& root) - : thread(util::make_unique>(cache, root)) { + : thread(util::make_unique>("FileSource", cache, root)) { } DefaultFileSource::~DefaultFileSource() { @@ -62,7 +60,7 @@ Request* DefaultFileSource::request(const Resource& resource, // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. - (*thread)->invoke([=] { (*thread)->processAdd(req); }); + thread->invoke(&Impl::processAdd, std::move(req), thread->get()); return req; } @@ -76,14 +74,14 @@ void DefaultFileSource::cancel(Request *req) { // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. - (*thread)->invoke([=] { (*thread)->processCancel(req); }); + thread->invoke(&Impl::processCancel, std::move(req)); } void DefaultFileSource::abort(const Environment &env) { - (*thread)->invoke([=, &env] { (*thread)->processAbort(env); }); + thread->invoke(&Impl::processAbort, std::ref(env)); } -void DefaultFileSource::Impl::processAdd(Request* req) { +void DefaultFileSource::Impl::processAdd(Request* req, uv_loop_t* loop) { const Resource &resource = req->resource; // We're adding a new Request. @@ -102,13 +100,12 @@ void DefaultFileSource::Impl::processAdd(Request* req) { // But first, we're going to start querying the database if it exists. if (!cache) { - sharedRequest->start(loop().get()); + sharedRequest->start(loop); } else { // Otherwise, first check the cache for existing data so that we can potentially // revalidate the information without having to redownload everything. - cache->get(resource, [this, resource](std::unique_ptr response) { - std::shared_ptr sharedResponse = std::move(response); - invoke([this, resource, sharedResponse] { processResult(resource, sharedResponse); }); + cache->get(resource, [this, resource, loop](std::unique_ptr response) { + processResult(resource, std::move(response), loop); }); } } @@ -132,7 +129,7 @@ void DefaultFileSource::Impl::processCancel(Request* req) { req->destruct(); } -void DefaultFileSource::Impl::processResult(const Resource& resource, std::shared_ptr response) { +void DefaultFileSource::Impl::processResult(const Resource& resource, std::shared_ptr response, uv_loop_t* loop) { SharedRequestBase *sharedRequest = find(resource); if (sharedRequest) { if (response) { @@ -146,11 +143,11 @@ void DefaultFileSource::Impl::processResult(const Resource& resource, std::share return; } else { // The cached response is stale. Now run the real request. - sharedRequest->start(loop().get(), response); + sharedRequest->start(loop, response); } } else { // There is no response. Now run the real request. - sharedRequest->start(loop().get()); + sharedRequest->start(loop); } } else { // There is no request for this URL anymore. Likely, the request was canceled diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp index 82a3b9a1a..6383d13dc 100644 --- a/src/mbgl/storage/default_file_source_impl.hpp +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -2,7 +2,6 @@ #define MBGL_STORAGE_DEFAULT_DEFAULT_FILE_SOURCE_IMPL #include -#include #include #include @@ -11,26 +10,23 @@ namespace mbgl { class SharedRequestBase; -class DefaultFileSource::Impl : public util::RunLoop { - friend class util::Thread; - +class DefaultFileSource::Impl { public: Impl(FileCache *cache, const std::string &root = ""); -public: void notify(SharedRequestBase *sharedRequest, const std::set &observers, std::shared_ptr response, FileCache::Hint hint); SharedRequestBase *find(const Resource &resource); - void processAdd(Request* request); + void processAdd(Request* request, uv_loop_t* loop); void processCancel(Request* request); - void processResult(const Resource& resource, std::shared_ptr response); void processAbort(const Environment& env); -public: const std::string assetRoot; private: + void processResult(const Resource& resource, std::shared_ptr response, uv_loop_t* loop); + std::unordered_map pending; FileCache *cache = nullptr; }; diff --git a/src/mbgl/util/run_loop.cpp b/src/mbgl/util/run_loop.cpp index dba75fcfc..e945a0232 100644 --- a/src/mbgl/util/run_loop.cpp +++ b/src/mbgl/util/run_loop.cpp @@ -1,54 +1,38 @@ #include -#include -#include - -#include - -namespace /* anonymous */ { -inline void critical_section(std::mutex& mutex, std::function fn) { - std::lock_guard lock(mutex); - fn(); -} -} namespace mbgl { namespace util { +uv::tls RunLoop::current; + RunLoop::RunLoop() - : runloop(util::make_unique()), - runloopAsync(util::make_unique(runloop->get(), std::bind(&RunLoop::process, this))) { + : async(*loop, std::bind(&RunLoop::process, this)) { } -// Define here since we can't destroy the uv::* objects from just the header file. -RunLoop::~RunLoop() = default; - -void RunLoop::start() { - runloop->run(); -} - -void RunLoop::stop() { - critical_section(runloopMutex, [this] { runloopQueue.push(nullptr); }); - runloopAsync->send(); +void RunLoop::withMutex(std::function&& fn) { + std::lock_guard lock(mutex); + fn(); } void RunLoop::process() { - std::queue> queue; - critical_section(runloopMutex, [this, &queue] { queue.swap(runloopQueue); }); - while (!queue.empty()) { - if (queue.front()) { - queue.front()(); - } else { - runloopAsync->unref(); - } - queue.pop(); + Queue queue_; + withMutex([&] { queue_.swap(queue); }); + + while (!queue_.empty()) { + (queue_.front())(); + queue_.pop(); } } -void RunLoop::invoke(std::function&& fn) { - if (fn) { - critical_section(runloopMutex, [this, &fn] { runloopQueue.emplace(std::move(fn)); }); - runloopAsync->send(); - } +void RunLoop::run() { + assert(!current.get()); + current.set(this); + loop.run(); + current.set(nullptr); +} + +void RunLoop::stop() { + invoke([&] { async.unref(); }); } } diff --git a/src/mbgl/util/run_loop.hpp b/src/mbgl/util/run_loop.hpp index 9bed417c3..c39fb6018 100644 --- a/src/mbgl/util/run_loop.hpp +++ b/src/mbgl/util/run_loop.hpp @@ -1,53 +1,97 @@ #ifndef MBGL_UTIL_RUN_LOOP #define MBGL_UTIL_RUN_LOOP -#include -#include +#include + #include #include - -namespace uv { -class async; -class loop; -} +#include namespace mbgl { namespace util { -template class Thread; - class RunLoop { - friend Thread; - -protected: - // These are called by the Thread<> wrapper. +public: RunLoop(); - ~RunLoop(); - // Called by the Thread<> wrapper to start the loop. When you implement this - // method in a child class, you *must* call this function as the last action. - void start(); - -protected: - // Called by the Thread<> wrapper to terminate this loop. + void run(); void stop(); - // Obtain the underlying loop object in case you want to attach additional listeners. - uv::loop& loop() { return *runloop; }; + // Invoke fn() in the runloop thread. + template + void invoke(Fn&& fn) { + withMutex([&] { queue.push(Message(std::move(fn))); }); + async.send(); + } + + // Invoke fn() in the runloop thread, then invoke callback(result) in the current thread. + template + void invokeWithResult(Fn&& fn, std::function callback) { + RunLoop* outer = current.get(); + assert(outer); + + invoke([fn, callback, outer] { + /* + With C++14, we could write: + + outer->invoke([callback, result = std::move(fn())] () mutable { + callback(std::move(result)); + }); + + Instead we're using a workaround with std::bind + to obtain move-capturing semantics with C++11: + http://stackoverflow.com/a/12744730/52207 + */ + outer->invoke(std::bind([callback] (R& result) { + callback(std::move(result)); + }, std::move(fn()))); + }); + } + + uv_loop_t* get() { return *loop; } private: - // Invokes function in the run loop. + struct Message { + struct Base { + virtual void operator()() = 0; + virtual ~Base() = default; + }; + + template + struct Invoker : Base { + Invoker(F&& f) : func(std::move(f)) {} + void operator()() override { func(); } + F func; + }; + + Message() = default; + Message(Message&&) = default; + ~Message() = default; + Message& operator=(Message&&) = default; + + // copy members implicitly deleted + + template + Message(Fn fn) + : p_fn(new Invoker(std::move(fn))) { + } + + void operator()() const { (*p_fn)(); } + std::unique_ptr p_fn; + }; + + using Queue = std::queue; + + static uv::tls current; + + void withMutex(std::function&&); void process(); -public: - // Schedules a function to be executed as part of this run loop. - void invoke(std::function&& fn); + Queue queue; + std::mutex mutex; -private: - const std::unique_ptr runloop; - const std::unique_ptr runloopAsync; - std::mutex runloopMutex; - std::queue> runloopQueue; + uv::loop loop; + uv::async async; }; } diff --git a/src/mbgl/util/thread.hpp b/src/mbgl/util/thread.hpp index 7c9ba70e1..4831b9efc 100644 --- a/src/mbgl/util/thread.hpp +++ b/src/mbgl/util/thread.hpp @@ -3,6 +3,9 @@ #include #include +#include + +#include namespace { @@ -34,49 +37,80 @@ template class Thread { public: template - Thread(Args&&... args); + Thread(const std::string& name, Args&&... args); + ~Thread(); + + // Invoke object->fn(args...) in the runloop thread. + template + void invoke(Fn fn, Args&&... args) { + loop->invoke(std::bind(fn, object, args...)); + } + + // Invoke object->fn(args...) in the runloop thread, then invoke callback(result) in the current thread. + template + void invokeWithResult(Fn fn, std::function callback, Args&&... args) { + loop->invokeWithResult(std::bind(fn, object, args...), callback); + } + + uv_loop_t* get() { return loop->get(); } + +private: Thread(const Thread&) = delete; Thread(Thread&&) = delete; Thread& operator=(const Thread&) = delete; Thread& operator=(Thread&&) = delete; - ~Thread(); - inline Object* operator->() const { return &object; } - inline operator Object*() const { return &object; } - -private: template - void run(std::promise& promise, P&& params, index_sequence) { - Object context(std::get(std::forward

(params))...); - promise.set_value(context); - context.start(); - joinable.get_future().get(); - } + void run(P&& params, index_sequence); -private: - std::thread thread; + std::promise running; std::promise joinable; - Object& object; + + std::thread thread; + + Object* object; + RunLoop* loop; }; template template -Thread::Thread(Args&&... args) - : object([&](std::tuple&& params) -> Object& { - // Note: We're using std::tuple<> to store the arguments because GCC 4.9 has a bug - // when expanding parameters packs captured in lambdas. - std::promise promise; - constexpr auto seq = typename integer_sequence::type(); - thread = std::thread([&] { - run(promise, std::move(params), seq); - }); - return promise.get_future().get(); - }(std::move(std::forward_as_tuple(::std::forward(args)...)))) { +Thread::Thread(const std::string& name, Args&&... args) { + // Note: We're using std::tuple<> to store the arguments because GCC 4.9 has a bug + // when expanding parameters packs captured in lambdas. + std::tuple params = std::forward_as_tuple(::std::forward(args)...); + + thread = std::thread([&] { + #ifdef __APPLE__ + pthread_setname_np(name.c_str()); + #else + (void(name)); + #endif + + constexpr auto seq = typename integer_sequence::type(); + run(std::move(params), seq); + }); + + running.get_future().get(); +} + +template +template +void Thread::run(P&& params, index_sequence) { + Object object_(std::get(std::forward

(params))...); + object = &object_; + + RunLoop loop_; + loop = &loop_; + + running.set_value(); + loop_.run(); + + joinable.get_future().get(); } template Thread::~Thread() { - object.stop(); + loop->stop(); joinable.set_value(); thread.join(); } diff --git a/src/mbgl/util/uv.cpp b/src/mbgl/util/uv.cpp index d465dfd96..5dae34ebd 100644 --- a/src/mbgl/util/uv.cpp +++ b/src/mbgl/util/uv.cpp @@ -3,6 +3,28 @@ #include +#if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 + +int uv_key_create(uv_key_t* key) { + return -pthread_key_create(key, NULL); +} + +void uv_key_delete(uv_key_t* key) { + if (pthread_key_delete(*key)) + abort(); +} + +void* uv_key_get(uv_key_t* key) { + return pthread_getspecific(*key); +} + +void uv_key_set(uv_key_t* key, void* value) { + if (pthread_setspecific(*key, value)) + abort(); +} + +#endif + namespace uv { std::string cwd() { diff --git a/src/mbgl/util/uv_detail.hpp b/src/mbgl/util/uv_detail.hpp index 6acef1b38..96d544246 100644 --- a/src/mbgl/util/uv_detail.hpp +++ b/src/mbgl/util/uv_detail.hpp @@ -11,6 +11,21 @@ #include #include +#if UV_VERSION_MAJOR == 0 && UV_VERSION_MINOR <= 10 + +// Add thread local storage to libuv API: +// https://github.com/joyent/libuv/commit/5d2434bf71e47802841bad218d521fa254d1ca2d + +typedef pthread_key_t uv_key_t; + +UV_EXTERN int uv_key_create(uv_key_t* key); +UV_EXTERN void uv_key_delete(uv_key_t* key); +UV_EXTERN void* uv_key_get(uv_key_t* key); +UV_EXTERN void uv_key_set(uv_key_t* key, void* value); + +#endif + + namespace uv { template @@ -133,6 +148,22 @@ private: uv_rwlock_t mtx; }; +template +class tls : public mbgl::util::noncopyable { +public: + inline tls() { + if (uv_key_create(&key) != 0) { + throw std::runtime_error("failed to initialize thread local storage key"); + } + } + inline ~tls() { uv_key_delete(&key); } + inline T* get() { return reinterpret_cast(uv_key_get(&key)); } + inline void set(T* val) { uv_key_set(&key, val); } + +private: + uv_key_t key; +}; + } #endif From 5fa0512be289dd5a6e55d515352f9f8fb93a1b5e Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 15 Apr 2015 18:09:34 -0700 Subject: [PATCH 12/13] Shorten method names --- platform/default/sqlite_cache.cpp | 12 ++++++------ platform/default/sqlite_cache_impl.hpp | 6 +++--- src/mbgl/storage/default_file_source.cpp | 12 ++++++------ src/mbgl/storage/default_file_source_impl.hpp | 6 +++--- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index e73618f2b..a57e666e9 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -131,10 +131,10 @@ void SQLiteCache::get(const Resource &resource, Callback callback) { // Will try to load the URL from the SQLite database and call the callback when done. // Note that the callback is probably going to invoked from another thread, so the caller // must make sure that it can run in that thread. - thread->invokeWithResult(&Impl::processGet, callback, resource); + thread->invokeWithResult(&Impl::get, callback, resource); } -std::unique_ptr SQLiteCache::Impl::processGet(const Resource &resource) { +std::unique_ptr SQLiteCache::Impl::get(const Resource &resource) { try { // This is called in the SQLite event loop. if (!db) { @@ -183,13 +183,13 @@ void SQLiteCache::put(const Resource &resource, std::shared_ptr // storing a new response or updating the currently stored response, potentially setting a new // expiry date. if (hint == Hint::Full) { - thread->invoke(&Impl::processPut, resource, std::move(response)); + thread->invoke(&Impl::put, resource, std::move(response)); } else if (hint == Hint::Refresh) { - thread->invoke(&Impl::processRefresh, resource, int64_t(response->expires)); + thread->invoke(&Impl::refresh, resource, int64_t(response->expires)); } } -void SQLiteCache::Impl::processPut(const Resource& resource, std::shared_ptr response) { +void SQLiteCache::Impl::put(const Resource& resource, std::shared_ptr response) { try { if (!db) { createDatabase(); @@ -238,7 +238,7 @@ void SQLiteCache::Impl::processPut(const Resource& resource, std::shared_ptr processGet(const Resource&); - void processPut(const Resource& resource, std::shared_ptr response); - void processRefresh(const Resource& resource, int64_t expires); + std::unique_ptr get(const Resource&); + void put(const Resource& resource, std::shared_ptr response); + void refresh(const Resource& resource, int64_t expires); private: void createDatabase(); diff --git a/src/mbgl/storage/default_file_source.cpp b/src/mbgl/storage/default_file_source.cpp index d866e2353..4055001fc 100644 --- a/src/mbgl/storage/default_file_source.cpp +++ b/src/mbgl/storage/default_file_source.cpp @@ -60,7 +60,7 @@ Request* DefaultFileSource::request(const Resource& resource, // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. - thread->invoke(&Impl::processAdd, std::move(req), thread->get()); + thread->invoke(&Impl::add, std::move(req), thread->get()); return req; } @@ -74,14 +74,14 @@ void DefaultFileSource::cancel(Request *req) { // This function can be called from any thread. Make sure we're executing the actual call in the // file source loop by sending it over the queue. - thread->invoke(&Impl::processCancel, std::move(req)); + thread->invoke(&Impl::cancel, std::move(req)); } void DefaultFileSource::abort(const Environment &env) { - thread->invoke(&Impl::processAbort, std::ref(env)); + thread->invoke(&Impl::abort, std::ref(env)); } -void DefaultFileSource::Impl::processAdd(Request* req, uv_loop_t* loop) { +void DefaultFileSource::Impl::add(Request* req, uv_loop_t* loop) { const Resource &resource = req->resource; // We're adding a new Request. @@ -112,7 +112,7 @@ void DefaultFileSource::Impl::processAdd(Request* req, uv_loop_t* loop) { sharedRequest->subscribe(req); } -void DefaultFileSource::Impl::processCancel(Request* req) { +void DefaultFileSource::Impl::cancel(Request* req) { SharedRequestBase *sharedRequest = find(req->resource); if (sharedRequest) { // If the number of dependent requests of the SharedRequestBase drops to zero, the @@ -156,7 +156,7 @@ void DefaultFileSource::Impl::processResult(const Resource& resource, std::share } // Aborts all requests that are part of the current environment. -void DefaultFileSource::Impl::processAbort(const Environment& env) { +void DefaultFileSource::Impl::abort(const Environment& env) { // Construct a cancellation response. auto res = util::make_unique(); res->status = Response::Error; diff --git a/src/mbgl/storage/default_file_source_impl.hpp b/src/mbgl/storage/default_file_source_impl.hpp index 6383d13dc..97210dc44 100644 --- a/src/mbgl/storage/default_file_source_impl.hpp +++ b/src/mbgl/storage/default_file_source_impl.hpp @@ -18,9 +18,9 @@ public: std::shared_ptr response, FileCache::Hint hint); SharedRequestBase *find(const Resource &resource); - void processAdd(Request* request, uv_loop_t* loop); - void processCancel(Request* request); - void processAbort(const Environment& env); + void add(Request* request, uv_loop_t* loop); + void cancel(Request* request); + void abort(const Environment& env); const std::string assetRoot; From ac25d32a094f83ed51d5854c9f234b10eedbeac0 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 15 Apr 2015 19:11:59 -0700 Subject: [PATCH 13/13] Convert storage tests to RunLoop --- test/storage/database.cpp | 248 ++++++++++++++++++++------------------ 1 file changed, 133 insertions(+), 115 deletions(-) diff --git a/test/storage/database.cpp b/test/storage/database.cpp index 5498dd389..4a9cc3e16 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -1,36 +1,14 @@ #include "storage.hpp" #include "../fixtures/fixture_log_observer.hpp" -#include - #include #include #include #include +#include #include -class ScopedTest { -public: - ScopedTest(std::function destructor_) : destructor(destructor_) {} - ScopedTest() {} - - void finish() { - promise.set_value(); - } - - ~ScopedTest() { - promise.get_future().get(); - if (destructor) { - destructor(); - } - } - -private: - const std::function destructor; - std::promise promise; -}; - TEST_F(Storage, DatabaseDoesNotExist) { using namespace mbgl; @@ -38,15 +16,19 @@ TEST_F(Storage, DatabaseDoesNotExist) { SQLiteCache cache("test/fixtures/404/cache.db"); - ScopedTest test([&] { - auto observer = Log::removeObserver(); - EXPECT_EQ(1ul, dynamic_cast(observer.get())->count({ EventSeverity::Error, Event::Database, 14, "unable to open database file" })); + util::RunLoop loop; + + loop.invoke([&] { + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + loop.stop(); + }); }); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - test.finish(); - }); + loop.run(); + + auto observer = Log::removeObserver(); + EXPECT_EQ(1ul, dynamic_cast(observer.get())->count({ EventSeverity::Error, Event::Database, 14, "unable to open database file" })); } void createDir(const char* name) { @@ -83,14 +65,18 @@ TEST_F(Storage, DatabaseCreate) { SQLiteCache cache("test/fixtures/database/cache.db"); - ScopedTest test([&] { - Log::removeObserver(); + util::RunLoop loop; + + loop.invoke([&] { + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + loop.stop(); + }); }); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - test.finish(); - }); + loop.run(); + + Log::removeObserver(); } class FileLock { @@ -143,17 +129,20 @@ TEST_F(Storage, DatabaseLockedRead) { SQLiteCache cache("test/fixtures/database/locked.db"); - std::promise promise; - { // First request should fail. Log::setObserver(util::make_unique()); - promise = {}; - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - promise.set_value(); + + util::RunLoop loop; + + loop.invoke([&] { + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); // Make sure that we got a few "database locked" errors auto observer = Log::removeObserver(); @@ -167,13 +156,17 @@ TEST_F(Storage, DatabaseLockedRead) { { // First, try getting a file (the cache value should not exist). Log::setObserver(util::make_unique()); - promise = {}; - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - promise.set_value(); + util::RunLoop loop; + + loop.invoke([&] { + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); // Make sure that we got a no errors Log::removeObserver(); @@ -192,19 +185,22 @@ TEST_F(Storage, DatabaseLockedWrite) { SQLiteCache cache("test/fixtures/database/locked.db"); - std::promise promise; - { // Adds a file (which should fail). Log::setObserver(util::make_unique()); - promise = {}; - auto response = std::make_shared(); - cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - promise.set_value(); + + util::RunLoop loop; + + loop.invoke([&] { + auto response = std::make_shared(); + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get()); @@ -217,17 +213,21 @@ TEST_F(Storage, DatabaseLockedWrite) { { // Then, set a file and obtain it again. Log::setObserver(util::make_unique()); - promise = {}; - auto response = std::make_shared(); - response->data = "Demo"; - cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_NE(nullptr, res.get()); - EXPECT_EQ("Demo", res->data); - promise.set_value(); + util::RunLoop loop; + + loop.invoke([&] { + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_NE(nullptr, res.get()); + EXPECT_EQ("Demo", res->data); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); // Make sure that we got a no errors Log::removeObserver(); @@ -249,20 +249,23 @@ TEST_F(Storage, DatabaseLockedRefresh) { // Then, lock the file and try again. FileLock guard("test/fixtures/database/locked.db"); - std::promise promise; - { // Adds a file. Log::setObserver(util::make_unique()); - promise = {}; - auto response = std::make_shared(); - response->data = "Demo"; - cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - promise.set_value(); + + util::RunLoop loop; + + loop.invoke([&] { + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get()); @@ -272,16 +275,20 @@ TEST_F(Storage, DatabaseLockedRefresh) { { // Then, try to refresh it. Log::setObserver(util::make_unique()); - promise = {}; - auto response = std::make_shared(); - response->data = "Demo"; - cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Refresh); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - promise.set_value(); + util::RunLoop loop; + + loop.invoke([&] { + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Refresh); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); // Make sure that we got the right errors. auto observer = Log::removeObserver(); @@ -301,21 +308,24 @@ TEST_F(Storage, DatabaseDeleted) { SQLiteCache cache("test/fixtures/database/locked.db"); - std::promise promise; - { // Adds a file. Log::setObserver(util::make_unique()); - promise = {}; - auto response = std::make_shared(); - response->data = "Demo"; - cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_NE(nullptr, res.get()); - EXPECT_EQ("Demo", res->data); - promise.set_value(); + + util::RunLoop loop; + + loop.invoke([&] { + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_NE(nullptr, res.get()); + EXPECT_EQ("Demo", res->data); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); Log::removeObserver(); } @@ -325,16 +335,21 @@ TEST_F(Storage, DatabaseDeleted) { { // Adds a file. Log::setObserver(util::make_unique()); - promise = {}; - auto response = std::make_shared(); - response->data = "Demo"; - cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_NE(nullptr, res.get()); - EXPECT_EQ("Demo", res->data); - promise.set_value(); + + util::RunLoop loop; + + loop.invoke([&] { + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_NE(nullptr, res.get()); + EXPECT_EQ("Demo", res->data); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get()); @@ -354,21 +369,24 @@ TEST_F(Storage, DatabaseInvalid) { SQLiteCache cache("test/fixtures/database/invalid.db"); - std::promise promise; - { // Adds a file. Log::setObserver(util::make_unique()); - promise = {}; - auto response = std::make_shared(); - response->data = "Demo"; - cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_NE(nullptr, res.get()); - EXPECT_EQ("Demo", res->data); - promise.set_value(); + + util::RunLoop loop; + + loop.invoke([&] { + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response, FileCache::Hint::Full); + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_NE(nullptr, res.get()); + EXPECT_EQ("Demo", res->data); + loop.stop(); + }); }); - promise.get_future().get(); + + loop.run(); auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get());