From 591012401072e63b89071787d90bf5ae4362dca1 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Tue, 9 Feb 2016 11:16:55 -0800 Subject: [core] Reset SQLite statements after use in order to release locks --- platform/default/mbgl/storage/offline_database.cpp | 226 +++++++++++---------- platform/default/mbgl/storage/offline_database.hpp | 16 +- platform/default/sqlite3.cpp | 5 + platform/default/sqlite3.hpp | 1 + test/storage/offline_database.cpp | 32 +++ 5 files changed, 168 insertions(+), 112 deletions(-) diff --git a/platform/default/mbgl/storage/offline_database.cpp b/platform/default/mbgl/storage/offline_database.cpp index fb0dae0b2b..baaa1628d5 100644 --- a/platform/default/mbgl/storage/offline_database.cpp +++ b/platform/default/mbgl/storage/offline_database.cpp @@ -17,6 +17,11 @@ using namespace mapbox::sqlite; // If you change the schema you must write a migration from the previous version. static const uint32_t schemaVersion = 2; +OfflineDatabase::Statement::~Statement() { + stmt.reset(); + stmt.clearBindings(); +} + OfflineDatabase::OfflineDatabase(const std::string& path_) : path(path_) { ensureSchema(); @@ -40,7 +45,7 @@ void OfflineDatabase::ensureSchema() { db->setBusyTimeout(Milliseconds::max()); { - Statement userVersionStmt(db->prepare("PRAGMA user_version")); + auto userVersionStmt = db->prepare("PRAGMA user_version"); userVersionStmt.run(); switch (userVersionStmt.get(0)) { case 0: break; // cache-only database; ok to delete @@ -85,15 +90,14 @@ void OfflineDatabase::removeExisting() { } } -mapbox::sqlite::Statement& OfflineDatabase::getStatement(const char * sql) { +OfflineDatabase::Statement OfflineDatabase::getStatement(const char * sql) { auto it = statements.find(sql); if (it != statements.end()) { - it->second->reset(); - return *it->second; + return Statement(*it->second); } - return *(statements[sql] = std::make_unique(db->prepare(sql))); + return Statement(*statements.emplace(sql, std::make_unique(db->prepare(sql))).first->second); } optional OfflineDatabase::get(const Resource& resource) { @@ -120,28 +124,28 @@ uint64_t OfflineDatabase::put(const Resource& resource, const Response& response } optional OfflineDatabase::getResource(const Resource& resource) { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( // 0 1 2 3 4 "SELECT etag, expires, modified, data, compressed " "FROM resources " "WHERE url = ?"); - stmt.bind(1, resource.url); + stmt->bind(1, resource.url); - if (!stmt.run()) { + if (!stmt->run()) { return {}; } Response response; - response.etag = stmt.get>(0); - response.expires = stmt.get>(1); - response.modified = stmt.get>(2); + response.etag = stmt->get>(0); + response.expires = stmt->get>(1); + response.modified = stmt->get>(2); - optional data = stmt.get>(3); + optional data = stmt->get>(3); if (!data) { response.noContent = true; - } else if (stmt.get(4)) { + } else if (stmt->get(4)) { response.data = std::make_shared(util::decompress(*data)); } else { response.data = std::make_shared(*data); @@ -152,54 +156,54 @@ optional OfflineDatabase::getResource(const Resource& resource) { uint64_t OfflineDatabase::putResource(const Resource& resource, const Response& response) { if (response.notModified) { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( // 1 2 3 "UPDATE resources SET accessed = ?, expires = ? WHERE url = ?"); - stmt.bind(1, SystemClock::now()); - stmt.bind(2, response.expires); - stmt.bind(3, resource.url); - stmt.run(); + stmt->bind(1, SystemClock::now()); + stmt->bind(2, response.expires); + stmt->bind(3, resource.url); + stmt->run(); return 0; } else { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( // 1 2 3 4 5 6 7 8 "REPLACE INTO resources (url, kind, etag, expires, modified, accessed, data, compressed) " "VALUES (?, ?, ?, ?, ?, ?, ?, ?)"); - stmt.bind(1 /* url */, resource.url); - stmt.bind(2 /* kind */, int(resource.kind)); - stmt.bind(3 /* etag */, response.etag); - stmt.bind(4 /* expires */, response.expires); - stmt.bind(5 /* modified */, response.modified); - stmt.bind(6 /* accessed */, SystemClock::now()); + stmt->bind(1 /* url */, resource.url); + stmt->bind(2 /* kind */, int(resource.kind)); + stmt->bind(3 /* etag */, response.etag); + stmt->bind(4 /* expires */, response.expires); + stmt->bind(5 /* modified */, response.modified); + stmt->bind(6 /* accessed */, SystemClock::now()); std::string data; uint64_t size = 0; if (response.noContent) { - stmt.bind(7 /* data */, nullptr); - stmt.bind(8 /* compressed */, false); + stmt->bind(7 /* data */, nullptr); + stmt->bind(8 /* compressed */, false); } else { data = util::compress(*response.data); if (data.size() < response.data->size()) { size = data.size(); - stmt.bindBlob(7 /* data */, data.data(), size, false); - stmt.bind(8 /* compressed */, true); + stmt->bindBlob(7 /* data */, data.data(), size, false); + stmt->bind(8 /* compressed */, true); } else { size = response.data->size(); - stmt.bindBlob(7 /* data */, response.data->data(), size, false); - stmt.bind(8 /* compressed */, false); + stmt->bindBlob(7 /* data */, response.data->data(), size, false); + stmt->bind(8 /* compressed */, false); } } - stmt.run(); + stmt->run(); return size; } } optional OfflineDatabase::getTile(const Resource::TileData& tile) { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( // 0 1 2 3 4 "SELECT etag, expires, modified, data, compressed " "FROM tilesets, tiles " @@ -210,26 +214,26 @@ optional OfflineDatabase::getTile(const Resource::TileData& tile) { "AND tiles.z = ? " // 5 "AND tilesets.id = tiles.tileset_id "); - stmt.bind(1, tile.urlTemplate); - stmt.bind(2, tile.pixelRatio); - stmt.bind(3, tile.x); - stmt.bind(4, tile.y); - stmt.bind(5, tile.z); + stmt->bind(1, tile.urlTemplate); + stmt->bind(2, tile.pixelRatio); + stmt->bind(3, tile.x); + stmt->bind(4, tile.y); + stmt->bind(5, tile.z); - if (!stmt.run()) { + if (!stmt->run()) { return {}; } Response response; - response.etag = stmt.get>(0); - response.expires = stmt.get>(1); - response.modified = stmt.get>(2); + response.etag = stmt->get>(0); + response.expires = stmt->get>(1); + response.modified = stmt->get>(2); - optional data = stmt.get>(3); + optional data = stmt->get>(3); if (!data) { response.noContent = true; - } else if (stmt.get(4)) { + } else if (stmt->get(4)) { response.data = std::make_shared(util::decompress(*data)); } else { response.data = std::make_shared(*data); @@ -240,7 +244,7 @@ optional OfflineDatabase::getTile(const Resource::TileData& tile) { uint64_t OfflineDatabase::putTile(const Resource::TileData& tile, const Response& response) { if (response.notModified) { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( "UPDATE tiles SET accessed = ?1, expires = ?2 " "WHERE tileset_id = ( " " SELECT id FROM tilesets " @@ -250,76 +254,76 @@ uint64_t OfflineDatabase::putTile(const Resource::TileData& tile, const Response "AND tiles.y = ?6 " "AND tiles.z = ?7 "); - stmt.bind(1, SystemClock::now()); - stmt.bind(2, response.expires); - stmt.bind(3, tile.urlTemplate); - stmt.bind(4, tile.pixelRatio); - stmt.bind(5, tile.x); - stmt.bind(6, tile.y); - stmt.bind(7, tile.z); - stmt.run(); + stmt->bind(1, SystemClock::now()); + stmt->bind(2, response.expires); + stmt->bind(3, tile.urlTemplate); + stmt->bind(4, tile.pixelRatio); + stmt->bind(5, tile.x); + stmt->bind(6, tile.y); + stmt->bind(7, tile.z); + stmt->run(); return 0; } else { - mapbox::sqlite::Statement& stmt1 = getStatement( + Statement stmt1 = getStatement( "REPLACE INTO tilesets (url_template, pixel_ratio) " "VALUES (?1, ?2) "); - stmt1.bind(1 /* url_template */, tile.urlTemplate); - stmt1.bind(2 /* pixel_ratio */, tile.pixelRatio); - stmt1.run(); + stmt1->bind(1 /* url_template */, tile.urlTemplate); + stmt1->bind(2 /* pixel_ratio */, tile.pixelRatio); + stmt1->run(); - mapbox::sqlite::Statement& stmt2 = getStatement( + Statement stmt2 = getStatement( "REPLACE INTO tiles (tileset_id, x, y, z, modified, etag, expires, accessed, data, compressed) " "SELECT tilesets.id, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11 " "FROM tilesets " "WHERE url_template = ?1 " "AND pixel_ratio = ?2 "); - stmt2.bind(1 /* url_template */, tile.urlTemplate); - stmt2.bind(2 /* pixel_ratio */, tile.pixelRatio); - stmt2.bind(3 /* x */, tile.x); - stmt2.bind(4 /* y */, tile.y); - stmt2.bind(5 /* z */, tile.z); - stmt2.bind(6 /* modified */, response.modified); - stmt2.bind(7 /* etag */, response.etag); - stmt2.bind(8 /* expires */, response.expires); - stmt2.bind(9 /* accessed */, SystemClock::now()); + stmt2->bind(1 /* url_template */, tile.urlTemplate); + stmt2->bind(2 /* pixel_ratio */, tile.pixelRatio); + stmt2->bind(3 /* x */, tile.x); + stmt2->bind(4 /* y */, tile.y); + stmt2->bind(5 /* z */, tile.z); + stmt2->bind(6 /* modified */, response.modified); + stmt2->bind(7 /* etag */, response.etag); + stmt2->bind(8 /* expires */, response.expires); + stmt2->bind(9 /* accessed */, SystemClock::now()); std::string data; uint64_t size = 0; if (response.noContent) { - stmt2.bind(10 /* data */, nullptr); - stmt2.bind(11 /* compressed */, false); + stmt2->bind(10 /* data */, nullptr); + stmt2->bind(11 /* compressed */, false); } else { data = util::compress(*response.data); if (data.size() < response.data->size()) { size = data.size(); - stmt2.bindBlob(10 /* data */, data.data(), size, false); - stmt2.bind(11 /* compressed */, true); + stmt2->bindBlob(10 /* data */, data.data(), size, false); + stmt2->bind(11 /* compressed */, true); } else { size = response.data->size(); - stmt2.bindBlob(10 /* data */, response.data->data(), size, false); - stmt2.bind(11 /* compressed */, false); + stmt2->bindBlob(10 /* data */, response.data->data(), size, false); + stmt2->bind(11 /* compressed */, false); } } - stmt2.run(); + stmt2->run(); return size; } } std::vector OfflineDatabase::listRegions() { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( "SELECT id, definition, description FROM regions"); std::vector result; - while (stmt.run()) { + while (stmt->run()) { result.push_back(OfflineRegion( - stmt.get(0), - decodeOfflineRegionDefinition(stmt.get(1)), - stmt.get>(2))); + stmt->get(0), + decodeOfflineRegionDefinition(stmt->get(1)), + stmt->get>(2))); } return std::move(result); @@ -327,23 +331,23 @@ std::vector OfflineDatabase::listRegions() { OfflineRegion OfflineDatabase::createRegion(const OfflineRegionDefinition& definition, const OfflineRegionMetadata& metadata) { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( "INSERT INTO regions (definition, description) " "VALUES (?1, ?2) "); - stmt.bind(1, encodeOfflineRegionDefinition(definition)); - stmt.bindBlob(2, metadata); - stmt.run(); + stmt->bind(1, encodeOfflineRegionDefinition(definition)); + stmt->bindBlob(2, metadata); + stmt->run(); return OfflineRegion(db->lastInsertRowid(), definition, metadata); } void OfflineDatabase::deleteRegion(OfflineRegion&& region) { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( "DELETE FROM regions WHERE id = ?"); - stmt.bind(1, region.getID()); - stmt.run(); + stmt->bind(1, region.getID()); + stmt->run(); } optional OfflineDatabase::getRegionResource(int64_t regionID, const Resource& resource) { @@ -364,45 +368,45 @@ uint64_t OfflineDatabase::putRegionResource(int64_t regionID, const Resource& re void OfflineDatabase::markUsed(int64_t regionID, const Resource& resource) { if (resource.kind == Resource::Kind::Tile) { - mapbox::sqlite::Statement& stmt1 = getStatement( + Statement stmt1 = getStatement( "REPLACE INTO region_tiles (region_id, tileset_id, x, y, z) " "SELECT ?1, tilesets.id, ?4, ?5, ?6 " "FROM tilesets " "WHERE url_template = ?2 " "AND pixel_ratio = ?3 "); - stmt1.bind(1, regionID); - stmt1.bind(2, (*resource.tileData).urlTemplate); - stmt1.bind(3, (*resource.tileData).pixelRatio); - stmt1.bind(4, (*resource.tileData).x); - stmt1.bind(5, (*resource.tileData).y); - stmt1.bind(6, (*resource.tileData).z); - stmt1.run(); + stmt1->bind(1, regionID); + stmt1->bind(2, (*resource.tileData).urlTemplate); + stmt1->bind(3, (*resource.tileData).pixelRatio); + stmt1->bind(4, (*resource.tileData).x); + stmt1->bind(5, (*resource.tileData).y); + stmt1->bind(6, (*resource.tileData).z); + stmt1->run(); } else { - mapbox::sqlite::Statement& stmt1 = getStatement( + Statement stmt1 = getStatement( "REPLACE INTO region_resources (region_id, resource_url) " "VALUES (?1, ?2) "); - stmt1.bind(1, regionID); - stmt1.bind(2, resource.url); - stmt1.run(); + stmt1->bind(1, regionID); + stmt1->bind(2, resource.url); + stmt1->run(); } } OfflineRegionDefinition OfflineDatabase::getRegionDefinition(int64_t regionID) { - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( "SELECT definition FROM regions WHERE id = ?1"); - stmt.bind(1, regionID); - stmt.run(); + stmt->bind(1, regionID); + stmt->run(); - return decodeOfflineRegionDefinition(stmt.get(0)); + return decodeOfflineRegionDefinition(stmt->get(0)); } OfflineRegionStatus OfflineDatabase::getRegionCompletedStatus(int64_t regionID) { OfflineRegionStatus result; - mapbox::sqlite::Statement& stmt = getStatement( + Statement stmt = getStatement( "SELECT COUNT(*), SUM(size) FROM ( " " SELECT LENGTH(data) as size " " FROM region_resources, resources " @@ -418,26 +422,26 @@ OfflineRegionStatus OfflineDatabase::getRegionCompletedStatus(int64_t regionID) " AND tiles.y = region_tiles.y " ") "); - stmt.bind(1, regionID); - stmt.run(); + stmt->bind(1, regionID); + stmt->run(); - result.completedResourceCount = stmt.get(0); - result.completedResourceSize = stmt.get(1); + result.completedResourceCount = stmt->get(0); + result.completedResourceSize = stmt->get(1); return result; } void OfflineDatabase::removeUnusedResources() { - mapbox::sqlite::Statement& stmt1 = getStatement( + Statement stmt1 = getStatement( "DELETE FROM resources " "WHERE ROWID NOT IN ( " " SELECT resources.ROWID " " FROM resources, region_resources " " WHERE resources.url = region_resources.resource_url " ") "); - stmt1.run(); + stmt1->run(); - mapbox::sqlite::Statement& stmt2 = getStatement( + Statement stmt2 = getStatement( "DELETE FROM tiles " "WHERE ROWID NOT IN ( " " SELECT tiles.ROWID " @@ -447,7 +451,7 @@ void OfflineDatabase::removeUnusedResources() { " AND tiles.x = region_tiles.x " " AND tiles.y = region_tiles.y " ") "); - stmt2.run(); + stmt2->run(); } } // namespace mbgl diff --git a/platform/default/mbgl/storage/offline_database.hpp b/platform/default/mbgl/storage/offline_database.hpp index 554bb16068..b0b02f871f 100644 --- a/platform/default/mbgl/storage/offline_database.hpp +++ b/platform/default/mbgl/storage/offline_database.hpp @@ -47,7 +47,21 @@ public: private: void ensureSchema(); void removeExisting(); - mapbox::sqlite::Statement& getStatement(const char *); + + class Statement { + public: + explicit Statement(mapbox::sqlite::Statement& stmt_) : stmt(stmt_) {} + Statement(Statement&&) = default; + Statement(const Statement&) = delete; + ~Statement(); + + mapbox::sqlite::Statement* operator->() { return &stmt; }; + + private: + mapbox::sqlite::Statement& stmt; + }; + + Statement getStatement(const char *); optional getTile(const Resource::TileData&); uint64_t putTile(const Resource::TileData&, const Response&); diff --git a/platform/default/sqlite3.cpp b/platform/default/sqlite3.cpp index 31b9d56566..5059c85c80 100644 --- a/platform/default/sqlite3.cpp +++ b/platform/default/sqlite3.cpp @@ -310,5 +310,10 @@ void Statement::reset() { sqlite3_reset(stmt); } +void Statement::clearBindings() { + assert(stmt); + sqlite3_clear_bindings(stmt); +} + } // namespace sqlite } // namespace mapbox diff --git a/platform/default/sqlite3.hpp b/platform/default/sqlite3.hpp index 9e511ebaf9..13e92bf07f 100644 --- a/platform/default/sqlite3.hpp +++ b/platform/default/sqlite3.hpp @@ -81,6 +81,7 @@ public: bool run(); void reset(); + void clearBindings(); private: sqlite3_stmt *stmt = nullptr; diff --git a/test/storage/offline_database.cpp b/test/storage/offline_database.cpp index 2018f6a40b..4af262deb1 100644 --- a/test/storage/offline_database.cpp +++ b/test/storage/offline_database.cpp @@ -7,6 +7,7 @@ #include #include +#include namespace { @@ -448,3 +449,34 @@ TEST(OfflineDatabase, CreateRegionInfiniteMaxZoom) { EXPECT_EQ(0, region.getDefinition().minZoom); EXPECT_EQ(INFINITY, region.getDefinition().maxZoom); } + +TEST(OfflineDatabase, ConcurrentUse) { + using namespace mbgl; + + createDir("test/fixtures/database"); + deleteFile("test/fixtures/database/offline.db"); + + OfflineDatabase db1("test/fixtures/database/offline.db"); + OfflineDatabase db2("test/fixtures/database/offline.db"); + + Resource resource { Resource::Style, "http://example.com/" }; + Response response; + response.noContent = true; + + std::thread thread1([&] { + for (auto i = 0; i < 100; i++) { + db1.put(resource, response); + EXPECT_TRUE(bool(db1.get(resource))); + } + }); + + std::thread thread2([&] { + for (auto i = 0; i < 100; i++) { + db2.put(resource, response); + EXPECT_TRUE(bool(db2.get(resource))); + } + }); + + thread1.join(); + thread2.join(); +} -- cgit v1.2.1