[core] Fix offline region download freezing

CHANGELOG.md

@@ -2,6 +2,16 @@
 
 ## master
 
+### 🐞 Bug fixes
+
+- [core] Fix offline region download freezing ([#16230](https://github.com/mapbox/mapbox-gl-native/pull/16230))
+
+  Downloaded resources are put in the buffer and inserted in the database in batches.
+
+  Before this change, the buffer was flushed only at the network response callback and thus it never got flushed if the last required resources were present locally and did not initiate network requests - it caused freezing.
+
+  Now the buffer is flushed every time the remaining resources container gets empty.
+
 ## maps-v1.2.0 (2020.02-release-vanillashake)
 
 ### ✨ New features

include/mbgl/storage/database_file_source.hpp

@@ -7,12 +7,6 @@
 
 namespace mbgl {
 
-// Properties that may be supported by database file sources.
-
-// Property to set database mode. When set, database opens in read-only mode; database opens in read-write-create mode
-// otherwise. type: bool
-constexpr const char* READ_ONLY_MODE_KEY = "read-only-mode";
-
 class ResourceOptions;
 
 // TODO: Split DatabaseFileSource into Ambient cache and Database interfaces.

include/mbgl/storage/file_source.hpp

@@ -84,4 +84,24 @@ class FileSource {
     FileSource() = default;
 };
 
+// Properties that may be supported by online file sources:
+
+// Property name to set / get an access token.
+// type: std::string
+constexpr const char* ACCESS_TOKEN_KEY = "access-token";
+
+// Property name to set / get base url.
+// type: std::string
+constexpr const char* API_BASE_URL_KEY = "api-base-url";
+
+// Property name to set / get maximum number of concurrent requests.
+// type: unsigned
+constexpr const char* MAX_CONCURRENT_REQUESTS_KEY = "max-concurrent-requests";
+
+// Properties that may be supported by database file sources:
+
+// Property to set database mode. When set, database opens in read-only mode; database opens in read-write-create mode
+// otherwise. type: bool
+constexpr const char* READ_ONLY_MODE_KEY = "read-only-mode";
+
 } // namespace mbgl

include/mbgl/storage/online_file_source.hpp

@@ -7,20 +7,6 @@ namespace mbgl {
 
 class ResourceTransform;
 
-// Properties that may be supported by online file sources.
-
-// Property name to set / get an access token.
-// type: std::string
-constexpr const char* ACCESS_TOKEN_KEY = "access-token";
-
-// Property name to set / get base url.
-// type: std::string
-constexpr const char* API_BASE_URL_KEY = "api-base-url";
-
-// Property name to set / get maximum number of concurrent requests.
-// type: unsigned
-constexpr const char* MAX_CONCURRENT_REQUESTS_KEY = "max-concurrent-requests";
-
 class OnlineFileSource : public FileSource {
 public:
     OnlineFileSource();

platform/default/include/mbgl/storage/offline_download.hpp

@@ -40,6 +40,7 @@ class OfflineDownload {
     void activateDownload();
     void continueDownload();
     void deactivateDownload();
+    bool flushResourcesBuffer();
 
     /*
      * Ensure that the resource is stored in the database, requesting it if necessary.

platform/default/src/mbgl/storage/offline_download.cpp

@@ -360,16 +360,20 @@ void OfflineDownload::activateDownload() {
    the first few errors is fruitless anyway.
 */
 void OfflineDownload::continueDownload() {
-    if (resourcesRemaining.empty() && status.complete()) {
-        markPendingUsedResources();
-        setState(OfflineRegionDownloadState::Inactive);
-        return;
+    if (resourcesRemaining.empty()) {
+        // Flush pending buffers.
+        if (!flushResourcesBuffer()) return;
+        if (status.complete()) {
+            markPendingUsedResources();
+            setState(OfflineRegionDownloadState::Inactive);
+            return;
+        }
     }
 
     if (resourcesToBeMarkedAsUsed.size() >= kMarkBatchSize) markPendingUsedResources();
 
     uint32_t maxConcurrentRequests = util::DEFAULT_MAXIMUM_CONCURRENT_REQUESTS;
-    auto value = onlineFileSource.getProperty("max-concurrent-requests");
+    auto value = onlineFileSource.getProperty(MAX_CONCURRENT_REQUESTS_KEY);
     if (uint64_t* maxRequests = value.getUint()) {
         maxConcurrentRequests = static_cast<uint32_t>(*maxRequests);
     }
@@ -387,6 +391,19 @@ void OfflineDownload::deactivateDownload() {
     buffer.clear();
 }
 
+bool OfflineDownload::flushResourcesBuffer() {
+    if (buffer.empty()) return true;
+    try {
+        offlineDatabase.putRegionResources(id, buffer, status);
+        buffer.clear();
+        observer->statusChanged(status);
+        return true;
+    } catch (const MapboxTileLimitExceededException&) {
+        onMapboxTileCountLimitExceeded();
+        return false;
+    }
+}
+
 void OfflineDownload::queueResource(Resource&& resource) {
     resource.setPriority(Resource::Priority::Low);
     resource.setUsage(Resource::Usage::Offline);
@@ -479,18 +496,10 @@ void OfflineDownload::ensureResource(Resource&& resource,
             // Queue up for batched insertion
             buffer.emplace_back(resource, onlineResponse);
 
-            // Flush buffer periodically
-            if (buffer.size() == kResourcesBatchSize || resourcesRemaining.empty()) {
-                try {
-                    offlineDatabase.putRegionResources(id, buffer, status);
-                } catch (const MapboxTileLimitExceededException&) {
-                    onMapboxTileCountLimitExceeded();
-                    return;
-                }
-
-                buffer.clear();
-                observer->statusChanged(status);
-            }
+            // Flush buffer periodically.
+            // Have to keep `resourcesRemaining.empty()` as the following condition would fail otherwise.
+            // TODO: Simplify the tile count limit check code path!
+            if ((buffer.size() == kResourcesBatchSize || resourcesRemaining.empty()) && !flushResourcesBuffer()) return;
 
             if (offlineDatabase.exceedsOfflineMapboxTileCountLimit(resource)) {
                 onMapboxTileCountLimitExceeded();

render-test/file_source.cpp

@@ -20,7 +20,7 @@ ProxyFileSource::ProxyFileSource(std::shared_ptr<FileSource> defaultResourceLoad
     assert(defaultResourceLoader);
     if (offline) {
         auto dbfs = FileSourceManager::get()->getFileSource(FileSourceType::Database, options);
-        dbfs->setProperty("read-only-mode", true);
+        dbfs->setProperty(READ_ONLY_MODE_KEY, true);
     }
 }
 

test/src/mbgl/test/stub_file_source.cpp

@@ -70,6 +70,15 @@ void StubFileSource::remove(AsyncRequest* req) {
     }
 }
 
+void StubFileSource::setProperty(const std::string& key, const mapbox::base::Value& value) {
+    properties[key] = value;
+}
+
+mapbox::base::Value StubFileSource::getProperty(const std::string& key) const {
+    auto it = properties.find(key);
+    return (it != properties.end()) ? it->second : mapbox::base::Value();
+}
+
 optional<Response> StubFileSource::defaultResponse(const Resource& resource) {
     switch (resource.kind) {
     case Resource::Kind::Style:

test/src/mbgl/test/stub_file_source.hpp

@@ -5,6 +5,7 @@
 #include <mbgl/storage/resource.hpp>
 #include <mbgl/util/timer.hpp>
 
+#include <map>
 #include <unordered_map>
 
 namespace mbgl {
@@ -22,6 +23,8 @@ class StubFileSource : public FileSource {
     std::unique_ptr<AsyncRequest> request(const Resource&, Callback) override;
     bool canRequest(const Resource&) const override { return true; }
     void remove(AsyncRequest*);
+    void setProperty(const std::string&, const mapbox::base::Value&) override;
+    mapbox::base::Value getProperty(const std::string&) const override;
 
     using ResponseFunction = std::function<optional<Response> (const Resource&)>;
 
@@ -48,6 +51,7 @@ class StubFileSource : public FileSource {
     std::unordered_map<AsyncRequest*, std::tuple<Resource, ResponseFunction, Callback>> pending;
     ResponseType type;
     util::Timer timer;
+    std::map<std::string, mapbox::base::Value> properties;
 };
 
 } // namespace mbgl

test/storage/offline_download.test.cpp

@@ -385,7 +385,7 @@ TEST(OfflineDownload, DoesNotFloodTheFileSourceWithRequests) {
     fileSource.respond(Resource::Kind::Style, test.response("style.json"));
     test.loop.runOnce();
 
-    EXPECT_EQ(*fileSource.getProperty("max-concurrent-requests").getUint(), fileSource.requests.size());
+    EXPECT_EQ(*fileSource.getProperty(MAX_CONCURRENT_REQUESTS_KEY).getUint(), fileSource.requests.size());
 }
 
 TEST(OfflineDownload, GetStatusNoResources) {
@@ -1026,3 +1026,39 @@ TEST(OfflineDownload, InterruptAndResume) {
     download.setState(OfflineRegionDownloadState::Active);
     test.loop.run();
 }
+
+TEST(OfflineDownload, NoFreezingOnCachedTilesAndNewStyle) {
+    OfflineTest test;
+    auto region = test.createRegion();
+    ASSERT_TRUE(region);
+    OfflineDownload download(region->getID(),
+                             OfflineTilePyramidRegionDefinition(
+                                 "http://127.0.0.1:3000/style.json", LatLngBounds::world(), 1.0, 1.0, 1.0, true),
+                             test.db,
+                             test.fileSource);
+
+    test.fileSource.setProperty(MAX_CONCURRENT_REQUESTS_KEY, 2u);
+    test.fileSource.styleResponse = [&](const Resource&) { return test.response("inline_source.style.json"); };
+    // Number of resources must exceed MAX_CONCURRENT_REQUESTS_KEY
+    test.db.put(Resource::tile("http://127.0.0.1:3000/{z}-{x}-{y}.vector.pbf", 1, 0, 0, 1, Tileset::Scheme::XYZ),
+                test.response("0-0-0.vector.pbf"));
+    test.db.put(Resource::tile("http://127.0.0.1:3000/{z}-{x}-{y}.vector.pbf", 1, 0, 1, 1, Tileset::Scheme::XYZ),
+                test.response("0-0-0.vector.pbf"));
+    test.db.put(Resource::tile("http://127.0.0.1:3000/{z}-{x}-{y}.vector.pbf", 1, 1, 0, 1, Tileset::Scheme::XYZ),
+                test.response("0-0-0.vector.pbf"));
+    test.db.put(Resource::tile("http://127.0.0.1:3000/{z}-{x}-{y}.vector.pbf", 1, 1, 1, 1, Tileset::Scheme::XYZ),
+                test.response("0-0-0.vector.pbf"));
+
+    auto observer = std::make_unique<MockObserver>();
+    observer->statusChangedFn = [&](OfflineRegionStatus status) {
+        if (status.complete()) {
+            test.loop.stop();
+        }
+    };
+
+    download.setObserver(std::move(observer));
+    download.setState(OfflineRegionDownloadState::Active);
+
+    test.loop.run();
+    // Passes if does not freeze.
+}

test/storage/online_file_source.test.cpp

@@ -449,7 +449,7 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequests)) {
     const std::size_t NUM_REQUESTS = 3;
 
     NetworkStatus::Set(NetworkStatus::Status::Offline);
-    fs.setProperty("max-concurrent-requests", 1u);
+    fs.setProperty(MAX_CONCURRENT_REQUESTS_KEY, 1u);
     // After DefaultFileSource was split, OnlineFileSource lives on a separate
     // thread. Pause OnlineFileSource, so that messages are queued for processing.
     fs.pause();
@@ -492,7 +492,7 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(LowHighPriorityRequestsMany)) {
     int correct_regular = 0;
 
     NetworkStatus::Set(NetworkStatus::Status::Offline);
-    fs.setProperty("max-concurrent-requests", 1u);
+    fs.setProperty(MAX_CONCURRENT_REQUESTS_KEY, 1u);
     fs.pause();
 
     std::vector<std::unique_ptr<AsyncRequest>> collector;
@@ -542,10 +542,10 @@ TEST(OnlineFileSource, TEST_REQUIRES_SERVER(MaximumConcurrentRequests)) {
     util::RunLoop loop;
     OnlineFileSource fs;
 
-    ASSERT_EQ(*fs.getProperty("max-concurrent-requests").getUint(), 20u);
+    ASSERT_EQ(*fs.getProperty(MAX_CONCURRENT_REQUESTS_KEY).getUint(), 20u);
 
-    fs.setProperty("max-concurrent-requests", 10u);
-    ASSERT_EQ(*fs.getProperty("max-concurrent-requests").getUint(), 10u);
+    fs.setProperty(MAX_CONCURRENT_REQUESTS_KEY, 10u);
+    ASSERT_EQ(*fs.getProperty(MAX_CONCURRENT_REQUESTS_KEY).getUint(), 10u);
 }
 
 TEST(OnlineFileSource, TEST_REQUIRES_SERVER(RequestSameUrlMultipleTimes)) {