diff options
author | Konstantin Käfer <mail@kkaefer.com> | 2016-01-07 11:40:43 +0100 |
---|---|---|
committer | Konstantin Käfer <mail@kkaefer.com> | 2016-01-08 16:25:54 +0100 |
commit | a06f28ddb70cd1963076d6df2ef3716d45b7a13e (patch) | |
tree | 3a7f15ddd8ebb6d552e26005e1d8abc2c380e5fe | |
parent | c5feadf0bd04d77335f56f79c1dea75a3c43c976 (diff) | |
download | qtlocation-mapboxgl-a06f28ddb70cd1963076d6df2ef3716d45b7a13e.tar.gz |
[core] only cache successful or NotFound responses
We don't want other types of error end up in our cache, since it'll likely evict perfectly good content.
-rw-r--r-- | platform/default/online_file_source.cpp | 7 | ||||
-rw-r--r-- | test/storage/cache_response.cpp | 172 | ||||
-rwxr-xr-x | test/storage/server.js | 4 |
3 files changed, 182 insertions, 1 deletions
diff --git a/platform/default/online_file_source.cpp b/platform/default/online_file_source.cpp index 37ac3e9838..120f21230c 100644 --- a/platform/default/online_file_source.cpp +++ b/platform/default/online_file_source.cpp @@ -276,7 +276,12 @@ void OnlineFileSource::Impl::startRealRequest(OnlineFileRequestImpl& request) { auto callback = [this, &request](std::shared_ptr<const Response> response) { request.realRequest = nullptr; - if (cache && !util::isAssetURL(request.resource.url)) { + // Only update the cache for successful or 404 responses. + // In particular, we don't want to write a Canceled request, or one that failed due to + // connection errors to the cache. Server errors are hopefully also temporary, so we're not + // caching them either. + if (cache && !util::isAssetURL(request.resource.url) && + (!response->error || (response->error->reason == Response::Error::Reason::NotFound))) { // Store response in database. Make sure we only refresh the expires column if the data // didn't change. FileCache::Hint hint = FileCache::Hint::Full; diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp index 16a9c3d47a..8339d8aa84 100644 --- a/test/storage/cache_response.cpp +++ b/test/storage/cache_response.cpp @@ -50,3 +50,175 @@ TEST_F(Storage, CacheResponse) { loop.run(); } + +// Make sure we don't store a bad server response into the cache +TEST_F(Storage, CacheNotFound) { + SCOPED_TEST(CacheNotFound); + + using namespace mbgl; + + util::RunLoop loop; + SQLiteCache cache(":memory:"); + OnlineFileSource fs(&cache); + + const Resource resource{ Resource::Unknown, "http://127.0.0.1:3000/not-found" }; + + // Insert existing data into the cache that will be marked as stale. + auto response = std::make_shared<Response>(); + response->data = std::make_shared<const std::string>("existing data"); + cache.put(resource, response, FileCache::Hint::Full); + + std::unique_ptr<FileRequest> req1; + std::unique_ptr<WorkRequest> req2; + + int counter = 0; + + // Then, request the actual URL and check that we're getting the rigged cache response first, + // then the connection error message. + req1 = fs.request(resource, [&](Response res) { + if (counter == 0) { + EXPECT_EQ(nullptr, res.error); + EXPECT_EQ(true, res.stale); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("existing data", *res.data); + EXPECT_EQ(Seconds::zero(), res.expires); + EXPECT_EQ(Seconds::zero(), res.modified); + EXPECT_EQ("", res.etag); + } else if (counter == 1) { + EXPECT_NE(nullptr, res.error); + EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Not Found!", *res.data); + req1.reset(); + + // Finally, check the cache to make sure we cached the 404 response. + req2 = cache.get(resource, [&](std::unique_ptr<Response> res2) { + EXPECT_NE(nullptr, res2->error); + EXPECT_EQ(Response::Error::Reason::NotFound, res2->error->reason); + ASSERT_TRUE(res2->data.get()); + EXPECT_EQ("Not Found!", *res2->data); + CacheNotFound.finish(); + loop.stop(); + }); + } else { + FAIL() << "Got too many responses"; + } + counter++; + }); + + loop.run(); +} + +// Make sure we don't store a bad server response into the cache +TEST_F(Storage, DontCacheConnectionErrors) { + SCOPED_TEST(DontCacheConnectionErrors); + + using namespace mbgl; + + util::RunLoop loop; + SQLiteCache cache(":memory:"); + OnlineFileSource fs(&cache); + + const Resource resource{ Resource::Unknown, "http://127.0.0.1:3001" }; + + // Insert existing data into the cache that will be marked as stale. + auto response = std::make_shared<Response>(); + response->data = std::make_shared<const std::string>("existing data"); + cache.put(resource, response, FileCache::Hint::Full); + + std::unique_ptr<FileRequest> req1; + std::unique_ptr<WorkRequest> req2; + + int counter = 0; + + // Then, request the actual URL and check that we're getting the rigged cache response first, + // then the connection error message. + req1 = fs.request(resource, [&](Response res) { + if (counter == 0) { + EXPECT_EQ(nullptr, res.error); + EXPECT_EQ(true, res.stale); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("existing data", *res.data); + EXPECT_EQ(Seconds::zero(), res.expires); + EXPECT_EQ(Seconds::zero(), res.modified); + EXPECT_EQ("", res.etag); + } else if (counter == 1) { + EXPECT_NE(nullptr, res.error); + EXPECT_EQ(Response::Error::Reason::Connection, res.error->reason); + req1.reset(); + + // Finally, check the cache to make sure we still have our original data in there rather + // than the failed connection attempt. + req2 = cache.get(resource, [&](std::unique_ptr<Response> res2) { + EXPECT_EQ(nullptr, res2->error); + ASSERT_TRUE(res2->data.get()); + EXPECT_EQ("existing data", *res2->data); + DontCacheConnectionErrors.finish(); + loop.stop(); + }); + } else { + FAIL() << "Got too many responses"; + } + counter++; + }); + + loop.run(); +} + +// Make sure we don't store a bad server response into the cache +TEST_F(Storage, DontCacheServerErrors) { + SCOPED_TEST(DontCacheServerErrors); + + using namespace mbgl; + + util::RunLoop loop; + SQLiteCache cache(":memory:"); + OnlineFileSource fs(&cache); + + const Resource resource{ Resource::Unknown, "http://127.0.0.1:3000/permanent-error" }; + + // Insert existing data into the cache that will be marked as stale. + auto response = std::make_shared<Response>(); + response->data = std::make_shared<const std::string>("existing data"); + cache.put(resource, response, FileCache::Hint::Full); + + std::unique_ptr<FileRequest> req1; + std::unique_ptr<WorkRequest> req2; + + int counter = 0; + + // Then, request the actual URL and check that we're getting the rigged cache response first, + // then the server error message. + req1 = fs.request(resource, [&](Response res) { + if (counter == 0) { + EXPECT_EQ(nullptr, res.error); + EXPECT_EQ(true, res.stale); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("existing data", *res.data); + EXPECT_EQ(Seconds::zero(), res.expires); + EXPECT_EQ(Seconds::zero(), res.modified); + EXPECT_EQ("", res.etag); + } else if (counter == 1) { + EXPECT_NE(nullptr, res.error); + EXPECT_EQ(Response::Error::Reason::Server, res.error->reason); + ASSERT_TRUE(res.data.get()); + EXPECT_EQ("Server Error!", *res.data); + req1.reset(); + + // Finally, check the cache to make sure we still have our original data in there rather + // than the failed connection attempt. + req2 = cache.get(resource, [&](std::unique_ptr<Response> res2) { + EXPECT_EQ(nullptr, res2->error); + ASSERT_TRUE(res2->data.get()); + EXPECT_EQ("existing data", *res2->data); + DontCacheServerErrors.finish(); + loop.stop(); + }); + } else { + FAIL() << "Got too many responses"; + } + counter++; + }); + + loop.run(); +} diff --git a/test/storage/server.js b/test/storage/server.js index a814ce15e6..5d547b1506 100755 --- a/test/storage/server.js +++ b/test/storage/server.js @@ -81,6 +81,10 @@ app.get('/revalidate-etag', function(req, res) { revalidateEtagCounter++; }); +app.get('/not-found', function(req, res) { + res.status(404).send('Not Found!'); +}); + app.get('/permanent-error', function(req, res) { res.status(500).send('Server Error!'); }); |