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

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;

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

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