Implement Cache-Control: must-revalidate support #9670
Conversation
| @@ -126,6 +127,14 @@ void OfflineDatabase::migrateToVersion5() { | |||
| db->exec("PRAGMA user_version = 5"); | |||
| } | |||
|
|
|||
| void OfflineDatabase::migrateToVersion6() { | |||
There was a problem hiding this comment.
I'm a bit worried about this migration in combination with #9666. Any idea how long it will take if the database contains large offline regions?
There was a problem hiding this comment.
Good point. Before I opted to use ALTER TABLE, I looked up the SQLite docs and they say this:
The execution time of the
ALTER TABLEcommand is independent of the amount of data in the table. TheALTER TABLEcommand runs as quickly on a table with 10 million rows as it does on a table with 1 row.
I also conducted a test with a database that was ~300 MB in size and contained 6000 tiles. The query execution time the SQLite3 command line showed was < 1 millisecond.
| // v5.db is a v5 database, migrated from v2, v3 & v4. | ||
|
|
||
| deleteFile("test/fixtures/offline_database/migrated.db"); | ||
| writeFile("test/fixtures/offline_database/migrated.db", util::read_file("test/fixtures/offline_database/v5.db")); |
There was a problem hiding this comment.
v5.db should be checked in. It's currently ignored in .gitignore. I like the rename of the output to migrated.db -- that means we can just migrated.db and not v*.db.
|
@jfirebaugh another thing I considered was to add a generic " |
kkaefer
force-pushed
the
cache-control-must-revalidate
branch
2 times, most recently
from
408b142 to
4c1fc5e
Compare
|
There's one remaining bug before this can be merged: When a file is cached, but expired, and has |
kkaefer
force-pushed
the
cache-control-must-revalidate
branch
from
4c1fc5e to
e34e5c4
Compare
|
Fixed the edge case described in the previous comment. |
kkaefer
removed
the
⚠️ DO NOT MERGE
|
@jfirebaugh Can you please re-review? |
In #9670, we implemented support for the Cache-Control: must-revalidate header. While we now respect this header and don't show resources that are stale and have this header set, the optional cache request never completes. This means we're also never going to try to actually get a fresh tile and just never show this tile anymore. This commit fixes this by responding with a Not Found error when the resource is unusable (= stale and has must-revalidate set). Since we actually still have the data (but can't use it due to caching rules), we're responding with the data as well. To avoid a second cache hit, tile_loader_impl.hpp now passes on the data from the Optional to the Required request so that it can be reused when we get a 304 Not Modified response from the server.
In #9670, we implemented support for the Cache-Control: must-revalidate header. While we now respect this header and don't show resources that are stale and have this header set, the optional cache request never completes. This means we're also never going to try to actually get a fresh tile and just never show this tile anymore. This commit fixes this by responding with a Not Found error when the resource is unusable (= stale and has must-revalidate set). Since we actually still have the data (but can't use it due to caching rules), we're responding with the data as well. To avoid a second cache hit, tile_loader_impl.hpp now passes on the data from the Optional to the Required request so that it can be reused when we get a 304 Not Modified response from the server.
|
Is there any possibility that these changes will cause startup to fail on iOS if cache.db (DefaultFileSource) is not found? I am seeing an issue where my app crashes at startup with an unknown error when trying to read cach.db (DefaultFileSource), if cache.db does not exist. |
|
@tekbob27 if you are seeing an issue with Mapbox GL, please open a new ticket on this issue tracker and make sure to describe the version of Mapbox GL, platform etc. you're using. |
We already had a half-finished implementation, in particular, we already parsed the header and had (incorrect) tests for it.
This PR adds support for indicating that revalidation is required to the database scheme (updating it to v6).
Fixes #9103