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

10 files changed, 56 insertions, 64 deletions

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;