From 36f695b4112e83c1c71e05a94215bc5bd9447a90 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 1 May 2015 14:31:39 -0400 Subject: [PATCH 1/2] Write SQLite tests in terms of Impl --- include/mbgl/storage/sqlite_cache.hpp | 3 +- test/storage/database.cpp | 198 +++++++------------------- test/test.gypi | 2 +- 3 files changed, 57 insertions(+), 146 deletions(-) diff --git a/include/mbgl/storage/sqlite_cache.hpp b/include/mbgl/storage/sqlite_cache.hpp index b216f74d7..fd67d48bb 100644 --- a/include/mbgl/storage/sqlite_cache.hpp +++ b/include/mbgl/storage/sqlite_cache.hpp @@ -20,8 +20,9 @@ public: void get(const Resource &resource, Callback callback) override; void put(const Resource &resource, std::shared_ptr response, Hint hint) override; -private: class Impl; + +private: const std::unique_ptr> thread; }; diff --git a/test/storage/database.cpp b/test/storage/database.cpp index 4a9cc3e16..a44c7ff4b 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -1,11 +1,10 @@ #include "storage.hpp" #include "../fixtures/fixture_log_observer.hpp" -#include +#include "sqlite_cache_impl.hpp" #include #include #include -#include #include @@ -14,18 +13,10 @@ TEST_F(Storage, DatabaseDoesNotExist) { Log::setObserver(util::make_unique()); - SQLiteCache cache("test/fixtures/404/cache.db"); + SQLiteCache::Impl cache(nullptr, "test/fixtures/404/cache.db"); - util::RunLoop loop; - - loop.invoke([&] { - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - loop.stop(); - }); - }); - - loop.run(); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_EQ(nullptr, res.get()); auto observer = Log::removeObserver(); EXPECT_EQ(1ul, dynamic_cast(observer.get())->count({ EventSeverity::Error, Event::Database, 14, "unable to open database file" })); @@ -63,18 +54,10 @@ TEST_F(Storage, DatabaseCreate) { Log::setObserver(util::make_unique()); - SQLiteCache cache("test/fixtures/database/cache.db"); + SQLiteCache::Impl cache(nullptr, "test/fixtures/database/cache.db"); - util::RunLoop loop; - - loop.invoke([&] { - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - loop.stop(); - }); - }); - - loop.run(); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_EQ(nullptr, res.get()); Log::removeObserver(); } @@ -126,23 +109,14 @@ TEST_F(Storage, DatabaseLockedRead) { deleteFile("test/fixtures/database/locked.db"); FileLock guard("test/fixtures/database/locked.db"); - - SQLiteCache cache("test/fixtures/database/locked.db"); + SQLiteCache::Impl cache(nullptr, "test/fixtures/database/locked.db"); { // First request should fail. Log::setObserver(util::make_unique()); - util::RunLoop loop; - - loop.invoke([&] { - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - loop.stop(); - }); - }); - - loop.run(); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_EQ(nullptr, res.get()); // Make sure that we got a few "database locked" errors auto observer = Log::removeObserver(); @@ -157,16 +131,8 @@ TEST_F(Storage, DatabaseLockedRead) { // First, try getting a file (the cache value should not exist). Log::setObserver(util::make_unique()); - util::RunLoop loop; - - loop.invoke([&] { - cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { - EXPECT_EQ(nullptr, res.get()); - loop.stop(); - }); - }); - - loop.run(); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_EQ(nullptr, res.get()); // Make sure that we got a no errors Log::removeObserver(); @@ -183,24 +149,16 @@ TEST_F(Storage, DatabaseLockedWrite) { deleteFile("test/fixtures/database/locked.db"); FileLock guard("test/fixtures/database/locked.db"); - SQLiteCache cache("test/fixtures/database/locked.db"); + SQLiteCache::Impl cache(nullptr, "test/fixtures/database/locked.db"); { // Adds a file (which should fail). Log::setObserver(util::make_unique()); - 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(); - }); - }); - - loop.run(); + auto response = std::make_shared(); + cache.put({ Resource::Unknown, "mapbox://test" }, response); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_EQ(nullptr, res.get()); auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get()); @@ -214,20 +172,12 @@ TEST_F(Storage, DatabaseLockedWrite) { // Then, set a file and obtain it again. Log::setObserver(util::make_unique()); - 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(); - }); - }); - - loop.run(); + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_NE(nullptr, res.get()); + EXPECT_EQ("Demo", res->data); // Make sure that we got a no errors Log::removeObserver(); @@ -244,7 +194,7 @@ TEST_F(Storage, DatabaseLockedRefresh) { createDir("test/fixtures/database"); deleteFile("test/fixtures/database/locked.db"); - SQLiteCache cache("test/fixtures/database/locked.db"); + SQLiteCache::Impl cache(nullptr, "test/fixtures/database/locked.db"); // Then, lock the file and try again. FileLock guard("test/fixtures/database/locked.db"); @@ -253,19 +203,11 @@ TEST_F(Storage, DatabaseLockedRefresh) { // Adds a file. Log::setObserver(util::make_unique()); - 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(); - }); - }); - - loop.run(); + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_EQ(nullptr, res.get()); auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get()); @@ -276,19 +218,11 @@ TEST_F(Storage, DatabaseLockedRefresh) { // Then, try to refresh it. Log::setObserver(util::make_unique()); - 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(); - }); - }); - - loop.run(); + auto response = std::make_shared(); + response->data = "Demo"; + cache.refresh({ Resource::Unknown, "mapbox://test" }, response->expires); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_EQ(nullptr, res.get()); // Make sure that we got the right errors. auto observer = Log::removeObserver(); @@ -306,26 +240,18 @@ TEST_F(Storage, DatabaseDeleted) { createDir("test/fixtures/database"); deleteFile("test/fixtures/database/locked.db"); - SQLiteCache cache("test/fixtures/database/locked.db"); + SQLiteCache::Impl cache(nullptr, "test/fixtures/database/locked.db"); { // Adds a file. Log::setObserver(util::make_unique()); - 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(); - }); - }); - - loop.run(); + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_NE(nullptr, res.get()); + EXPECT_EQ("Demo", res->data); Log::removeObserver(); } @@ -336,20 +262,12 @@ TEST_F(Storage, DatabaseDeleted) { // Adds a file. Log::setObserver(util::make_unique()); - 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(); - }); - }); - - loop.run(); + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_NE(nullptr, res.get()); + EXPECT_EQ("Demo", res->data); auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get()); @@ -367,26 +285,18 @@ TEST_F(Storage, DatabaseInvalid) { deleteFile("test/fixtures/database/invalid.db"); writeFile("test/fixtures/database/invalid.db", "this is an invalid file"); - SQLiteCache cache("test/fixtures/database/invalid.db"); + SQLiteCache::Impl cache(nullptr, "test/fixtures/database/invalid.db"); { // Adds a file. Log::setObserver(util::make_unique()); - 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(); - }); - }); - - loop.run(); + auto response = std::make_shared(); + response->data = "Demo"; + cache.put({ Resource::Unknown, "mapbox://test" }, response); + std::unique_ptr res = cache.get({ Resource::Unknown, "mapbox://test" }); + EXPECT_NE(nullptr, res.get()); + EXPECT_EQ("Demo", res->data); auto observer = Log::removeObserver(); auto flo = dynamic_cast(observer.get()); diff --git a/test/test.gypi b/test/test.gypi index f025b1b82..1c81b76c7 100644 --- a/test/test.gypi +++ b/test/test.gypi @@ -17,7 +17,7 @@ }, { 'target_name': 'test', 'type': 'executable', - 'include_dirs': [ '../include', '../src' ], + 'include_dirs': [ '../include', '../src', '../platform/default' ], 'dependencies': [ 'symlink_TEST_DATA', '../mbgl.gyp:core', From 4c8f716f20bd95c9821b20a6c30ab7af35bf1497 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Fri, 1 May 2015 18:13:35 -0400 Subject: [PATCH 2/2] Eliminate uv::async leak (fixes #1393) --- src/mbgl/util/run_loop.cpp | 16 +++++++--------- src/mbgl/util/run_loop.hpp | 8 +++----- src/mbgl/util/thread.hpp | 20 +++++++++----------- 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/mbgl/util/run_loop.cpp b/src/mbgl/util/run_loop.cpp index fd9ad4306..7f277c988 100644 --- a/src/mbgl/util/run_loop.cpp +++ b/src/mbgl/util/run_loop.cpp @@ -5,8 +5,13 @@ namespace util { uv::tls RunLoop::current; -RunLoop::RunLoop() - : async(*loop, std::bind(&RunLoop::process, this)) { +RunLoop::RunLoop(uv_loop_t* loop) + : async(loop, std::bind(&RunLoop::process, this)) { + current.set(this); +} + +RunLoop::~RunLoop() { + current.set(nullptr); } void RunLoop::withMutex(std::function&& fn) { @@ -24,13 +29,6 @@ void RunLoop::process() { } } -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 971ad2cac..1fc742372 100644 --- a/src/mbgl/util/run_loop.hpp +++ b/src/mbgl/util/run_loop.hpp @@ -14,9 +14,9 @@ namespace util { class RunLoop : private util::noncopyable { public: - RunLoop(); + RunLoop(uv_loop_t*); + ~RunLoop(); - void run(); void stop(); // Invoke fn() in the runloop thread. @@ -63,7 +63,7 @@ public: }); } - uv_loop_t* get() { return *loop; } + uv_loop_t* get() { return async.get()->loop; } static uv::tls current; @@ -90,8 +90,6 @@ private: Queue queue; std::mutex mutex; - - uv::loop loop; uv::async async; }; diff --git a/src/mbgl/util/thread.hpp b/src/mbgl/util/thread.hpp index 415bda38d..23b89c0ab 100644 --- a/src/mbgl/util/thread.hpp +++ b/src/mbgl/util/thread.hpp @@ -77,11 +77,6 @@ public: return future.get(); } - // Join the thread, but call the given function repeatedly in the current thread - // while waiting for the join to finish. This should be immediately followed by - // destroying the Thread. - void pumpingStop(std::function); - private: Thread(const Thread&) = delete; Thread(Thread&&) = delete; @@ -124,21 +119,24 @@ Thread::Thread(const std::string& name, Args&&... args) { template template void Thread::run(P&& params, index_sequence) { - RunLoop loop_; - loop = &loop_; + uv::loop l; { - Object object_(loop_.get(), std::get(std::forward

(params))...); + Object object_(l.get(), std::get(std::forward

(params))...); object = &object_; - running.set_value(); - loop_.run(); + RunLoop loop_(l.get()); + loop = &loop_; + running.set_value(); + l.run(); + + loop = nullptr; object = nullptr; } // Run the loop again to ensure that async close callbacks have been called. - loop_.run(); + l.run(); joinable.get_future().get(); }