[core] Eliminate Response::stale and inline Response::isExpired()

Response::isExpired() had subtle and potentially confusing behavior around Seconds::zero(). It's best to inline it and comment why.

12 files changed, 64 insertions, 116 deletions

diff --git a/test/storage/asset_file_source.cpp b/test/storage/asset_file_source.cpp
index d7910373f6..22ee3f793a 100644
--- a/test/storage/asset_file_source.cpp
+++ b/test/storage/asset_file_source.cpp
@@ -30,10 +30,6 @@ public:
             ASSERT_TRUE(res.data.get());
             EXPECT_EQ("content is here\n", *res.data);
 
-            if (res.stale) {
-                return;
-            }
-
             if (!--numRequests) {
                 endCallback();
                 request.reset();
@@ -104,7 +100,6 @@ TEST_F(Storage, AssetEmptyFile) {
     std::unique_ptr<FileRequest> req = fs.request({ Resource::Unknown, "asset://empty" }, [&](Response res) {
         req.reset();
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("", *res.data);
         loop.stop();
@@ -126,7 +121,6 @@ TEST_F(Storage, AssetNonEmptyFile) {
     std::unique_ptr<FileRequest> req = fs.request({ Resource::Unknown, "asset://nonempty" }, [&](Response res) {
         req.reset();
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("content is here\n", *res.data);
         loop.stop();
@@ -149,7 +143,6 @@ TEST_F(Storage, AssetNonExistentFile) {
         req.reset();
         ASSERT_NE(nullptr, res.error);
         EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason);
-        EXPECT_EQ(false, res.stale);
         ASSERT_FALSE(res.data.get());
         // Do not assert on platform-specific error message.
         loop.stop();
@@ -172,7 +165,6 @@ TEST_F(Storage, AssetReadDirectory) {
         req.reset();
         ASSERT_NE(nullptr, res.error);
         EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason);
-        EXPECT_EQ(false, res.stale);
         ASSERT_FALSE(res.data.get());
         // Do not assert on platform-specific error message.
         loop.stop();
diff --git a/test/storage/cache_response.cpp b/test/storage/cache_response.cpp
index 26f439ea8f..511403e840 100644
--- a/test/storage/cache_response.cpp
+++ b/test/storage/cache_response.cpp
@@ -23,7 +23,6 @@ TEST_F(Storage, CacheResponse) {
     req1 = fs.request(resource, [&](Response res) {
         req1.reset();
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Response 1", *res.data);
         EXPECT_LT(Seconds::zero(), res.expires);
@@ -36,7 +35,6 @@ TEST_F(Storage, CacheResponse) {
         req2 = fs.request(resource, [&](Response res2) {
             req2.reset();
             EXPECT_EQ(response.error, res2.error);
-            EXPECT_EQ(response.stale, res2.stale);
             ASSERT_TRUE(res2.data.get());
             EXPECT_EQ(*response.data, *res2.data);
             EXPECT_EQ(response.expires, res2.expires);
@@ -78,7 +76,6 @@ TEST_F(Storage, CacheNotFound) {
     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);
@@ -136,7 +133,6 @@ TEST_F(Storage, DontCacheConnectionErrors) {
     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);
@@ -192,7 +188,6 @@ TEST_F(Storage, DontCacheServerErrors) {
     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);
diff --git a/test/storage/cache_revalidate.cpp b/test/storage/cache_revalidate.cpp
index a440b69aaf..483a7edc74 100644
--- a/test/storage/cache_revalidate.cpp
+++ b/test/storage/cache_revalidate.cpp
@@ -17,47 +17,39 @@ TEST_F(Storage, CacheRevalidateSame) {
     const Resource revalidateSame { Resource::Unknown, "http://127.0.0.1:3000/revalidate-same" };
     std::unique_ptr<FileRequest> req1;
     std::unique_ptr<FileRequest> req2;
+    uint16_t counter = 0;
+
+    // First request causes the response to get cached.
     req1 = fs.request(revalidateSame, [&](Response res) {
-        // This callback can get triggered multiple times. We only care about the first invocation.
-        // It will get triggered again when refreshing the req2 (see below).
-        static bool first = true;
-        if (!first) {
-            return;
-        }
-        first = false;
+        req1.reset();
 
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Response", *res.data);
         EXPECT_EQ(Seconds::zero(), res.expires);
         EXPECT_EQ(Seconds::zero(), res.modified);
         EXPECT_EQ("snowfall", res.etag);
 
+        // Second request returns the cached response, then immediately revalidates.
         req2 = fs.request(revalidateSame, [&, res](Response res2) {
-            if (res2.stale) {
-                // Discard stale responses, if any.
-                return;
+            if (counter == 0) {
+                ++counter;
+                EXPECT_FALSE(res2.notModified);
+            } else {
+                req2.reset();
+
+                EXPECT_EQ(nullptr, res2.error);
+                EXPECT_TRUE(res2.notModified);
+                ASSERT_TRUE(res2.data.get());
+                EXPECT_EQ("Response", *res2.data);
+                EXPECT_LT(Seconds::zero(), res2.expires);
+                EXPECT_EQ(Seconds::zero(), res2.modified);
+                // We're not sending the ETag in the 304 reply, but it should still be there.
+                EXPECT_EQ("snowfall", res2.etag);
+
+                loop.stop();
+                CacheRevalidateSame.finish();
             }
-
-            ASSERT_TRUE(req1.get());
-            req1.reset();
-
-            ASSERT_TRUE(req2.get());
-            req2.reset();
-
-            EXPECT_EQ(nullptr, res2.error);
-            EXPECT_EQ(false, res2.stale);
-            EXPECT_TRUE(res2.notModified);
-            ASSERT_TRUE(res2.data.get());
-            EXPECT_EQ("Response", *res2.data);
-            EXPECT_LT(Seconds::zero(), res2.expires);
-            EXPECT_EQ(Seconds::zero(), res2.modified);
-            // We're not sending the ETag in the 304 reply, but it should still be there.
-            EXPECT_EQ("snowfall", res2.etag);
-
-            loop.stop();
-            CacheRevalidateSame.finish();
         });
     });
 
@@ -77,46 +69,38 @@ TEST_F(Storage, CacheRevalidateModified) {
                                        "http://127.0.0.1:3000/revalidate-modified" };
     std::unique_ptr<FileRequest> req1;
     std::unique_ptr<FileRequest> req2;
+    uint16_t counter = 0;
+
+    // First request causes the response to get cached.
     req1 = fs.request(revalidateModified, [&](Response res) {
-        // This callback can get triggered multiple times. We only care about the first invocation.
-        // It will get triggered again when refreshing the req2 (see below).
-        static bool first = true;
-        if (!first) {
-            return;
-        }
-        first = false;
+        req1.reset();
 
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Response", *res.data);
         EXPECT_EQ(Seconds::zero(), res.expires);
         EXPECT_EQ(1420070400, res.modified.count());
         EXPECT_EQ("", res.etag);
 
+        // Second request returns the cached response, then immediately revalidates.
         req2 = fs.request(revalidateModified, [&, res](Response res2) {
-            if (res2.stale) {
-                // Discard stale responses, if any.
-                return;
+            if (counter == 0) {
+                ++counter;
+                EXPECT_FALSE(res2.notModified);
+            } else {
+                req2.reset();
+
+                EXPECT_EQ(nullptr, res2.error);
+                EXPECT_TRUE(res2.notModified);
+                ASSERT_TRUE(res2.data.get());
+                EXPECT_EQ("Response", *res2.data);
+                EXPECT_LT(Seconds::zero(), res2.expires);
+                EXPECT_EQ(1420070400, res2.modified.count());
+                EXPECT_EQ("", res2.etag);
+
+                loop.stop();
+                CacheRevalidateModified.finish();
             }
-
-            ASSERT_TRUE(req1.get());
-            req1.reset();
-
-            ASSERT_TRUE(req2.get());
-            req2.reset();
-
-            EXPECT_EQ(nullptr, res2.error);
-            EXPECT_EQ(false, res2.stale);
-            EXPECT_TRUE(res2.notModified);
-            ASSERT_TRUE(res2.data.get());
-            EXPECT_EQ("Response", *res2.data);
-            EXPECT_LT(Seconds::zero(), res2.expires);
-            EXPECT_EQ(1420070400, res2.modified.count());
-            EXPECT_EQ("", res2.etag);
-
-            loop.stop();
-            CacheRevalidateModified.finish();
         });
     });
 
@@ -135,46 +119,38 @@ TEST_F(Storage, CacheRevalidateEtag) {
     const Resource revalidateEtag { Resource::Unknown, "http://127.0.0.1:3000/revalidate-etag" };
     std::unique_ptr<FileRequest> req1;
     std::unique_ptr<FileRequest> req2;
+    uint16_t counter = 0;
+
+    // First request causes the response to get cached.
     req1 = fs.request(revalidateEtag, [&](Response res) {
-        // This callback can get triggered multiple times. We only care about the first invocation.
-        // It will get triggered again when refreshing the req2 (see below).
-        static bool first = true;
-        if (!first) {
-            return;
-        }
-        first = false;
+        req1.reset();
 
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Response 1", *res.data);
         EXPECT_EQ(Seconds::zero(), res.expires);
         EXPECT_EQ(Seconds::zero(), res.modified);
         EXPECT_EQ("response-1", res.etag);
 
+        // Second request returns the cached response, then immediately revalidates.
         req2 = fs.request(revalidateEtag, [&, res](Response res2) {
-            if (res2.stale) {
-                // Discard stale responses, if any.
-                return;
+            if (counter == 0) {
+                ++counter;
+                EXPECT_FALSE(res2.notModified);
+            } else {
+                req2.reset();
+
+                EXPECT_EQ(nullptr, res2.error);
+                ASSERT_TRUE(res2.data.get());
+                EXPECT_NE(res.data, res2.data);
+                EXPECT_EQ("Response 2", *res2.data);
+                EXPECT_EQ(Seconds::zero(), res2.expires);
+                EXPECT_EQ(Seconds::zero(), res2.modified);
+                EXPECT_EQ("response-2", res2.etag);
+
+                loop.stop();
+                CacheRevalidateEtag.finish();
             }
-
-            ASSERT_TRUE(req1.get());
-            req1.reset();
-
-            ASSERT_TRUE(req2.get());
-            req2.reset();
-
-            EXPECT_EQ(nullptr, res2.error);
-            EXPECT_EQ(false, res2.stale);
-            ASSERT_TRUE(res2.data.get());
-            EXPECT_NE(res.data, res2.data);
-            EXPECT_EQ("Response 2", *res2.data);
-            EXPECT_EQ(Seconds::zero(), res2.expires);
-            EXPECT_EQ(Seconds::zero(), res2.modified);
-            EXPECT_EQ("response-2", res2.etag);
-
-            loop.stop();
-            CacheRevalidateEtag.finish();
         });
     });
 
diff --git a/test/storage/http_cancel.cpp b/test/storage/http_cancel.cpp
index a20e78fa53..4b6ff75381 100644
--- a/test/storage/http_cancel.cpp
+++ b/test/storage/http_cancel.cpp
@@ -41,7 +41,6 @@ TEST_F(Storage, HTTPCancelMultiple) {
     std::unique_ptr<FileRequest> req = fs.request(resource, [&](Response res) {
         req.reset();
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Hello World!", *res.data);
         EXPECT_EQ(Seconds::zero(), res.expires);
diff --git a/test/storage/http_error.cpp b/test/storage/http_error.cpp
index 3ed7638d48..159e5a17a1 100644
--- a/test/storage/http_error.cpp
+++ b/test/storage/http_error.cpp
@@ -26,7 +26,6 @@ TEST_F(Storage, HTTPTemporaryError) {
             ASSERT_NE(nullptr, res.error);
             EXPECT_EQ(Response::Error::Reason::Server, res.error->reason);
             EXPECT_EQ("HTTP status code 500", res.error->message);
-            EXPECT_EQ(false, res.stale);
             ASSERT_TRUE(res.data.get());
             EXPECT_EQ("", *res.data);
             EXPECT_EQ(Seconds::zero(), res.expires);
@@ -39,7 +38,6 @@ TEST_F(Storage, HTTPTemporaryError) {
             EXPECT_LT(0.99, duration) << "Backoff timer didn't wait 1 second";
             EXPECT_GT(1.2, duration) << "Backoff timer fired too late";
             EXPECT_EQ(nullptr, res.error);
-            EXPECT_EQ(false, res.stale);
             ASSERT_TRUE(res.data.get());
             EXPECT_EQ("Hello World!", *res.data);
             EXPECT_EQ(Seconds::zero(), res.expires);
@@ -83,7 +81,6 @@ TEST_F(Storage, HTTPConnectionError) {
 #else
         FAIL();
 #endif
-        EXPECT_EQ(false, res.stale);
         ASSERT_FALSE(res.data.get());
         EXPECT_EQ(Seconds::zero(), res.expires);
         EXPECT_EQ(Seconds::zero(), res.modified);
diff --git a/test/storage/http_header_parsing.cpp b/test/storage/http_header_parsing.cpp
index 72c42e8f0a..14bd99bdb3 100644
--- a/test/storage/http_header_parsing.cpp
+++ b/test/storage/http_header_parsing.cpp
@@ -19,7 +19,6 @@ TEST_F(Storage, HTTPExpiresParsing) {
                [&](Response res) {
         req1.reset();
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Hello World!", *res.data);
         EXPECT_EQ(1420797926, res.expires.count());
@@ -46,7 +45,6 @@ TEST_F(Storage, HTTPCacheControlParsing) {
                [&](Response res) {
         req2.reset();
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Hello World!", *res.data);
         EXPECT_GT(2, std::abs(res.expires.count() - now.count() - 120)) << "Expiration date isn't about 120 seconds in the future";
diff --git a/test/storage/http_issue_1369.cpp b/test/storage/http_issue_1369.cpp
index 81cf964e7c..bf67da0da1 100644
--- a/test/storage/http_issue_1369.cpp
+++ b/test/storage/http_issue_1369.cpp
@@ -34,7 +34,6 @@ TEST_F(Storage, HTTPIssue1369) {
     req = fs.request(resource, [&](Response res) {
         req.reset();
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Hello World!", *res.data);
         EXPECT_EQ(Seconds::zero(), res.expires);
diff --git a/test/storage/http_load.cpp b/test/storage/http_load.cpp
index 9b37cbd231..0750e04cf4 100644
--- a/test/storage/http_load.cpp
+++ b/test/storage/http_load.cpp
@@ -25,7 +25,6 @@ TEST_F(Storage, HTTPLoad) {
                    [&, i, current](Response res) {
             reqs[i].reset();
             EXPECT_EQ(nullptr, res.error);
-            EXPECT_EQ(false, res.stale);
             ASSERT_TRUE(res.data.get());
             EXPECT_EQ(std::string("Request ") +  std::to_string(current), *res.data);
             EXPECT_EQ(Seconds::zero(), res.expires);
diff --git a/test/storage/http_other_loop.cpp b/test/storage/http_other_loop.cpp
index 9d98f1878b..0fbe43b3d3 100644
--- a/test/storage/http_other_loop.cpp
+++ b/test/storage/http_other_loop.cpp
@@ -17,7 +17,6 @@ TEST_F(Storage, HTTPOtherLoop) {
                [&](Response res) {
         req.reset();
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Hello World!", *res.data);
         EXPECT_EQ(Seconds::zero(), res.expires);
diff --git a/test/storage/http_reading.cpp b/test/storage/http_reading.cpp
index a4a7d0480d..ba3907eb3a 100644
--- a/test/storage/http_reading.cpp
+++ b/test/storage/http_reading.cpp
@@ -21,7 +21,6 @@ TEST_F(Storage, HTTPTest) {
         req1.reset();
         EXPECT_TRUE(util::ThreadContext::currentlyOn(util::ThreadType::Main));
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Hello World!", *res.data);
         EXPECT_EQ(Seconds::zero(), res.expires);
@@ -48,7 +47,6 @@ TEST_F(Storage, HTTP404) {
         EXPECT_TRUE(util::ThreadContext::currentlyOn(util::ThreadType::Main));
         ASSERT_NE(nullptr, res.error);
         EXPECT_EQ(Response::Error::Reason::NotFound, res.error->reason);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Cannot GET /doesnotexist\n", *res.data);
         EXPECT_EQ("HTTP status code 404", res.error->message);
@@ -76,7 +74,6 @@ TEST_F(Storage, HTTP500) {
         EXPECT_TRUE(util::ThreadContext::currentlyOn(util::ThreadType::Main));
         ASSERT_NE(nullptr, res.error);
         EXPECT_EQ(Response::Error::Reason::Server, res.error->reason);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Server Error!", *res.data);
         EXPECT_EQ("HTTP status code 500", res.error->message);
diff --git a/test/storage/http_retry_network_status.cpp b/test/storage/http_retry_network_status.cpp
index 7816fab01d..1925e714d4 100644
--- a/test/storage/http_retry_network_status.cpp
+++ b/test/storage/http_retry_network_status.cpp
@@ -26,7 +26,6 @@ TEST_F(Storage, HTTPNetworkStatusChange) {
     std::unique_ptr<FileRequest> req = fs.request(resource, [&](Response res) {
          req.reset();
          EXPECT_EQ(nullptr, res.error);
-         EXPECT_EQ(false, res.stale);
          ASSERT_TRUE(res.data.get());
          EXPECT_EQ("Response", *res.data);
          EXPECT_EQ(Seconds::zero(), res.expires);
@@ -87,7 +86,6 @@ TEST_F(Storage, HTTPNetworkStatusChangePreempt) {
 #else
         FAIL();
 #endif
-        EXPECT_EQ(false, res.stale);
         ASSERT_FALSE(res.data.get());
         EXPECT_EQ(Seconds::zero(), res.expires);
         EXPECT_EQ(Seconds::zero(), res.modified);
diff --git a/test/storage/http_timeout.cpp b/test/storage/http_timeout.cpp
index 241d5f5549..c736dbe05f 100644
--- a/test/storage/http_timeout.cpp
+++ b/test/storage/http_timeout.cpp
@@ -19,7 +19,6 @@ TEST_F(Storage, HTTPTimeout) {
     std::unique_ptr<FileRequest> req = fs.request(resource, [&](Response res) {
         counter++;
         EXPECT_EQ(nullptr, res.error);
-        EXPECT_EQ(false, res.stale);
         ASSERT_TRUE(res.data.get());
         EXPECT_EQ("Hello World!", *res.data);
         EXPECT_LT(Seconds::zero(), res.expires);