[core] default file source - ensure thread safety for local file and asset requests

1 files changed, 46 insertions, 38 deletions

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<FileSource> assetFileSource_, const std::string& cachePath, uint64_t maximumCacheSize)
+            : assetFileSource(assetFileSource_)
+            , localFileSource(std::make_unique<LocalFileSource>())
+            , 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>(
-                    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>(
+                            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<OfflineDownload>(regionID, offlineDatabase.getRegionDefinition(regionID), offlineDatabase, onlineFileSource)).first->second;
     }
 
+    // shared so that destruction is done on the creating thread
+    const std::shared_ptr<FileSource> assetFileSource;
+    const std::unique_ptr<FileSource> localFileSource;
     OfflineDatabase offlineDatabase;
     OnlineFileSource onlineFileSource;
     std::unordered_map<AsyncRequest*, std::unique_ptr<AsyncRequest>> tasks;
@@ -173,10 +188,9 @@ DefaultFileSource::DefaultFileSource(const std::string& cachePath,
 DefaultFileSource::DefaultFileSource(const std::string& cachePath,
                                      std::unique_ptr<FileSource>&& assetFileSource_,
                                      uint64_t maximumCacheSize)
-    : thread(std::make_unique<util::Thread<Impl>>(util::ThreadContext{"DefaultFileSource", util::ThreadPriority::Low},
-            cachePath, maximumCacheSize)),
-      assetFileSource(std::move(assetFileSource_)),
-      localFileSource(std::make_unique<LocalFileSource>()) {
+        : assetFileSource(std::move(assetFileSource_))
+        , thread(std::make_unique<util::Thread<Impl>>(util::ThreadContext{"DefaultFileSource", util::ThreadPriority::Low},
+                                                      assetFileSource, cachePath, maximumCacheSize)) {
 }
 
 DefaultFileSource::~DefaultFileSource() = default;
@@ -228,13 +242,7 @@ std::unique_ptr<AsyncRequest> DefaultFileSource::request(const Resource& resourc
         std::unique_ptr<AsyncRequest> 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<DefaultFileRequest>(resource, callback, *thread);
-    }
+    return std::make_unique<DefaultFileRequest>(resource, callback, *thread);
 }
 
 void DefaultFileSource::listOfflineRegions(std::function<void (std::exception_ptr, optional<std::vector<OfflineRegion>>)> callback) {