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: 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 --- platform/default/sqlite3.cpp | 33 +++--- platform/default/sqlite_cache.cpp | 208 ++++++++++++++++++++++---------------- 2 files changed, 141 insertions(+), 100 deletions(-) (limited to 'platform') diff --git a/platform/default/sqlite3.cpp b/platform/default/sqlite3.cpp index 19ecaba0bf..b2ae4e65bc 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 b8d47159ce..3f95db7c2d 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(); - } + try { + // 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(); - } + if (!schema) { + createSchema(); + } - 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); + 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(); } - action.callback(std::move(response)); - } else { - // There is no data. + + 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 (!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(); - } + if (!schema) { + createSchema(); + } - 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 (!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(); + } - 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); - } + 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); + } - putStmt->run(); + 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()); + } } 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(); + } + + 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(); + 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 &) { -- cgit v1.2.1 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: guard against invalid database files --- platform/default/sqlite_cache.cpp | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) (limited to 'platform') diff --git a/platform/default/sqlite_cache.cpp b/platform/default/sqlite_cache.cpp index 3f95db7c2d..a3114098c8 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) { -- cgit v1.2.1