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

cmake/core-files.cmake

@@ -489,6 +489,7 @@ set(MBGL_CORE_FILES
 
     # tile
     include/mbgl/tile/tile_id.hpp
+    include/mbgl/tile/tile_necessity.hpp
     src/mbgl/tile/geojson_tile.cpp
     src/mbgl/tile/geojson_tile.hpp
     src/mbgl/tile/geojson_tile_data.hpp

include/mbgl/storage/default_file_source.hpp

@@ -34,7 +34,7 @@ class DefaultFileSource : public FileSource {
                       uint64_t maximumCacheSize = util::DEFAULT_MAX_CACHE_SIZE);
     ~DefaultFileSource() override;
 
-    bool supportsOptionalRequests() const override {
+    bool supportsCacheOnlyRequests() const override {
         return true;
     }
 

include/mbgl/storage/file_source.hpp

@@ -24,11 +24,11 @@ class FileSource : private util::noncopyable {
     // not be executed.
     virtual std::unique_ptr<AsyncRequest> request(const Resource&, Callback) = 0;
 
-    // When a file source supports optional requests, it must return true.
-    // Optional requests are requests that aren't as urgent, but could be useful, e.g.
+    // When a file source supports consulting a local cache only, it must return true.
+    // Cache-only requests are requests that aren't as urgent, but could be useful, e.g.
     // to cover part of the map while loading. The FileSource should only do cheap actions to
     // retrieve the data, e.g. load it from a cache, but not from the internet.
-    virtual bool supportsOptionalRequests() const {
+    virtual bool supportsCacheOnlyRequests() const {
         return false;
     }
 };

include/mbgl/storage/resource.hpp

@@ -4,6 +4,8 @@
 #include <mbgl/util/optional.hpp>
 #include <mbgl/util/font_stack.hpp>
 #include <mbgl/util/tileset.hpp>
+#include <mbgl/util/util.hpp>
+#include <mbgl/util/traits.hpp>
 
 #include <string>
 
@@ -30,18 +32,28 @@ class Resource {
         int8_t z;
     };
 
-    enum Necessity : bool {
-        Optional = false,
-        Required = true,
+    enum class LoadingMethod : uint8_t {
+        None        = 0b00,
+        Cache       = 0b01,
+        Network     = 0b10,
+
+        CacheOnly   = Cache,
+        NetworkOnly = Network,
+        All         = Cache | Network,
     };
 
-    Resource(Kind kind_, std::string url_, optional<TileData> tileData_ = {}, Necessity necessity_ = Required)
+    Resource(Kind kind_,
+             std::string url_,
+             optional<TileData> tileData_ = {},
+             LoadingMethod loadingMethod_ = LoadingMethod::All)
         : kind(kind_),
-          necessity(necessity_),
+          loadingMethod(loadingMethod_),
           url(std::move(url_)),
           tileData(std::move(tileData_)) {
     }
 
+    bool hasLoadingMethod(LoadingMethod method);
+
     static Resource style(const std::string& url);
     static Resource source(const std::string& url);
     static Resource tile(const std::string& urlTemplate,
@@ -50,7 +62,7 @@ class Resource {
                          int32_t y,
                          int8_t z,
                          Tileset::Scheme scheme,
-                         Necessity = Required);
+                         LoadingMethod = LoadingMethod::All);
     static Resource glyphs(const std::string& urlTemplate,
                            const FontStack& fontStack,
                            const std::pair<uint16_t, uint16_t>& glyphRange);
@@ -59,7 +71,7 @@ class Resource {
     static Resource image(const std::string& url);
 
     Kind kind;
-    Necessity necessity;
+    LoadingMethod loadingMethod;
     std::string url;
 
     // Includes auxiliary data if this is a tile request.
@@ -71,4 +83,21 @@ class Resource {
     std::shared_ptr<const std::string> priorData;
 };
 
+
+MBGL_CONSTEXPR Resource::LoadingMethod operator|(Resource::LoadingMethod a, Resource::LoadingMethod b) {
+    return Resource::LoadingMethod(mbgl::underlying_type(a) | mbgl::underlying_type(b));
+}
+
+MBGL_CONSTEXPR Resource::LoadingMethod& operator|=(Resource::LoadingMethod& a, Resource::LoadingMethod b) {
+    return (a = a | b);
+}
+
+MBGL_CONSTEXPR Resource::LoadingMethod operator&(Resource::LoadingMethod a, Resource::LoadingMethod b) {
+    return Resource::LoadingMethod(mbgl::underlying_type(a) & mbgl::underlying_type(b));
+}
+
+inline bool Resource::hasLoadingMethod(Resource::LoadingMethod method) {
+    return (loadingMethod & method) != Resource::LoadingMethod::None;
+}
+
 } // namespace mbgl

include/mbgl/tile/tile_necessity.hpp

@@ -0,0 +1,15 @@
+#pragma once
+
+namespace mbgl {
+
+// 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)
+enum class TileNecessity : bool {
+    Optional = false,
+    Required = true,
+};
+
+} // namespace mbgl

platform/default/default_file_source.cpp

@@ -126,47 +126,41 @@ class DefaultFileSource::Impl {
             tasks[req] = localFileSource->request(resource, callback);
         } else {
             // Try the offline database
-            const bool hasPrior = resource.priorEtag || resource.priorModified ||
-                                  resource.priorExpires || resource.priorData;
-            if (!hasPrior || resource.necessity == Resource::Optional) {
+            if (resource.hasLoadingMethod(Resource::LoadingMethod::Cache)) {
                 auto offlineResponse = offlineDatabase->get(resource);
 
-                if (resource.necessity == Resource::Optional && !offlineResponse) {
-                    // Ensure there's always a response that we can send, so the caller knows that
-                    // there's no optional data available in the cache.
-                    offlineResponse.emplace();
-                    offlineResponse->noContent = true;
-                    offlineResponse->error = std::make_unique<Response::Error>(
-                            Response::Error::Reason::NotFound, "Not found in offline database");
-                }
-
-                if (offlineResponse) {
+                if (resource.loadingMethod == Resource::LoadingMethod::CacheOnly) {
+                    if (!offlineResponse) {
+                        // Ensure there's always a response that we can send, so the caller knows that
+                        // there's no optional data available in the cache, when it's the only place
+                        // we're supposed to load from.
+                        offlineResponse.emplace();
+                        offlineResponse->noContent = true;
+                        offlineResponse->error = std::make_unique<Response::Error>(
+                                Response::Error::Reason::NotFound, "Not found in offline database");
+                    } else if (!offlineResponse->isUsable()) {
+                        // Don't return resources the server requested not to show when they're stale.
+                        // Even if we can't directly use the response, we may still use it to send a
+                        // conditional HTTP request, which is why we're saving it above.
+                        offlineResponse->error = std::make_unique<Response::Error>(
+                            Response::Error::Reason::NotFound, "Cached resource is unusable");
+                    }
+                    callback(*offlineResponse);
+                } else if (offlineResponse) {
+                    // Copy over the fields so that we can use them when making a refresh request.
                     resource.priorModified = offlineResponse->modified;
                     resource.priorExpires = offlineResponse->expires;
                     resource.priorEtag = offlineResponse->etag;
+                    resource.priorData = offlineResponse->data;
 
-                    // Don't return resources the server requested not to show when they're stale.
-                    // Even if we can't directly use the response, we may still use it to send a
-                    // conditional HTTP request.
                     if (offlineResponse->isUsable()) {
                         callback(*offlineResponse);
-                    } else if (resource.necessity == Resource::Optional) {
-                        // Instead of the data that we got, return a not found error so that
-                        // underlying implementations know about the fact that we couldn't find
-                        // usable cache data.
-                        offlineResponse->error = std::make_unique<Response::Error>(
-                            Response::Error::Reason::NotFound, "Cached resource is unusable");
-                        callback(*offlineResponse);
-                    } else {
-                        // Since we can't return the data immediately, we'll have to hold on so that
-                        // we can return it later in case we get a 304 Not Modified response.
-                        resource.priorData = offlineResponse->data;
                     }
                 }
             }
 
             // Get from the online file source
-            if (resource.necessity == Resource::Required) {
+            if (resource.hasLoadingMethod(Resource::LoadingMethod::Network)) {
                 tasks[req] = onlineFileSource.request(resource, [=] (Response onlineResponse) mutable {
                     this->offlineDatabase->put(resource, onlineResponse);
                     callback(onlineResponse);

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,23 +40,23 @@ 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) {
                 // We're looking for an overzoomed child tile.
                 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()) {

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_,

src/mbgl/annotation/annotation_tile.hpp

@@ -14,8 +14,6 @@ class AnnotationTile : public GeometryTile {
     AnnotationTile(const OverscaledTileID&, const TileParameters&);
     ~AnnotationTile() override;
 
-    void setNecessity(Necessity) final;
-
 private:
     AnnotationManager& annotationManager;
 };

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++);

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
     };
 }
 

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) {

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,

src/mbgl/tile/geojson_tile.hpp

@@ -15,8 +15,6 @@ class GeoJSONTile : public GeometryTile {
                 mapbox::geometry::feature_collection<int16_t>);
 
     void updateData(mapbox::geometry::feature_collection<int16_t>);
-
-    void setNecessity(Necessity) final;
 
     void querySourceFeatures(
         std::vector<Feature>& result,

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);
 }
 

src/mbgl/tile/raster_tile.hpp

@@ -22,12 +22,11 @@ class RasterTile : public Tile {
                    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;