[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.

16 files changed, 77 insertions, 83 deletions

diff --git a/src/mbgl/algorithm/update_renderables.hpp b/src/mbgl/algorithm/update_renderables.hpp
index 0c2266ff47..c583b6b2b6 100644
--- a/src/mbgl/algorithm/update_renderables.hpp
+++ b/src/mbgl/algorithm/update_renderables.hpp
@@ -1,8 +1,8 @@
 #pragma once
 
 #include <mbgl/tile/tile_id.hpp>
+#include <mbgl/tile/tile_necessity.hpp>
 #include <mbgl/util/range.hpp>
-#include <mbgl/storage/resource.hpp>
 
 #include <unordered_set>
 
@@ -40,15 +40,15 @@ void updateRenderables(GetTileFn getTile,
 
         // if (source has the tile and bucket is loaded) {
         if (tile->isRenderable()) {
-            retainTile(*tile, Resource::Necessity::Required);
+            retainTile(*tile, TileNecessity::Required);
             renderTile(idealRenderTileID, *tile);
         } else {
             // We are now attempting to load child and parent tiles.
-            bool parentHasTriedOptional = tile->hasTriedOptional();
+            bool parentHasTriedOptional = tile->hasTriedCache();
             bool parentIsLoaded = tile->isLoaded();
 
             // The tile isn't loaded yet, but retain it anyway because it's an ideal tile.
-            retainTile(*tile, Resource::Necessity::Required);
+            retainTile(*tile, TileNecessity::Required);
             covered = true;
             overscaledZ = dataTileZoom + 1;
             if (overscaledZ > zoomRange.max) {
@@ -56,7 +56,7 @@ void updateRenderables(GetTileFn getTile,
                 const auto childDataTileID = idealDataTileID.scaledTo(overscaledZ);
                 tile = getTile(childDataTileID);
                 if (tile && tile->isRenderable()) {
-                    retainTile(*tile, Resource::Necessity::Optional);
+                    retainTile(*tile, TileNecessity::Optional);
                     renderTile(idealRenderTileID, *tile);
                 } else {
                     covered = false;
@@ -67,7 +67,7 @@ void updateRenderables(GetTileFn getTile,
                     const OverscaledTileID childDataTileID(overscaledZ, idealRenderTileID.wrap, childTileID);
                     tile = getTile(childDataTileID);
                     if (tile && tile->isRenderable()) {
-                        retainTile(*tile, Resource::Necessity::Optional);
+                        retainTile(*tile, TileNecessity::Optional);
                         renderTile(childDataTileID.toUnwrapped(), *tile);
                     } else {
                         // At least one child tile doesn't exist, so we are going to look for
@@ -97,12 +97,19 @@ void updateRenderables(GetTileFn getTile,
                     }
 
                     if (tile) {
-                        retainTile(*tile, parentIsLoaded ? Resource::Necessity::Required
-                                                         : Resource::Necessity::Optional);
+                        if (!parentIsLoaded) {
+                            // We haven't completed loading the child, so we only do an optional
+                            // (cache) request in an attempt to quickly load data that we can show.
+                            retainTile(*tile, TileNecessity::Optional);
+                        } else {
+                            // Now that we've checked the child and know for sure that we can't load
+                            // it, we attempt to load the parent from the network.
+                            retainTile(*tile, TileNecessity::Required);
+                        }
 
                         // Save the current values, since they're the parent of the next iteration
                         // of the parent tile ascent loop.
-                        parentHasTriedOptional = tile->hasTriedOptional();
+                        parentHasTriedOptional = tile->hasTriedCache();
                         parentIsLoaded = tile->isLoaded();
 
                         if (tile->isRenderable()) {
diff --git a/src/mbgl/annotation/annotation_tile.cpp b/src/mbgl/annotation/annotation_tile.cpp
index 0596d60f4f..d405418a45 100644
--- a/src/mbgl/annotation/annotation_tile.cpp
+++ b/src/mbgl/annotation/annotation_tile.cpp
@@ -19,9 +19,6 @@ AnnotationTile::~AnnotationTile() {
     annotationManager.removeTile(*this);
 }
 
-void AnnotationTile::setNecessity(Necessity) {
-}
-
 class AnnotationTileFeatureData {
 public:
     AnnotationTileFeatureData(const AnnotationID id_,
diff --git a/src/mbgl/annotation/annotation_tile.hpp b/src/mbgl/annotation/annotation_tile.hpp
index 88505c50e3..a4d1e66802 100644
--- a/src/mbgl/annotation/annotation_tile.hpp
+++ b/src/mbgl/annotation/annotation_tile.hpp
@@ -14,8 +14,6 @@ public:
     AnnotationTile(const OverscaledTileID&, const TileParameters&);
     ~AnnotationTile() override;
 
-    void setNecessity(Necessity) final;
-
 private:
     AnnotationManager& annotationManager;
 };
diff --git a/src/mbgl/renderer/tile_pyramid.cpp b/src/mbgl/renderer/tile_pyramid.cpp
index 6cd9bd9ebd..3e2311089d 100644
--- a/src/mbgl/renderer/tile_pyramid.cpp
+++ b/src/mbgl/renderer/tile_pyramid.cpp
@@ -121,7 +121,7 @@ void TilePyramid::update(const std::vector<Immutable<style::Layer::Impl>>& layer
     // we're actively using, e.g. as a replacement for tile that aren't loaded yet.
     std::set<OverscaledTileID> retain;
 
-    auto retainTileFn = [&](Tile& tile, Resource::Necessity necessity) -> void {
+    auto retainTileFn = [&](Tile& tile, TileNecessity necessity) -> void {
         if (retain.emplace(tile.id).second) {
             tile.setNecessity(necessity);
         }
@@ -179,7 +179,7 @@ void TilePyramid::update(const std::vector<Immutable<style::Layer::Impl>>& layer
         while (tilesIt != tiles.end()) {
             if (retainIt == retain.end() || tilesIt->first < *retainIt) {
                 if (!needsRelayout) {
-                    tilesIt->second->setNecessity(Tile::Necessity::Optional);
+                    tilesIt->second->setNecessity(TileNecessity::Optional);
                     cache.add(tilesIt->first, std::move(tilesIt->second));
                 }
                 tiles.erase(tilesIt++);
diff --git a/src/mbgl/storage/resource.cpp b/src/mbgl/storage/resource.cpp
index 94bba7f8bf..e75302b986 100644
--- a/src/mbgl/storage/resource.cpp
+++ b/src/mbgl/storage/resource.cpp
@@ -95,7 +95,7 @@ Resource Resource::tile(const std::string& urlTemplate,
                         int32_t y,
                         int8_t z,
                         Tileset::Scheme scheme,
-                        Necessity necessity) {
+                        LoadingMethod loadingMethod) {
     bool supportsRatio = urlTemplate.find("{ratio}") != std::string::npos;
     if (scheme == Tileset::Scheme::TMS) {
         y = (1 << z) - y - 1;
@@ -131,7 +131,7 @@ Resource Resource::tile(const std::string& urlTemplate,
             y,
             z
         },
-        necessity
+        loadingMethod
     };
 }
 
diff --git a/src/mbgl/style/sources/image_source.cpp b/src/mbgl/style/sources/image_source.cpp
index 9b60ba1a48..fa268da0ef 100644
--- a/src/mbgl/style/sources/image_source.cpp
+++ b/src/mbgl/style/sources/image_source.cpp
@@ -59,7 +59,7 @@ void ImageSource::loadDescription(FileSource& fileSource) {
     if (req ||  loaded) {
         return;
     }
-    const Resource imageResource { Resource::Image, *url, {}, Resource::Necessity::Required };
+    const Resource imageResource { Resource::Image, *url, {} };
 
     req = fileSource.request(imageResource, [this](Response res) {
         if (res.error) {
diff --git a/src/mbgl/tile/geojson_tile.cpp b/src/mbgl/tile/geojson_tile.cpp
index ee4989462c..d648d2e5ff 100644
--- a/src/mbgl/tile/geojson_tile.cpp
+++ b/src/mbgl/tile/geojson_tile.cpp
@@ -17,8 +17,6 @@ GeoJSONTile::GeoJSONTile(const OverscaledTileID& overscaledTileID,
 void GeoJSONTile::updateData(mapbox::geometry::feature_collection<int16_t> features) {
     setData(std::make_unique<GeoJSONTileData>(std::move(features)));
 }
-
-void GeoJSONTile::setNecessity(Necessity) {}
     
 void GeoJSONTile::querySourceFeatures(
     std::vector<Feature>& result,
diff --git a/src/mbgl/tile/geojson_tile.hpp b/src/mbgl/tile/geojson_tile.hpp
index d8a0a379d7..270406267c 100644
--- a/src/mbgl/tile/geojson_tile.hpp
+++ b/src/mbgl/tile/geojson_tile.hpp
@@ -15,8 +15,6 @@ public:
                 mapbox::geometry::feature_collection<int16_t>);
 
     void updateData(mapbox::geometry::feature_collection<int16_t>);
-
-    void setNecessity(Necessity) final;
     
     void querySourceFeatures(
         std::vector<Feature>& result,
diff --git a/src/mbgl/tile/raster_tile.cpp b/src/mbgl/tile/raster_tile.cpp
index 2a3c9eeb0e..85fcea77b7 100644
--- a/src/mbgl/tile/raster_tile.cpp
+++ b/src/mbgl/tile/raster_tile.cpp
@@ -32,12 +32,12 @@ void RasterTile::setError(std::exception_ptr err) {
     observer->onTileError(*this, err);
 }
 
-void RasterTile::setData(std::shared_ptr<const std::string> data,
-                             optional<Timestamp> modified_,
-                             optional<Timestamp> expires_) {
+void RasterTile::setMetadata(optional<Timestamp> modified_, optional<Timestamp> expires_) {
     modified = modified_;
     expires = expires_;
+}
 
+void RasterTile::setData(std::shared_ptr<const std::string> data) {
     pending = true;
     ++correlationID;
     worker.invoke(&RasterTileWorker::parse, data, correlationID);
@@ -77,7 +77,7 @@ void RasterTile::setMask(TileMask&& mask) {
     }
 }
 
-void RasterTile::setNecessity(Necessity necessity) {
+void RasterTile::setNecessity(TileNecessity necessity) {
     loader.setNecessity(necessity);
 }
 
diff --git a/src/mbgl/tile/raster_tile.hpp b/src/mbgl/tile/raster_tile.hpp
index 2cb64e8ed7..192769ed8f 100644
--- a/src/mbgl/tile/raster_tile.hpp
+++ b/src/mbgl/tile/raster_tile.hpp
@@ -22,12 +22,11 @@ public:
                    const Tileset&);
     ~RasterTile() final;
 
-    void setNecessity(Necessity) final;
+    void setNecessity(TileNecessity) final;
 
     void setError(std::exception_ptr);
-    void setData(std::shared_ptr<const std::string> data,
-                 optional<Timestamp> modified_,
-                 optional<Timestamp> expires_);
+    void setMetadata(optional<Timestamp> modified, optional<Timestamp> expires);
+    void setData(std::shared_ptr<const std::string> data);
 
     void cancel() override;
 
diff --git a/src/mbgl/tile/tile.cpp b/src/mbgl/tile/tile.cpp
index 7d7eb0b3fc..f36a472e72 100644
--- a/src/mbgl/tile/tile.cpp
+++ b/src/mbgl/tile/tile.cpp
@@ -18,7 +18,7 @@ void Tile::setObserver(TileObserver* observer_) {
     observer = observer_;
 }
 
-void Tile::setTriedOptional() {
+void Tile::setTriedCache() {
     triedOptional = true;
     observer->onTileChanged(*this);
 }
diff --git a/src/mbgl/tile/tile.hpp b/src/mbgl/tile/tile.hpp
index 39cc0de8bd..8be7c4d862 100644
--- a/src/mbgl/tile/tile.hpp
+++ b/src/mbgl/tile/tile.hpp
@@ -6,6 +6,7 @@
 #include <mbgl/util/feature.hpp>
 #include <mbgl/util/tile_coordinate.hpp>
 #include <mbgl/tile/tile_id.hpp>
+#include <mbgl/tile/tile_necessity.hpp>
 #include <mbgl/renderer/tile_mask.hpp>
 #include <mbgl/renderer/bucket.hpp>
 #include <mbgl/tile/geometry_tile_data.hpp>
@@ -38,14 +39,7 @@ public:
 
     void setObserver(TileObserver* observer);
 
-    // Tiles can have two states: optional or required.
-    // - optional means that only low-cost actions should be taken to obtain the data
-    //   (e.g. load from cache, but accept stale data)
-    // - required means that every effort should be taken to obtain the data (e.g. load
-    //   from internet and keep the data fresh if it expires)
-    using Necessity = Resource::Necessity;
-
-    virtual void setNecessity(Necessity) = 0;
+    virtual void setNecessity(TileNecessity) {}
 
     // Mark this tile as no longer needed and cancel any pending work.
     virtual void cancel() = 0;
@@ -68,11 +62,11 @@ public:
             std::vector<Feature>& result,
             const SourceQueryOptions&);
 
-    void setTriedOptional();
+    void setTriedCache();
 
     // Returns true when the tile source has received a first response, regardless of whether a load
     // error occurred or actual data was loaded.
-    bool hasTriedOptional() const {
+    bool hasTriedCache() const {
         return triedOptional;
     }
 
diff --git a/src/mbgl/tile/tile_loader.hpp b/src/mbgl/tile/tile_loader.hpp
index bc408ebaf6..92ca74330f 100644
--- a/src/mbgl/tile/tile_loader.hpp
+++ b/src/mbgl/tile/tile_loader.hpp
@@ -21,12 +21,10 @@ public:
                const Tileset&);
     ~TileLoader();
 
-    using Necessity = Resource::Necessity;
-
-    void setNecessity(Necessity newNecessity) {
+    void setNecessity(TileNecessity newNecessity) {
         if (newNecessity != necessity) {
             necessity = newNecessity;
-            if (necessity == Necessity::Required) {
+            if (necessity == TileNecessity::Required) {
                 makeRequired();
             } else {
                 makeOptional();
@@ -45,12 +43,12 @@ private:
     // an up-to-date version or load new data
     void makeOptional();
 
-    void loadOptional();
+    void loadFromCache();
     void loadedData(const Response&);
-    void loadRequired();
+    void loadFromNetwork();
 
     T& tile;
-    Necessity necessity;
+    TileNecessity necessity;
     Resource resource;
     FileSource& fileSource;
     std::unique_ptr<AsyncRequest> request;
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); });
 }
 
diff --git a/src/mbgl/tile/vector_tile.cpp b/src/mbgl/tile/vector_tile.cpp
index e2e700b7b7..0756d3e526 100644
--- a/src/mbgl/tile/vector_tile.cpp
+++ b/src/mbgl/tile/vector_tile.cpp
@@ -12,16 +12,16 @@ VectorTile::VectorTile(const OverscaledTileID& id_,
     : GeometryTile(id_, sourceID_, parameters), loader(*this, id_, parameters, tileset) {
 }
 
-void VectorTile::setNecessity(Necessity necessity) {
+void VectorTile::setNecessity(TileNecessity necessity) {
     loader.setNecessity(necessity);
 }
 
-void VectorTile::setData(std::shared_ptr<const std::string> data_,
-                         optional<Timestamp> modified_,
-                         optional<Timestamp> expires_) {
+void VectorTile::setMetadata(optional<Timestamp> modified_, optional<Timestamp> expires_) {
     modified = modified_;
     expires = expires_;
+}
 
+void VectorTile::setData(std::shared_ptr<const std::string> data_) {
     GeometryTile::setData(data_ ? std::make_unique<VectorTileData>(data_) : nullptr);
 }
 
diff --git a/src/mbgl/tile/vector_tile.hpp b/src/mbgl/tile/vector_tile.hpp
index 566cde4f37..7dae414fef 100644
--- a/src/mbgl/tile/vector_tile.hpp
+++ b/src/mbgl/tile/vector_tile.hpp
@@ -15,10 +15,9 @@ public:
                const TileParameters&,
                const Tileset&);
 
-    void setNecessity(Necessity) final;
-    void setData(std::shared_ptr<const std::string> data,
-                 optional<Timestamp> modified,
-                 optional<Timestamp> expires);
+    void setNecessity(TileNecessity) final;
+    void setMetadata(optional<Timestamp> modified, optional<Timestamp> expires);
+    void setData(std::shared_ptr<const std::string> data);
 
 private:
     TileLoader<VectorTile> loader;