[core] make forcing cache/network only more explicit

Previously, we used the existence of a `prior*` field in the Resource object as an indication for whether we should consult the cache or not. However, this is prone to error, since a failed cache lookup won't set any prior fields. Therefore, we manually set `priorExpires` to 0. This in turn triggered another bug where generated wrong expiration timestamps when the server response we got was expired (or expired between sending and receiving).

This commit changes the flags so that we can now explicitly request CacheOnly/NetworkOnly (or All) loading methods, rather than the implicit Optional/Required naming scheme.

1 files changed, 32 insertions, 26 deletions

diff --git a/src/mbgl/tile/tile_loader_impl.hpp b/src/mbgl/tile/tile_loader_impl.hpp
index 598ec32c10..1b29638269 100644
--- a/src/mbgl/tile/tile_loader_impl.hpp
+++ b/src/mbgl/tile/tile_loader_impl.hpp
@@ -15,32 +15,31 @@ TileLoader<T>::TileLoader(T& tile_,
                           const TileParameters& parameters,
                           const Tileset& tileset)
     : tile(tile_),
-      necessity(Necessity::Optional),
+      necessity(TileNecessity::Optional),
       resource(Resource::tile(
         tileset.tiles.at(0),
         parameters.pixelRatio,
         id.canonical.x,
         id.canonical.y,
         id.canonical.z,
-        tileset.scheme)),
+        tileset.scheme,
+        Resource::LoadingMethod::CacheOnly)),
       fileSource(parameters.fileSource) {
     assert(!request);
-    if (fileSource.supportsOptionalRequests()) {
+    if (fileSource.supportsCacheOnlyRequests()) {
         // When supported, the first request is always optional, even if the TileLoader
         // is marked as required. That way, we can let the first optional request continue
         // to load when the TileLoader is later changed from required to optional. If we
         // started out with a required request, we'd have to cancel everything, including the
         // initial optional part of the request.
-        loadOptional();
+        loadFromCache();
+    } else if (necessity == TileNecessity::Required) {
+        // When the file source doesn't support cache-only requests, and we definiitely need this
+        // data, we can start out with a network request immediately.
+        loadFromNetwork();
     } else {
-        // When the FileSource doesn't support optional requests, we do nothing until the
+        // When the FileSource doesn't support cache-only requests, we do nothing until the
         // data is definitely required.
-        if (necessity == Necessity::Required) {
-            loadRequired();
-        } else {
-            // We're using this field to check whether the pending request is optional or required.
-            resource.necessity = Resource::Optional;
-        }
     }
 }
 
@@ -48,29 +47,31 @@ template <typename T>
 TileLoader<T>::~TileLoader() = default;
 
 template <typename T>
-void TileLoader<T>::loadOptional() {
+void TileLoader<T>::loadFromCache() {
     assert(!request);
 
-    resource.necessity = Resource::Optional;
+    resource.loadingMethod = Resource::LoadingMethod::CacheOnly;
     request = fileSource.request(resource, [this](Response res) {
         request.reset();
 
-        tile.setTriedOptional();
+        tile.setTriedCache();
 
         if (res.error && res.error->reason == Response::Error::Reason::NotFound) {
-            // When the optional request could not be satisfied, don't treat it as an error.
-            // Instead, we make sure that the next request knows that there has been an optional
-            // request before by setting one of the prior* fields.
+            // When the cache-only request could not be satisfied, don't treat it as an error.
+            // A cache lookup could still return data, _and_ an error, in particular when we were
+            // able to find the data, but it is expired and the Cache-Control headers indicated that
+            // we aren't allowed to use expired responses. In this case, we still get the data which
+            // we can use in our conditional network request.
             resource.priorModified = res.modified;
-            resource.priorExpires = Timestamp{ Seconds::zero() };
+            resource.priorExpires = res.expires;
             resource.priorEtag = res.etag;
             resource.priorData = res.data;
         } else {
             loadedData(res);
         }
 
-        if (necessity == Necessity::Required) {
-            loadRequired();
+        if (necessity == TileNecessity::Required) {
+            loadFromNetwork();
         }
     });
 }
@@ -78,14 +79,15 @@ void TileLoader<T>::loadOptional() {
 template <typename T>
 void TileLoader<T>::makeRequired() {
     if (!request) {
-        loadRequired();
+        loadFromNetwork();
     }
 }
 
 template <typename T>
 void TileLoader<T>::makeOptional() {
-    if (resource.necessity == Resource::Required && request) {
-        // Abort a potential HTTP request.
+    if (resource.loadingMethod == Resource::LoadingMethod::NetworkOnly && request) {
+        // Abort the current request, but only when we know that we're specifically querying for a
+        // network resource only.
         request.reset();
     }
 }
@@ -98,19 +100,23 @@ void TileLoader<T>::loadedData(const Response& res) {
         resource.priorExpires = res.expires;
         // Do not notify the tile; when we get this message, it already has the current
         // version of the data.
+        tile.setMetadata(res.modified, res.expires);
     } else {
         resource.priorModified = res.modified;
         resource.priorExpires = res.expires;
         resource.priorEtag = res.etag;
-        tile.setData(res.noContent ? nullptr : res.data, res.modified, res.expires);
+        tile.setMetadata(res.modified, res.expires);
+        tile.setData(res.noContent ? nullptr : res.data);
     }
 }
 
 template <typename T>
-void TileLoader<T>::loadRequired() {
+void TileLoader<T>::loadFromNetwork() {
     assert(!request);
 
-    resource.necessity = Resource::Required;
+    // Instead of using Resource::LoadingMethod::All, we're first doing a CacheOnly, and then a
+    // NetworkOnly request.
+    resource.loadingMethod = Resource::LoadingMethod::NetworkOnly;
     request = fileSource.request(resource, [this](Response res) { loadedData(res); });
 }