From ec54ce2aa6eb002ec805f1b1eca8918c1be3b06e Mon Sep 17 00:00:00 2001 From: Mikhail Pozdnyakov Date: Mon, 11 Nov 2019 11:57:54 +0200 Subject: [PATCH] [core] Improve OfflineDatabase error handling - fixes missing exception handlers (e.g. runtim error from the `initialize()` method) - introduces generic exception handling mechanism to reduce the repeated code --- .../include/mbgl/storage/offline_database.hpp | 1 + .../src/mbgl/storage/offline_database.cpp | 116 +++++++++--------- 2 files changed, 62 insertions(+), 55 deletions(-) diff --git a/platform/default/include/mbgl/storage/offline_database.hpp b/platform/default/include/mbgl/storage/offline_database.hpp index e599094a6d8..f097cd54406 100644 --- a/platform/default/include/mbgl/storage/offline_database.hpp +++ b/platform/default/include/mbgl/storage/offline_database.hpp @@ -99,6 +99,7 @@ class OfflineDatabase : private util::noncopyable { void handleError(const mapbox::sqlite::Exception&, const char* action); void handleError(const util::IOException&, const char* action); void handleError(const std::runtime_error& ex, const char* action); + void handleError(const char* action); void removeExisting(); void removeOldCacheTable(); diff --git a/platform/default/src/mbgl/storage/offline_database.cpp b/platform/default/src/mbgl/storage/offline_database.cpp index 5aa5738f41f..4f813e906d5 100644 --- a/platform/default/src/mbgl/storage/offline_database.cpp +++ b/platform/default/src/mbgl/storage/offline_database.cpp @@ -17,10 +17,8 @@ OfflineDatabase::OfflineDatabase(std::string path_) : path(std::move(path_)) { try { initialize(); - } catch (const util::IOException& ex) { - handleError(ex, "open database"); - } catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "open database"); + } catch (...) { + handleError("open database"); } // Assume that we can't open the database right now and work with an empty database object. } @@ -80,10 +78,8 @@ void OfflineDatabase::cleanup() { try { statements.clear(); db.reset(); - } catch (const util::IOException& ex) { - handleError(ex, "close database"); - } catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "close database"); + } catch (...) { + handleError("close database"); } } @@ -125,7 +121,25 @@ void OfflineDatabase::handleError(const util::IOException& ex, const char* actio } void OfflineDatabase::handleError(const std::runtime_error& ex, const char* action) { - Log::Error(Event::Database, -1, "Can't %s: %s", action, ex.what()); + Log::Error(Event::Database, "Can't %s: %s", action, ex.what()); +} + +void OfflineDatabase::handleError(const char* action) { + // Note: mbgl-defined exceptions must be handled first. + try { + throw; + } catch (const mapbox::sqlite::Exception& ex) { + handleError(ex, action); + } catch (const util::IOException& ex) { + handleError(ex, action); + } catch (const MapboxTileLimitExceededException& ex) { + throw; // This is ours and must be handled on client side. + } catch (const std::runtime_error& ex) { + handleError(ex, action); + } catch (...) { + assert(false); + Log::Error(Event::Database, "Can't %s", action); + } } void OfflineDatabase::removeExisting() { @@ -209,11 +223,8 @@ optional OfflineDatabase::get(const Resource& resource) try { auto result = getInternal(resource); return result ? optional{ result->first } : nullopt; -} catch (const util::IOException& ex) { - handleError(ex, "read resource"); - return nullopt; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "read resource"); +} catch (...) { + handleError("read resource"); return nullopt; } @@ -248,9 +259,9 @@ std::pair OfflineDatabase::put(const Resource& resource, const R auto result = putInternal(resource, response, true); transaction.commit(); return result; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "write resource"); - return { false, 0 }; +} catch (...) { + handleError("write resource"); + return {false, 0}; } std::pair OfflineDatabase::putInternal(const Resource& resource, const Response& response, bool evict_) { @@ -665,8 +676,8 @@ std::exception_ptr OfflineDatabase::invalidateAmbientCache() try { resourceQuery.run(); return nullptr; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "invalidate ambient cache"); +} catch (...) { + handleError("invalidate ambient cache"); return std::current_exception(); } @@ -696,8 +707,8 @@ std::exception_ptr OfflineDatabase::clearAmbientCache() try { vacuum(); return nullptr; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "clear ambient cache"); +} catch (...) { + handleError("clear ambient cache"); return std::current_exception(); } @@ -732,8 +743,8 @@ std::exception_ptr OfflineDatabase::invalidateRegion(int64_t regionID) try { assert(db); return nullptr; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "invalidate region"); +} catch (...) { + handleError("invalidate region"); return std::current_exception(); } @@ -756,8 +767,8 @@ expected OfflineDatabase::listRegions() try } // Explicit move to avoid triggering the copy constructor. return { std::move(result) }; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "list regions"); +} catch (...) { + handleError("list regions"); return unexpected(std::current_exception()); } @@ -774,8 +785,8 @@ OfflineDatabase::createRegion(const OfflineRegionDefinition& definition, query.bindBlob(2, metadata); query.run(); return OfflineRegion(query.lastInsertRowId(), definition, metadata); -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "create region"); +} catch (...) { + handleError("create region"); return unexpected(std::current_exception()); } @@ -867,8 +878,8 @@ OfflineDatabase::updateMetadata(const int64_t regionID, const OfflineRegionMetad query.run(); return metadata; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "update region metadata"); +} catch (...) { + handleError("update region metadata"); return unexpected(std::current_exception()); } @@ -886,22 +897,22 @@ std::exception_ptr OfflineDatabase::deleteRegion(OfflineRegion&& region) try { // Ensure that the cached offlineTileCount value is recalculated. offlineMapboxTileCount = {}; return nullptr; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "delete region"); +} catch (...) { + handleError("delete region"); return std::current_exception(); } optional> OfflineDatabase::getRegionResource(const Resource& resource) try { return getInternal(resource); -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "read region resource"); +} catch (...) { + handleError("read region resource"); return nullopt; } optional OfflineDatabase::hasRegionResource(const Resource& resource) try { return hasInternal(resource); -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "query region resource"); +} catch (...) { + handleError("query region resource"); return nullopt; } @@ -915,8 +926,8 @@ uint64_t OfflineDatabase::putRegionResource(int64_t regionID, auto size = putRegionResourceInternal(regionID, resource, response); transaction.commit(); return size; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "write region resource"); +} catch (...) { + handleError("write region resource"); return 0; } @@ -961,8 +972,8 @@ void OfflineDatabase::putRegionResources(int64_t regionID, status.completedResourceSize += completedResourceSize; status.completedTileCount += completedTileCount; status.completedTileSize += completedTileSize; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "write region resources"); +} catch (...) { + handleError("write region resources"); } uint64_t OfflineDatabase::putRegionResourceInternal(int64_t regionID, const Resource& resource, const Response& response) { @@ -1070,13 +1081,8 @@ expected OfflineDatabase::getRegion query.run(); return decodeOfflineRegionDefinition(query.get(0)); -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "load region"); - return unexpected(std::current_exception()); -} catch (const std::runtime_error& ex) { - // Catch errors from malformed offline region definitions - // and skip them (as above). - handleError(ex, "load region"); +} catch (...) { + handleError("load region"); return unexpected(std::current_exception()); } @@ -1092,8 +1098,8 @@ expected OfflineDatabase::getRegionComp result.completedResourceSize += result.completedTileSize; return result; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "get region status"); +} catch (...) { + handleError("get region status"); return unexpected(std::current_exception()); } @@ -1232,10 +1238,9 @@ std::exception_ptr OfflineDatabase::setMaximumAmbientCacheSize(uint64_t size) { } return nullptr; - } catch (const mapbox::sqlite::Exception& ex) { + } catch (...) { maximumAmbientCacheSize = previousMaximumAmbientCacheSize; - handleError(ex, "set maximum ambient cache size"); - + handleError("set maximum ambient cache size"); return std::current_exception(); } } @@ -1274,8 +1279,8 @@ uint64_t OfflineDatabase::getOfflineMapboxTileCount() try { offlineMapboxTileCount = query.get(0); return *offlineMapboxTileCount; -} catch (const mapbox::sqlite::Exception& ex) { - handleError(ex, "get offline Mapbox tile count"); +} catch (...) { + handleError("get offline Mapbox tile count"); return std::numeric_limits::max(); } @@ -1294,8 +1299,8 @@ void OfflineDatabase::markUsedResources(int64_t regionID, const std::list