From b51831a48f06ad28f627c3624e5edb41598a2bf8 Mon Sep 17 00:00:00 2001
From: Jeff Davidson <jpd@google.com>
Date: Tue, 26 Jan 2021 09:30:14 -0800
Subject: [PATCH] Use a consistent timebase when evaluating soft/hard TTLs.
 (#391)

Previously, we'd evaluate whether a cache entry was valid per these TTLs using two different checks against the current system time. This could result in a spurious trigger of soft TTL logic if the first time check occurs before the soft TTL expiry but the second occurs after it, and the hard and soft TTL are identical.

Unit tests are omitted since we don't have an easy way to mock the clock with the current architecture. This could be improved in the future.
---
 .../com/android/volley/AsyncRequestQueue.java    | 16 ++++++++++++----
 src/main/java/com/android/volley/Cache.java      | 12 ++++++++++--
 .../java/com/android/volley/CacheDispatcher.java | 10 ++++++++--
 3 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/src/main/java/com/android/volley/AsyncRequestQueue.java b/src/main/java/com/android/volley/AsyncRequestQueue.java
index 2370d485..41cafbcb 100644
--- a/src/main/java/com/android/volley/AsyncRequestQueue.java
+++ b/src/main/java/com/android/volley/AsyncRequestQueue.java
@@ -270,8 +270,14 @@ private void handleEntry(final Entry entry, final Request<?> mRequest) {
             return;
         }
 
+        // Use a single instant to evaluate cache expiration. Otherwise, a cache entry with
+        // identical soft and hard TTL times may appear to be valid when checking isExpired but
+        // invalid upon checking refreshNeeded(), triggering a soft TTL refresh which should be
+        // impossible.
+        long currentTimeMillis = System.currentTimeMillis();
+
         // If it is completely expired, just send it to the network.
-        if (entry.isExpired()) {
+        if (entry.isExpired(currentTimeMillis)) {
             mRequest.addMarker("cache-hit-expired");
             mRequest.setCacheEntry(entry);
             if (!mWaitingRequestManager.maybeAddToWaitingRequests(mRequest)) {
@@ -281,15 +287,17 @@ private void handleEntry(final Entry entry, final Request<?> mRequest) {
         }
 
         // We have a cache hit; parse its data for delivery back to the request.
-        mBlockingExecutor.execute(new CacheParseTask<>(mRequest, entry));
+        mBlockingExecutor.execute(new CacheParseTask<>(mRequest, entry, currentTimeMillis));
     }
 
     private class CacheParseTask<T> extends RequestTask<T> {
         Cache.Entry entry;
+        long startTimeMillis;
 
-        CacheParseTask(Request<T> request, Cache.Entry entry) {
+        CacheParseTask(Request<T> request, Cache.Entry entry, long startTimeMillis) {
             super(request);
             this.entry = entry;
+            this.startTimeMillis = startTimeMillis;
         }
 
         @Override
@@ -305,7 +313,7 @@ public void run() {
                                     entry.allResponseHeaders));
             mRequest.addMarker("cache-hit-parsed");
 
-            if (!entry.refreshNeeded()) {
+            if (!entry.refreshNeeded(startTimeMillis)) {
                 // Completely unexpired cache hit. Just deliver the response.
                 getResponseDelivery().postResponse(mRequest, response);
             } else {
diff --git a/src/main/java/com/android/volley/Cache.java b/src/main/java/com/android/volley/Cache.java
index b8908ac8..7348d0f3 100644
--- a/src/main/java/com/android/volley/Cache.java
+++ b/src/main/java/com/android/volley/Cache.java
@@ -102,12 +102,20 @@ class Entry {
 
         /** True if the entry is expired. */
         public boolean isExpired() {
-            return this.ttl < System.currentTimeMillis();
+            return isExpired(System.currentTimeMillis());
+        }
+
+        boolean isExpired(long currentTimeMillis) {
+            return this.ttl < currentTimeMillis;
         }
 
         /** True if a refresh is needed from the original data source. */
         public boolean refreshNeeded() {
-            return this.softTtl < System.currentTimeMillis();
+            return refreshNeeded(System.currentTimeMillis());
+        }
+
+        boolean refreshNeeded(long currentTimeMillis) {
+            return this.softTtl < currentTimeMillis;
         }
     }
 }
diff --git a/src/main/java/com/android/volley/CacheDispatcher.java b/src/main/java/com/android/volley/CacheDispatcher.java
index 1bfc0ea5..4443143d 100644
--- a/src/main/java/com/android/volley/CacheDispatcher.java
+++ b/src/main/java/com/android/volley/CacheDispatcher.java
@@ -138,8 +138,14 @@ void processRequest(final Request<?> request) throws InterruptedException {
                 return;
             }
 
+            // Use a single instant to evaluate cache expiration. Otherwise, a cache entry with
+            // identical soft and hard TTL times may appear to be valid when checking isExpired but
+            // invalid upon checking refreshNeeded(), triggering a soft TTL refresh which should be
+            // impossible.
+            long currentTimeMillis = System.currentTimeMillis();
+
             // If it is completely expired, just send it to the network.
-            if (entry.isExpired()) {
+            if (entry.isExpired(currentTimeMillis)) {
                 request.addMarker("cache-hit-expired");
                 request.setCacheEntry(entry);
                 if (!mWaitingRequestManager.maybeAddToWaitingRequests(request)) {
@@ -164,7 +170,7 @@ void processRequest(final Request<?> request) throws InterruptedException {
                 }
                 return;
             }
-            if (!entry.refreshNeeded()) {
+            if (!entry.refreshNeeded(currentTimeMillis)) {
                 // Completely unexpired cache hit. Just deliver the response.
                 mDelivery.postResponse(request, response);
             } else {