Harden OfflineDatabase #12224

kkaefer · 2018-06-26T20:35:35Z

This changes OfflineDatabase to be more resilient. The design is to catch SQLite exceptions on the body of all public functions, and return with a failure code if the query failed.

Also adds tests for corrupt databases, as well as support for operating without read or write access to the file system. Also handles disk full error (in those cases, the database remains readable, but any updates, such as timestamp updates or resource insertions) will fail.

friedbunny

This looks like it has an inadvertent bump of the platform/ios/vendor/mapbox-events-ios submodule.

Please also add an iOS changelog entry. 🙇

jfirebaugh

We'll need to think about what the behavior should be when an offline download encounters a database error. For instance, if the disk is full, it looks like the current behavior may be indistinguishable from a successful download.

jfirebaugh · 2018-06-29T19:43:48Z

platform/default/default_file_source.cpp

-        } catch (...) {
+        if (auto download = getDownload(regionID)) {
+            callback({}, download->getStatus());
+        } else {
            callback(std::current_exception(), {});


std::current_exception() will always return an empty result here, since it's no longer inside a catch block.

This is still pending; it should synthesize an exception. Also, listRegions, createRegion, and updateMetadata above and deleteRegion below need a similar change.

jfirebaugh · 2018-06-29T19:44:46Z

platform/default/mbgl/storage/offline_database.cpp

@@ -13,7 +13,12 @@ namespace mbgl {
 OfflineDatabase::OfflineDatabase(std::string path_, uint64_t maximumCacheSize_)
    : path(std::move(path_)),
      maximumCacheSize(maximumCacheSize_) {
-    ensureSchema();
+      try {


Nit: whitespace.

jfirebaugh · 2018-06-29T19:49:35Z

platform/default/mbgl/storage/offline_database.hpp

    void migrateToVersion5();
-    void migrateToVersion6();
+    void migrateToVersion3();
+        void migrateToVersion6();


Whitespace.

jfirebaugh · 2018-06-29T19:51:03Z

test/storage/offline_download.test.cpp

        OfflineRegionDefinition definition { "", LatLngBounds::hull({1, 2}, {3, 4}), 5, 6, 1.0 };
        OfflineRegionMetadata metadata;
-        return db.createRegion(definition, metadata);
+        return  db.createRegion(definition, metadata);


Whitespace.

lilykaiser · 2018-07-30T14:25:51Z

Fixes #10031
(I believe)

kkaefer · 2018-08-01T13:45:13Z

We'll need to think about what the behavior should be when an offline download encounters a database error. For instance, if the disk is full, it looks like the current behavior may be indistinguishable from a successful download.

The current behavior is to crash if the disk is full (or any IO error occurs). When a network error occurs, the offline download never completes. This PR changes the crash when the disk is full to an offline download that never completes, which, while not ideal, is arguably better than the previous behavior.

I think erroneous downloads is something worth fixing, but I'd like to do that in a followup PR to avoid delaying the important crash fixes in this PR even longer. Overall, I think we need a concept for what an offline download should do in the error case: Retry a few times? Record failures and add them to the status? How are sparse tilesets handled (they return 404s on purpose, which means such a download will never complete atm)? What happens when you go offline mid-download, or try to do a download over a spotty connection?

jfirebaugh · 2018-08-01T21:41:11Z

Considering the call sequence OfflineDownload::ensureResource → OfflineDatabase::putRegionResources → putRegionResourceInternal → putInternal → putTile...

If putTile fails from a database error, the exception will pop the stack up to putRegionResources. The transaction will roll back, so none of the database changes will take effect, although changes to status.completedResourceCount, completedResourceSize, completedTileCount, and completedTileSize from prior elements of the batch will still be applied, so there may be an inconsistency between the database and status.

However, there will be at least one resource that does not trigger an increment of completedResourceCount, so OfflineRegionStatus::complete() will return false (unless the region is affected by #11930). So you're right, although the status may overcount completed resources, it will not be marked as complete. That does seem better than crashing. Can you add a test for this to offline_download.test.cpp? And it looks straightforward to correct the status fields by accumulating in temporary variables that are added to the status fields only after the transaction is committed.

When a network error occurs, the offline download never completes.

If it's a permanent error, that's true. But transient errors will get retried, since OfflineDownload::ensureResource keeps the request alive until it succeeds. I do think that it would be fair to handle database IO errors differently though -- e.g. immediately notify via an observer method, deactivate the download, and require developer intervention to reactivate. (Similarly to how exceeding the tile count limit is handled.)

How are sparse tilesets handled (they return 404s on purpose, which means such a download will never complete atm)?

404s get turned into "no content" non-errors by the file source. We might need to take this into account if we change that behavior though.

kkaefer · 2018-08-06T14:25:50Z

I fixed the test issues when using the VFS by porting the creation/deletion routines for sqlite3_file objects from https://www.sqlite.org/src/doc/trunk/src/test_vfstrace.c.

tobrun · 2018-08-13T18:39:12Z

@kkaefer took this PR for a testrun and it's looking great!

Testrun

Open activity with a mapview
Use adb shell followed by su rm -rf /data/data/com.mapbox.mapboxsdk.testapp/files
Scroll around map, close and reopen activity with a map
Application is not crashing

I'm seeing is that the files/mbgl-offline.db is only recreated with a fresh application start.

jfirebaugh · 2018-08-14T17:01:41Z

I'm seeing is that the files/mbgl-offline.db is only recreated with a fresh application start.

I think the expected behavior is that a fresh application start isn't required, so this may need more investigation.

kkaefer · 2018-08-14T20:06:10Z

Implemented database recreating when it is deleted in
410566f. Unfortunately, the behavior on macOS and Linux is different and we're not seeing the DB moved error code there.

…r feet

kkaefer · 2018-08-14T22:52:26Z

This is now ready for review 🎉

jfirebaugh

Awesome! Having an expected polyfill will definitely be useful.

friedbunny assigned kkaefer Jun 27, 2018

friedbunny added performance Speed, stability, CPU usage, memory usage, or power usage Core The cross-platform C++ core, aka mbgl labels Jun 27, 2018

friedbunny reviewed Jun 27, 2018

View reviewed changes

kkaefer force-pushed the filesource branch 2 times, most recently from 0fecaed to cd3880e Compare June 27, 2018 20:57

kkaefer requested review from brunoabinader and jfirebaugh June 27, 2018 20:59

kkaefer force-pushed the filesource branch from cd3880e to ee5cd16 Compare June 29, 2018 19:45

jfirebaugh suggested changes Jun 29, 2018

View reviewed changes

kkaefer mentioned this pull request Jul 4, 2018

[android] Mapbox Android Demo app native crash on a Pixel XL Android P (Beta) #12278

Closed

LukasPaczos mentioned this pull request Jul 9, 2018

Android crash log (Mapbox 5.5.2) #12195

Closed

kkaefer mentioned this pull request Jul 16, 2018

[Android] Offline region download crash #12390

Closed

LukasPaczos mentioned this pull request Jul 18, 2018

SIGSEV Fetching prefetched tiles #12401

Closed

lilykaiser mentioned this pull request Jul 30, 2018

Fatal Exception: mapbox::sqlite::Exception: database or disk is full #12507

Closed

lilykaiser added the high priority label Jul 31, 2018

kkaefer force-pushed the filesource branch 2 times, most recently from 528dd63 to e1b23db Compare August 1, 2018 13:13

kkaefer force-pushed the filesource branch 2 times, most recently from 02564b6 to 03a4dc2 Compare August 6, 2018 13:37

kkaefer force-pushed the filesource branch from 51ff691 to d81bafb Compare August 6, 2018 14:40

kkaefer force-pushed the filesource branch 4 times, most recently from 5fa7f67 to 0c90386 Compare August 14, 2018 02:04

kkaefer added 4 commits August 14, 2018 13:04

[build] vendor std::expected polyfill

83f0807

[core] harden OfflineDatabase

be9d77c

[test] add test for pending offline download when the disk is full

369b70e

WIP: use expected<T, E> for passing on errors

4d5a91f

kkaefer force-pushed the filesource branch from 0c90386 to 410566f Compare August 14, 2018 20:04

kkaefer force-pushed the filesource branch from 410566f to 651ee9a Compare August 14, 2018 20:28

[core] recreate offline database when it is deleted out from under ou…

67755d1

…r feet

kkaefer force-pushed the filesource branch 2 times, most recently from 0933103 to e783076 Compare August 14, 2018 21:32

[build] make sure we're also updating the mapbox-ios-events submodule

0f35e05

kkaefer force-pushed the filesource branch from e783076 to 0f35e05 Compare August 14, 2018 21:44

jfirebaugh approved these changes Aug 14, 2018

View reviewed changes

kkaefer merged commit 9d72bf8 into master Aug 15, 2018

kkaefer deleted the filesource branch August 15, 2018 00:03

friedbunny added the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 15, 2018

kkaefer mentioned this pull request Sep 6, 2018

Offline Tiles Downloading [Pause/Resume] #12831

Closed

friedbunny removed needs changelog Indicates PR needs a changelog entry prior to merging. labels Sep 20, 2018

This was referenced Sep 21, 2018

sqlite exception unable to open database file #6193

Closed

[iOS 12] crash on mapbox::sqlite::Query::run() on iOS 12 only #12950

Closed

zugaldia mentioned this pull request Nov 5, 2018

Fails to download offline map on Android (fatal signal 6 GC crash) #12848

Closed

asheemmamoowala mentioned this pull request Nov 8, 2018

Possibility to disable caching to DB (due to crashes after versions downgrade) #13273

Closed

zugaldia added the offline label Aug 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harden OfflineDatabase #12224

Harden OfflineDatabase #12224

kkaefer commented Jun 26, 2018

friedbunny left a comment

jfirebaugh left a comment

jfirebaugh Jun 29, 2018

jfirebaugh Aug 6, 2018

jfirebaugh Jun 29, 2018

jfirebaugh Jun 29, 2018

jfirebaugh Jun 29, 2018

lilykaiser commented Jul 30, 2018

kkaefer commented Aug 1, 2018

jfirebaugh commented Aug 1, 2018

kkaefer commented Aug 6, 2018

tobrun commented Aug 13, 2018

jfirebaugh commented Aug 14, 2018

kkaefer commented Aug 14, 2018

kkaefer commented Aug 14, 2018

jfirebaugh left a comment

Harden OfflineDatabase #12224

Harden OfflineDatabase #12224

Conversation

kkaefer commented Jun 26, 2018

friedbunny left a comment

Choose a reason for hiding this comment

jfirebaugh left a comment

Choose a reason for hiding this comment

jfirebaugh Jun 29, 2018

Choose a reason for hiding this comment

jfirebaugh Aug 6, 2018

Choose a reason for hiding this comment

jfirebaugh Jun 29, 2018

Choose a reason for hiding this comment

jfirebaugh Jun 29, 2018

Choose a reason for hiding this comment

jfirebaugh Jun 29, 2018

Choose a reason for hiding this comment

lilykaiser commented Jul 30, 2018

kkaefer commented Aug 1, 2018

jfirebaugh commented Aug 1, 2018

kkaefer commented Aug 6, 2018

tobrun commented Aug 13, 2018

Testrun

jfirebaugh commented Aug 14, 2018

kkaefer commented Aug 14, 2018

kkaefer commented Aug 14, 2018

jfirebaugh left a comment

Choose a reason for hiding this comment