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.

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 {

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;
         }
     }
 }

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 {