diff options
-rw-r--r-- | platform/default/default_file_source.cpp | 24 | ||||
-rw-r--r-- | src/mbgl/tile/tile_loader_impl.hpp | 3 | ||||
-rw-r--r-- | test/storage/default_file_source.test.cpp | 78 |
3 files changed, 96 insertions, 9 deletions
diff --git a/platform/default/default_file_source.cpp b/platform/default/default_file_source.cpp index 1e855dd9f7..3fdb03e6b4 100644 --- a/platform/default/default_file_source.cpp +++ b/platform/default/default_file_source.cpp @@ -126,9 +126,8 @@ public: tasks[req] = localFileSource->request(resource, callback); } else { // Try the offline database - Resource revalidation = resource; - - const bool hasPrior = resource.priorEtag || resource.priorModified || resource.priorExpires; + const bool hasPrior = resource.priorEtag || resource.priorModified || + resource.priorExpires || resource.priorData; if (!hasPrior || resource.necessity == Resource::Optional) { auto offlineResponse = offlineDatabase->get(resource); @@ -142,27 +141,34 @@ public: } if (offlineResponse) { - revalidation.priorModified = offlineResponse->modified; - revalidation.priorExpires = offlineResponse->expires; - revalidation.priorEtag = offlineResponse->etag; + resource.priorModified = offlineResponse->modified; + resource.priorExpires = offlineResponse->expires; + resource.priorEtag = offlineResponse->etag; // 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. - revalidation.priorData = offlineResponse->data; + resource.priorData = offlineResponse->data; } } } // Get from the online file source if (resource.necessity == Resource::Required) { - tasks[req] = onlineFileSource.request(revalidation, [=] (Response onlineResponse) mutable { - this->offlineDatabase->put(revalidation, onlineResponse); + tasks[req] = onlineFileSource.request(resource, [=] (Response onlineResponse) mutable { + this->offlineDatabase->put(resource, onlineResponse); callback(onlineResponse); }); } diff --git a/src/mbgl/tile/tile_loader_impl.hpp b/src/mbgl/tile/tile_loader_impl.hpp index 899cbaf9b0..598ec32c10 100644 --- a/src/mbgl/tile/tile_loader_impl.hpp +++ b/src/mbgl/tile/tile_loader_impl.hpp @@ -61,7 +61,10 @@ void TileLoader<T>::loadOptional() { // 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. + resource.priorModified = res.modified; resource.priorExpires = Timestamp{ Seconds::zero() }; + resource.priorEtag = res.etag; + resource.priorData = res.data; } else { loadedData(res); } diff --git a/test/storage/default_file_source.test.cpp b/test/storage/default_file_source.test.cpp index b8aebae7e7..b5686b5ffe 100644 --- a/test/storage/default_file_source.test.cpp +++ b/test/storage/default_file_source.test.cpp @@ -571,3 +571,81 @@ TEST(DefaultFileSource, TEST_REQUIRES_SERVER(SetResourceTransform)) { loop.run(); } + +// Test that a stale cache file that has must-revalidate set will trigger a response. +TEST(DefaultFileSource, TEST_REQUIRES_SERVER(RespondToStaleMustRevalidate)) { + util::RunLoop loop; + DefaultFileSource fs(":memory:", "."); + + Resource resource { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" }; + resource.necessity = Resource::Necessity::Optional; + + // using namespace std::chrono_literals; + + // Put an existing value in the cache that has expired, and has must-revalidate set. + Response response; + response.data = std::make_shared<std::string>("Cached value"); + response.modified = Timestamp(Seconds(1417392000)); // December 1, 2014 + response.expires = Timestamp(Seconds(1417392000)); + response.mustRevalidate = true; + response.etag.emplace("snowfall"); + fs.put(resource, response); + + std::unique_ptr<AsyncRequest> req; + req = fs.request(resource, [&](Response res) { + req.reset(); + ASSERT_TRUE(res.error.get()); + EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason); + EXPECT_EQ("Cached resource is unusable", res.error->message); + EXPECT_FALSE(res.notModified); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Cached value", *res.data); + ASSERT_TRUE(res.expires); + EXPECT_EQ(Timestamp{ Seconds(1417392000) }, *res.expires); + EXPECT_TRUE(res.mustRevalidate); + ASSERT_TRUE(res.modified); + EXPECT_EQ(Timestamp{ Seconds(1417392000) }, *res.modified); + ASSERT_TRUE(res.etag); + EXPECT_EQ("snowfall", *res.etag); + + resource.priorEtag = res.etag; + resource.priorModified = res.modified; + resource.priorExpires = res.expires; + resource.priorData = res.data; + + loop.stop(); + }); + + loop.run(); + + // Now run this request again, with the data we gathered from the previous stale/unusable + // request. We're replacing the data so that we can check that the DefaultFileSource doesn't + // attempt another database access if we already have the value. + resource.necessity = Resource::Necessity::Required; + resource.priorData = std::make_shared<std::string>("Prior value"); + + req = fs.request(resource, [&](Response res) { + req.reset(); + ASSERT_EQ(nullptr, res.error.get()); + // Since the data was found in the cache, we're doing a revalidation request. Yet, since + // this request hasn't returned data before, we're setting notModified to false in the + // OnlineFileSource to ensure that requestors know that this is the first time they're + // seeing this data. + EXPECT_FALSE(res.notModified); + ASSERT_TRUE(res.data.get()); + // Ensure that it's the value that we manually inserted into the cache rather than the value + // the server returns, since we should be executing a revalidation request which doesn't + // return new data, only a 304 Not Modified response. + EXPECT_EQ("Prior value", *res.data); + ASSERT_TRUE(res.expires); + EXPECT_LE(util::now(), *res.expires); + EXPECT_TRUE(res.mustRevalidate); + ASSERT_TRUE(res.modified); + EXPECT_EQ(Timestamp{ Seconds(1417392000) }, *res.modified); + ASSERT_TRUE(res.etag); + EXPECT_EQ("snowfall", *res.etag); + loop.stop(); + }); + + loop.run(); +} |