From 7d58a41de5dbf1b24b8bad9a2a98c21a7bf75382 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Tue, 7 Apr 2015 18:04:23 +0200 Subject: [PATCH 1/3] make sqlite storage more resilient to sporadic errors - catch SQLite exceptions and report them - failed statements are ignored, we're really just caching here, so if it fails we're handling it gracefully elsewhere - handle cases where the database file goes away after we opened it - handle cases where the schema wasn't created after the database file was opened successfully - add tests --- include/mbgl/platform/log.hpp | 4 +- include/mbgl/storage/default/sqlite_cache.hpp | 2 + platform/default/sqlite3.cpp | 33 +- platform/default/sqlite_cache.cpp | 218 ++++++----- src/mbgl/platform/log.cpp | 6 + test/fixtures/fixture_log_observer.cpp | 3 + test/fixtures/fixture_log_observer.hpp | 4 +- test/storage/database.cpp | 338 ++++++++++++++++++ test/test.gyp | 1 + 9 files changed, 501 insertions(+), 108 deletions(-) create mode 100644 test/storage/database.cpp diff --git a/include/mbgl/platform/log.hpp b/include/mbgl/platform/log.hpp index 5d287eb57..d6f3cd1ab 100644 --- a/include/mbgl/platform/log.hpp +++ b/include/mbgl/platform/log.hpp @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -12,7 +13,7 @@ namespace mbgl { class Log { public: - class Observer { + class Observer : private util::noncopyable { public: virtual ~Observer() = default; @@ -22,6 +23,7 @@ public: }; static void setObserver(std::unique_ptr Observer); + static std::unique_ptr removeObserver(); private: template diff --git a/include/mbgl/storage/default/sqlite_cache.hpp b/include/mbgl/storage/default/sqlite_cache.hpp index 8f2746561..fe80a41b5 100644 --- a/include/mbgl/storage/default/sqlite_cache.hpp +++ b/include/mbgl/storage/default/sqlite_cache.hpp @@ -38,6 +38,7 @@ private: void process(StopAction &action); void createDatabase(); + void createSchema(); const std::string path; uv_loop_t *loop = nullptr; @@ -45,6 +46,7 @@ private: std::thread thread; std::unique_ptr<::mapbox::sqlite::Database> db; std::unique_ptr<::mapbox::sqlite::Statement> getStmt, putStmt, refreshStmt; + bool schema = false; }; } diff --git a/platform/default/sqlite3.cpp b/platform/default/sqlite3.cpp index 19ecaba0b..b2ae4e65b 100644 --- a/platform/default/sqlite3.cpp +++ b/platform/default/sqlite3.cpp @@ -76,11 +76,16 @@ Statement::Statement(sqlite3 *db, const char *sql) { } } -#define CHECK_SQLITE_OK(err) \ +#define CHECK_SQLITE_OK_STMT(err, stmt) \ if (err != SQLITE_OK) { \ throw Exception { err, sqlite3_errmsg(sqlite3_db_handle(stmt)) }; \ } +#define CHECK_SQLITE_OK(err) \ + if (err != SQLITE_OK) { \ + throw Exception { err, sqlite3_errstr(err) }; \ + } + Statement::Statement(Statement &&other) { *this = std::move(other); } @@ -92,8 +97,7 @@ Statement &Statement::operator=(Statement &&other) { Statement::~Statement() { if (stmt) { - const int err = sqlite3_finalize(stmt); - CHECK_SQLITE_OK(err) + sqlite3_finalize(stmt); } } @@ -101,38 +105,38 @@ Statement::operator bool() const { return stmt != nullptr; } -#define BIND_3(type, value) \ +#define BIND_3(type, value, stmt) \ assert(stmt); \ const int err = sqlite3_bind_##type(stmt, offset, value); \ - CHECK_SQLITE_OK(err) + CHECK_SQLITE_OK_STMT(err, stmt) -#define BIND_5(type, value, length, param) \ +#define BIND_5(type, value, length, param, stmt) \ assert(stmt); \ const int err = sqlite3_bind_##type(stmt, offset, value, length, param); \ - CHECK_SQLITE_OK(err) + CHECK_SQLITE_OK_STMT(err, stmt) template <> void Statement::bind(int offset, int value) { - BIND_3(int, value) + BIND_3(int, value, stmt) } template <> void Statement::bind(int offset, int64_t value) { - BIND_3(int64, value) + BIND_3(int64, value, stmt) } template <> void Statement::bind(int offset, double value) { - BIND_3(double, value) + BIND_3(double, value, stmt) } template <> void Statement::bind(int offset, bool value) { - BIND_3(int, value) + BIND_3(int, value, stmt) } template <> void Statement::bind(int offset, const char *value) { - BIND_5(text, value, -1, nullptr) + BIND_5(text, value, -1, nullptr, stmt) } void Statement::bind(int offset, const std::string &value, bool retain) { - BIND_5(blob, value.data(), int(value.size()), retain ? SQLITE_TRANSIENT : SQLITE_STATIC) + BIND_5(blob, value.data(), int(value.size()), retain ? SQLITE_TRANSIENT : SQLITE_STATIC, stmt) } bool Statement::run() { @@ -143,7 +147,8 @@ bool Statement::run() { } else if (err == SQLITE_ROW) { return true; } else { - throw std::runtime_error("failed to run statement"); + CHECK_SQLITE_OK_STMT(err, stmt) + return false; } } diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index b8d47159c..3f95db7c2 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -9,6 +9,7 @@ #include #include "sqlite3.hpp" +#include #include @@ -88,10 +89,10 @@ struct SQLiteCache::ActionDispatcher { template void operator()(T &t) { cache.process(t); } }; -SQLiteCache::SQLiteCache(const std::string &path_) +SQLiteCache::SQLiteCache(const std::string& path_) : path(path_), loop(uv_loop_new()), - queue(new Queue(loop, [this](Action &action) { + queue(new Queue(loop, [this](Action& action) { mapbox::util::apply_visitor(ActionDispatcher{ *this }, action); })), thread([this]() { @@ -99,8 +100,16 @@ SQLiteCache::SQLiteCache(const std::string &path_) 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() { @@ -140,6 +149,10 @@ void SQLiteCache::put(const Resource &resource, std::shared_ptr void SQLiteCache::createDatabase() { db = util::make_unique(path.c_str(), ReadWrite | Create); + createSchema(); +} + +void SQLiteCache::createSchema() { constexpr const char *const sql = "" "CREATE TABLE IF NOT EXISTS `http_cache` (" " `url` TEXT PRIMARY KEY NOT NULL," @@ -155,111 +168,134 @@ void SQLiteCache::createDatabase() { try { db->exec(sql); - } catch(mapbox::sqlite::Exception &) { + schema = true; + } catch(mapbox::sqlite::Exception &ex) { + Log::Error(Event::Database, ex.code, ex.what()); + // Creating the database table + index failed. That means there may already be one, likely // with different columsn. Drop it and try to create a new one. - try { - db->exec("DROP TABLE IF EXISTS `http_cache`"); - db->exec(sql); - } catch (mapbox::sqlite::Exception &ex) { - Log::Error(Event::Database, "Failed to create database: %s", ex.what()); - db.reset(); - } + db->exec("DROP TABLE IF EXISTS `http_cache`"); + db->exec(sql); } } void SQLiteCache::process(GetAction &action) { - // This is called in the SQLite event loop. - if (!db) { - createDatabase(); - } - - if (!getStmt) { - // Initialize the statement 0 1 - getStmt = util::make_unique(db->prepare("SELECT `status`, `modified`, " - // 2 3 4 5 1 - "`etag`, `expires`, `data`, `compressed` FROM `http_cache` WHERE `url` = ?")); - } else { - getStmt->reset(); - } - - const std::string unifiedURL = unifyMapboxURLs(action.resource.url); - getStmt->bind(1, unifiedURL.c_str()); - if (getStmt->run()) { - // There is data. - auto response = util::make_unique(); - response->status = Response::Status(getStmt->get(0)); - response->modified = getStmt->get(1); - response->etag = getStmt->get(2); - response->expires = getStmt->get(3); - response->data = getStmt->get(4); - if (getStmt->get(5)) { // == compressed - response->data = util::decompress(response->data); + try { + // This is called in the SQLite event loop. + if (!db) { + createDatabase(); } - action.callback(std::move(response)); - } else { - // There is no data. + + if (!schema) { + createSchema(); + } + + if (!getStmt) { + // Initialize the statement 0 1 + getStmt = util::make_unique(db->prepare("SELECT `status`, `modified`, " + // 2 3 4 5 1 + "`etag`, `expires`, `data`, `compressed` FROM `http_cache` WHERE `url` = ?")); + } else { + getStmt->reset(); + } + + const std::string unifiedURL = unifyMapboxURLs(action.resource.url); + getStmt->bind(1, unifiedURL.c_str()); + if (getStmt->run()) { + // There is data. + auto response = util::make_unique(); + response->status = Response::Status(getStmt->get(0)); + response->modified = getStmt->get(1); + response->etag = getStmt->get(2); + response->expires = getStmt->get(3); + response->data = getStmt->get(4); + if (getStmt->get(5)) { // == compressed + response->data = util::decompress(response->data); + } + action.callback(std::move(response)); + } else { + // There is no data. + action.callback(nullptr); + } + } catch (mapbox::sqlite::Exception& ex) { + Log::Error(Event::Database, ex.code, ex.what()); action.callback(nullptr); } } void SQLiteCache::process(PutAction &action) { - if (!db) { - createDatabase(); + try { + if (!db) { + createDatabase(); + } + + if (!schema) { + createSchema(); + } + + if (!putStmt) { + putStmt = util::make_unique(db->prepare("REPLACE INTO `http_cache` (" + // 1 2 3 4 5 6 7 8 + "`url`, `status`, `kind`, `modified`, `etag`, `expires`, `data`, `compressed`" + ") VALUES(?, ?, ?, ?, ?, ?, ?, ?)")); + } else { + putStmt->reset(); + } + + const std::string unifiedURL = unifyMapboxURLs(action.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); + + std::string data; + if (action.resource.kind != Resource::Image) { + // Do not compress images, since they are typically compressed already. + data = util::compress(action.response->data); + } + + if (!data.empty() && data.size() < action.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(8 /* compressed */, false); + } + + putStmt->run(); + } catch (mapbox::sqlite::Exception& ex) { + Log::Error(Event::Database, ex.code, ex.what()); } - - if (!putStmt) { - putStmt = util::make_unique(db->prepare("REPLACE INTO `http_cache` (" - // 1 2 3 4 5 6 7 8 - "`url`, `status`, `kind`, `modified`, `etag`, `expires`, `data`, `compressed`" - ") VALUES(?, ?, ?, ?, ?, ?, ?, ?)")); - } else { - putStmt->reset(); - } - - const std::string unifiedURL = unifyMapboxURLs(action.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); - - std::string data; - if (action.resource.kind != Resource::Image) { - // Do not compress images, since they are typically compressed already. - data = util::compress(action.response->data); - } - - if (!data.empty() && data.size() < action.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(8 /* compressed */, false); - } - - putStmt->run(); } void SQLiteCache::process(RefreshAction &action) { - if (!db) { - createDatabase(); - } + try { + if (!db) { + createDatabase(); + } - if (!refreshStmt) { - refreshStmt = util::make_unique( // 1 2 - db->prepare("UPDATE `http_cache` SET `expires` = ? WHERE `url` = ?")); - } else { - refreshStmt->reset(); - } + if (!schema) { + createSchema(); + } - const std::string unifiedURL = unifyMapboxURLs(action.resource.url); - refreshStmt->bind(1, int64_t(action.expires)); - refreshStmt->bind(2, unifiedURL.c_str()); - refreshStmt->run(); + if (!refreshStmt) { + refreshStmt = util::make_unique( // 1 2 + db->prepare("UPDATE `http_cache` SET `expires` = ? WHERE `url` = ?")); + } else { + refreshStmt->reset(); + } + + const std::string unifiedURL = unifyMapboxURLs(action.resource.url); + refreshStmt->bind(1, int64_t(action.expires)); + refreshStmt->bind(2, unifiedURL.c_str()); + refreshStmt->run(); + } catch (mapbox::sqlite::Exception& ex) { + Log::Error(Event::Database, ex.code, ex.what()); + } } void SQLiteCache::process(StopAction &) { diff --git a/src/mbgl/platform/log.cpp b/src/mbgl/platform/log.cpp index d6cbc4fd1..2a761c626 100644 --- a/src/mbgl/platform/log.cpp +++ b/src/mbgl/platform/log.cpp @@ -17,6 +17,12 @@ void Log::setObserver(std::unique_ptr observer) { currentObserver = std::move(observer); } +std::unique_ptr Log::removeObserver() { + std::unique_ptr observer; + std::swap(observer, currentObserver); + return observer; +} + void Log::record(EventSeverity severity, Event event, const std::string &msg) { record(severity, event, -1, msg); } diff --git a/test/fixtures/fixture_log_observer.cpp b/test/fixtures/fixture_log_observer.cpp index f66d18543..b3b8e56fd 100644 --- a/test/fixtures/fixture_log_observer.cpp +++ b/test/fixtures/fixture_log_observer.cpp @@ -8,6 +8,9 @@ FixtureLogObserver::LogMessage::LogMessage(EventSeverity severity_, Event event_ : severity(severity_), event(event_), code(code_), msg(msg_) { } +FixtureLogObserver::LogMessage::LogMessage() : severity(), event(), code(), msg() { +} + bool FixtureLogObserver::LogMessage::operator==(const LogMessage &rhs) const { return (!severity || !rhs.severity || severity.get() == rhs.severity.get()) && (!event || !rhs.event || event.get() == rhs.event.get()) && diff --git a/test/fixtures/fixture_log_observer.hpp b/test/fixtures/fixture_log_observer.hpp index bdb7ea2ca..7e419a617 100644 --- a/test/fixtures/fixture_log_observer.hpp +++ b/test/fixtures/fixture_log_observer.hpp @@ -2,7 +2,6 @@ #define MBGL_TEST_FIXTURE_LOG_OBSERVER #include -#include #include #include @@ -11,10 +10,11 @@ namespace mbgl { -class FixtureLogObserver : public Log::Observer, private util::noncopyable { +class FixtureLogObserver : public Log::Observer { public: struct LogMessage { LogMessage(EventSeverity severity_, Event event_, int64_t code_, const std::string &msg_); + LogMessage(); bool operator==(const LogMessage &rhs) const; diff --git a/test/storage/database.cpp b/test/storage/database.cpp new file mode 100644 index 000000000..80cb95619 --- /dev/null +++ b/test/storage/database.cpp @@ -0,0 +1,338 @@ +#include "storage.hpp" +#include "../fixtures/fixture_log_observer.hpp" + +#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; + + Log::setObserver(util::make_unique()); + + ScopedTest test([&] { + auto observer = Log::removeObserver(); + 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"); + + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + test.finish(); + }); +} + +void createDir(const char* name) { + const int ret = mkdir(name, 0755); + if (ret == -1) { + ASSERT_EQ(EEXIST, errno); + } else { + ASSERT_EQ(0, ret); + } +} + +void deleteFile(const char* name) { + const int ret = unlink(name); + if (ret == -1) { + ASSERT_EQ(ENOENT, errno); + } else { + ASSERT_EQ(0, ret); + } +} + +TEST_F(Storage, DatabaseCreate) { + using namespace mbgl; + + createDir("test/fixtures/database"); + deleteFile("test/fixtures/database/cache.db"); + + Log::setObserver(util::make_unique()); + + ScopedTest test([&] { + Log::removeObserver(); + }); + + SQLiteCache cache("test/fixtures/database/cache.db"); + + cache.get({ Resource::Unknown, "mapbox://test" }, [&] (std::unique_ptr res) { + EXPECT_EQ(nullptr, res.get()); + test.finish(); + }); +} + +class FileLock { +public: + FileLock(const std::string& path) { + fd = open(path.c_str(), O_RDONLY | O_NOCTTY | O_CREAT, 0666); + if (fd <= 0) { + throw std::runtime_error("Could not open file"); + } + lock(); + } + + void lock() { + if (0 != flock(fd, LOCK_EX)) { + throw std::runtime_error("Could not lock file"); + } + } + + void unlock() { + if (0 != flock(fd, LOCK_UN)) { + throw std::runtime_error("Could not unlock file"); + } + } + + ~FileLock() { + unlock(); + } + +private: + int fd; +}; + +TEST_F(Storage, DatabaseLockedRead) { + using namespace mbgl; + + // Create a locked file. + createDir("test/fixtures/database"); + deleteFile("test/fixtures/database/locked.db"); + FileLock guard("test/fixtures/database/locked.db"); + + auto cache = util::make_unique("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(); + }); + promise.get_future().get(); + + // Make sure that we got a few "database locked" errors + auto observer = Log::removeObserver(); + auto flo = dynamic_cast(observer.get()); + EXPECT_EQ(2ul, flo->count({ EventSeverity::Error, Event::Database, 5, "database is locked" })); + } + + // Then, unlock the file and try again. + guard.unlock(); + + { + // 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(); + }); + promise.get_future().get(); + + // Make sure that we got a no errors + Log::removeObserver(); + } + + // Explicitly delete the Cache now. + cache.reset(); +} + + + +TEST_F(Storage, DatabaseLockedWrite) { + using namespace mbgl; + + // Create a locked file. + createDir("test/fixtures/database"); + deleteFile("test/fixtures/database/locked.db"); + FileLock guard("test/fixtures/database/locked.db"); + + auto cache = util::make_unique("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(); + }); + promise.get_future().get(); + + auto observer = Log::removeObserver(); + auto flo = dynamic_cast(observer.get()); + EXPECT_EQ(4ul, flo->count({ EventSeverity::Error, Event::Database, 5, "database is locked" })); + } + + // Then, unlock the file and try again. + guard.unlock(); + + { + // 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(); + }); + promise.get_future().get(); + + // Make sure that we got a no errors + Log::removeObserver(); + } + + // Explicitly delete the Cache now. + cache.reset(); +} + + + + +TEST_F(Storage, DatabaseLockedRefresh) { + using namespace mbgl; + + // Create a locked file. + createDir("test/fixtures/database"); + deleteFile("test/fixtures/database/locked.db"); + + auto cache = util::make_unique("test/fixtures/database/locked.db"); + + // 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(); + }); + promise.get_future().get(); + + auto observer = Log::removeObserver(); + auto flo = dynamic_cast(observer.get()); + EXPECT_EQ(4ul, flo->count({ EventSeverity::Error, Event::Database, 5, "database is locked" })); + } + + { + // 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(); + }); + promise.get_future().get(); + + // Make sure that we got the right errors. + auto observer = Log::removeObserver(); + 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(); +} + + + +TEST_F(Storage, DatabaseDeleted) { + using namespace mbgl; + + // Create a locked file. + createDir("test/fixtures/database"); + deleteFile("test/fixtures/database/locked.db"); + + auto cache = util::make_unique("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(); + }); + promise.get_future().get(); + + Log::removeObserver(); + } + + deleteFile("test/fixtures/database/locked.db"); + + { + // 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(); + }); + promise.get_future().get(); + + auto observer = Log::removeObserver(); + 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(); +} diff --git a/test/test.gyp b/test/test.gyp index c333a4006..d9e234fe0 100644 --- a/test/test.gyp +++ b/test/test.gyp @@ -54,6 +54,7 @@ 'storage/storage.cpp', 'storage/cache_response.cpp', 'storage/cache_revalidate.cpp', + 'storage/database.cpp', 'storage/directory_reading.cpp', 'storage/file_reading.cpp', 'storage/http_cancel.cpp', From 4b66c490b41e8ea18428828707ed4327734f92dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Wed, 8 Apr 2015 14:24:03 +0200 Subject: [PATCH 2/3] guard against invalid database files --- platform/default/sqlite_cache.cpp | 19 +++++++++++-- src/mbgl/util/io.cpp | 10 ++++++- src/mbgl/util/io.hpp | 9 ++++++ test/storage/database.cpp | 46 ++++++++++++++++++++++++++++++- 4 files changed, 79 insertions(+), 5 deletions(-) diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index 3f95db7c2..a3114098c 100644 --- a/platform/default/sqlite_cache.cpp +++ b/platform/default/sqlite_cache.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include "sqlite3.hpp" @@ -169,8 +170,20 @@ void SQLiteCache::createSchema() { try { db->exec(sql); schema = true; - } catch(mapbox::sqlite::Exception &ex) { - Log::Error(Event::Database, ex.code, ex.what()); + } catch (mapbox::sqlite::Exception &ex) { + + if (ex.code == SQLITE_NOTADB) { + Log::Warning(Event::Database, "Trashing invalid database"); + db.reset(); + try { + util::deleteFile(path); + } catch (util::IOException& ioEx) { + Log::Error(Event::Database, ex.code, ex.what()); + } + db = util::make_unique(path.c_str(), ReadWrite | Create); + } else { + Log::Error(Event::Database, ex.code, ex.what()); + } // Creating the database table + index failed. That means there may already be one, likely // with different columsn. Drop it and try to create a new one. @@ -269,7 +282,7 @@ void SQLiteCache::process(PutAction &action) { putStmt->run(); } catch (mapbox::sqlite::Exception& ex) { Log::Error(Event::Database, ex.code, ex.what()); - } + } } void SQLiteCache::process(RefreshAction &action) { diff --git a/src/mbgl/util/io.cpp b/src/mbgl/util/io.cpp index 76f7c35ad..bb4c3595c 100644 --- a/src/mbgl/util/io.cpp +++ b/src/mbgl/util/io.cpp @@ -4,7 +4,8 @@ #include #include #include -#include + +#include namespace mbgl { namespace util { @@ -30,5 +31,12 @@ std::string read_file(const std::string &filename) { } } +void deleteFile(const std::string& filename) { + const int ret = unlink(filename.c_str()); + if (ret == -1) { + throw IOException(errno, "failed to unlink file"); + } +} + } } diff --git a/src/mbgl/util/io.hpp b/src/mbgl/util/io.hpp index e95fc16d9..bf15253ee 100644 --- a/src/mbgl/util/io.hpp +++ b/src/mbgl/util/io.hpp @@ -2,13 +2,22 @@ #define MBGL_UTIL_IO #include +#include namespace mbgl { namespace util { +struct IOException : std::runtime_error { + inline IOException(int err, const char* msg) : std::runtime_error(msg), code(err) { + } + const int code = 0; +}; + void write_file(const std::string &filename, const std::string &data); std::string read_file(const std::string &filename); +void deleteFile(const std::string& filename); + } } diff --git a/test/storage/database.cpp b/test/storage/database.cpp index 80cb95619..c60388be4 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -6,8 +6,9 @@ #include #include #include +#include -#include +#include class ScopedTest { public: @@ -66,6 +67,12 @@ void deleteFile(const char* name) { } } + +void writeFile(const char* name, const std::string& data) { + mbgl::util::write_file(name, data); +} + + TEST_F(Storage, DatabaseCreate) { using namespace mbgl; @@ -336,3 +343,40 @@ TEST_F(Storage, DatabaseDeleted) { // Explicitly delete the Cache now. cache.reset(); } + + + +TEST_F(Storage, DatabaseInvalid) { + using namespace mbgl; + + // Create a locked file. + createDir("test/fixtures/database"); + 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"); + + 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(); + }); + promise.get_future().get(); + + auto observer = Log::removeObserver(); + 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 59bd300b4356cb8cee396032e8eb8324376fb630 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Wed, 8 Apr 2015 16:24:10 +0200 Subject: [PATCH 3/3] use sqlite itself to lock the database file rather than those unportable bsd locks --- test/storage/database.cpp | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/test/storage/database.cpp b/test/storage/database.cpp index c60388be4..1a2b618a5 100644 --- a/test/storage/database.cpp +++ b/test/storage/database.cpp @@ -8,7 +8,7 @@ #include #include -#include +#include class ScopedTest { public: @@ -96,31 +96,40 @@ TEST_F(Storage, DatabaseCreate) { class FileLock { public: FileLock(const std::string& path) { - fd = open(path.c_str(), O_RDONLY | O_NOCTTY | O_CREAT, 0666); - if (fd <= 0) { - throw std::runtime_error("Could not open file"); + const int err = sqlite3_open_v2(path.c_str(), &db, SQLITE_OPEN_CREATE | SQLITE_OPEN_READWRITE, nullptr); + if (err != SQLITE_OK) { + throw std::runtime_error("Could not open db"); } lock(); } void lock() { - if (0 != flock(fd, LOCK_EX)) { - throw std::runtime_error("Could not lock file"); + assert(!locked); + const int err = sqlite3_exec(db, "begin exclusive transaction", nullptr, nullptr, nullptr); + if (err != SQLITE_OK) { + throw std::runtime_error("Could not lock db"); } + locked = true; } void unlock() { - if (0 != flock(fd, LOCK_UN)) { - throw std::runtime_error("Could not unlock file"); + assert(locked); + const int err = sqlite3_exec(db, "commit", nullptr, nullptr, nullptr); + if (err != SQLITE_OK) { + throw std::runtime_error("Could not unlock db"); } + locked = false; } ~FileLock() { - unlock(); + if (locked) { + unlock(); + } } private: - int fd; + sqlite3* db; + bool locked = false; }; TEST_F(Storage, DatabaseLockedRead) {