From 89129379f18b913a7c2b7a7ff99644e277e9d3ff Mon Sep 17 00:00:00 2001 From: Ivo van Dongen Date: Fri, 26 May 2017 17:49:33 +0300 Subject: [core] default file source - ensure thread safety for local file and asset requests --- include/mbgl/storage/default_file_source.hpp | 4 +- platform/default/default_file_source.cpp | 84 +++++++++++++++------------- 2 files changed, 48 insertions(+), 40 deletions(-) diff --git a/include/mbgl/storage/default_file_source.hpp b/include/mbgl/storage/default_file_source.hpp index f612a01aac..1e60d2bc5a 100644 --- a/include/mbgl/storage/default_file_source.hpp +++ b/include/mbgl/storage/default_file_source.hpp @@ -140,9 +140,9 @@ public: class Impl; private: + // Shared so destruction is done on this thread + const std::shared_ptr assetFileSource; const std::unique_ptr> thread; - const std::unique_ptr assetFileSource; - const std::unique_ptr localFileSource; std::string cachedBaseURL = mbgl::util::API_BASE_URL; std::string cachedAccessToken; }; diff --git a/platform/default/default_file_source.cpp b/platform/default/default_file_source.cpp index 7db0b85461..142be5228f 100644 --- a/platform/default/default_file_source.cpp +++ b/platform/default/default_file_source.cpp @@ -26,8 +26,10 @@ namespace mbgl { class DefaultFileSource::Impl { public: - Impl(const std::string& cachePath, uint64_t maximumCacheSize) - : offlineDatabase(cachePath, maximumCacheSize) { + Impl(std::shared_ptr assetFileSource_, const std::string& cachePath, uint64_t maximumCacheSize) + : assetFileSource(assetFileSource_) + , localFileSource(std::make_unique()) + , offlineDatabase(cachePath, maximumCacheSize) { } void setAPIBaseURL(const std::string& url) { @@ -105,35 +107,45 @@ public: } void request(AsyncRequest* req, Resource resource, Callback callback) { - Resource revalidation = resource; - - const bool hasPrior = resource.priorEtag || resource.priorModified || resource.priorExpires; - if (!hasPrior || resource.necessity == Resource::Optional) { - auto offlineResponse = offlineDatabase.get(resource); - - if (resource.necessity == Resource::Optional && !offlineResponse) { - // Ensure there's always a response that we can send, so the caller knows that - // there's no optional data available in the cache. - offlineResponse.emplace(); - offlineResponse->noContent = true; - offlineResponse->error = std::make_unique( - Response::Error::Reason::NotFound, "Not found in offline database"); + if (isAssetURL(resource.url)) { + //Asset request + tasks[req] = assetFileSource->request(resource, callback); + } else if (LocalFileSource::acceptsURL(resource.url)) { + //Local file request + tasks[req] = localFileSource->request(resource, callback); + } else { + // Try the offline database + Resource revalidation = resource; + + const bool hasPrior = resource.priorEtag || resource.priorModified || resource.priorExpires; + if (!hasPrior || resource.necessity == Resource::Optional) { + auto offlineResponse = offlineDatabase.get(resource); + + if (resource.necessity == Resource::Optional && !offlineResponse) { + // Ensure there's always a response that we can send, so the caller knows that + // there's no optional data available in the cache. + offlineResponse.emplace(); + offlineResponse->noContent = true; + offlineResponse->error = std::make_unique( + Response::Error::Reason::NotFound, "Not found in offline database"); + } + + if (offlineResponse) { + revalidation.priorModified = offlineResponse->modified; + revalidation.priorExpires = offlineResponse->expires; + revalidation.priorEtag = offlineResponse->etag; + callback(*offlineResponse); + } } - if (offlineResponse) { - revalidation.priorModified = offlineResponse->modified; - revalidation.priorExpires = offlineResponse->expires; - revalidation.priorEtag = offlineResponse->etag; - callback(*offlineResponse); + // Get from the online file source + if (resource.necessity == Resource::Required) { + tasks[req] = onlineFileSource.request(revalidation, [=] (Response onlineResponse) { + this->offlineDatabase.put(revalidation, onlineResponse); + callback(onlineResponse); + }); } } - - if (resource.necessity == Resource::Required) { - tasks[req] = onlineFileSource.request(revalidation, [=] (Response onlineResponse) { - this->offlineDatabase.put(revalidation, onlineResponse); - callback(onlineResponse); - }); - } } void cancel(AsyncRequest* req) { @@ -158,6 +170,9 @@ private: std::make_unique(regionID, offlineDatabase.getRegionDefinition(regionID), offlineDatabase, onlineFileSource)).first->second; } + // shared so that destruction is done on the creating thread + const std::shared_ptr assetFileSource; + const std::unique_ptr localFileSource; OfflineDatabase offlineDatabase; OnlineFileSource onlineFileSource; std::unordered_map> tasks; @@ -173,10 +188,9 @@ DefaultFileSource::DefaultFileSource(const std::string& cachePath, DefaultFileSource::DefaultFileSource(const std::string& cachePath, std::unique_ptr&& assetFileSource_, uint64_t maximumCacheSize) - : thread(std::make_unique>(util::ThreadContext{"DefaultFileSource", util::ThreadPriority::Low}, - cachePath, maximumCacheSize)), - assetFileSource(std::move(assetFileSource_)), - localFileSource(std::make_unique()) { + : assetFileSource(std::move(assetFileSource_)) + , thread(std::make_unique>(util::ThreadContext{"DefaultFileSource", util::ThreadPriority::Low}, + assetFileSource, cachePath, maximumCacheSize)) { } DefaultFileSource::~DefaultFileSource() = default; @@ -228,13 +242,7 @@ std::unique_ptr DefaultFileSource::request(const Resource& resourc std::unique_ptr workRequest; }; - if (isAssetURL(resource.url)) { - return assetFileSource->request(resource, callback); - } else if (LocalFileSource::acceptsURL(resource.url)) { - return localFileSource->request(resource, callback); - } else { - return std::make_unique(resource, callback, *thread); - } + return std::make_unique(resource, callback, *thread); } void DefaultFileSource::listOfflineRegions(std::function>)> callback) { -- cgit v1.2.1