From da619e2bde611efd623ec1ec397a3b8c25931966 Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Sun, 10 Mar 2024 09:08:15 -0700
Subject: [PATCH] rls: Fix time handling in CachingRlsLbClient

`getMinEvictionTime()` was fixed to make sure only deltas were used for
comparisons (`a < b` is broken; `a - b < 0` is okay). It had also
returned `0` by default, which was meaningless as there is no epoch for
`System.nanoTime()`. LinkedHashLruCache now passes the current time into
a few more functions since the implementations need it and it was
sometimes already available. This made it easier to make some classes
static.
---
 .../java/io/grpc/rls/CachingRlsLbClient.java  | 22 ++++++++-----------
 .../java/io/grpc/rls/LinkedHashLruCache.java  | 15 +++++--------
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java
index f1981bcc223..f3ef4f323f2 100644
--- a/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java
+++ b/rls/src/main/java/io/grpc/rls/CachingRlsLbClient.java
@@ -528,7 +528,7 @@ public String toString() {
   }
 
   /** Common cache entry data for {@link RlsAsyncLruCache}. */
-  abstract class CacheEntry {
+  abstract static class CacheEntry {
 
     protected final RouteLookupRequest request;
 
@@ -538,16 +538,12 @@ abstract class CacheEntry {
 
     abstract int getSizeBytes();
 
-    final boolean isExpired() {
-      return isExpired(ticker.read());
-    }
-
     abstract boolean isExpired(long now);
 
     abstract void cleanup();
 
-    protected long getMinEvictionTime() {
-      return 0L;
+    protected boolean isOldEnoughToBeEvicted(long now) {
+      return true;
     }
   }
 
@@ -649,8 +645,8 @@ boolean isStaled(long now) {
     }
 
     @Override
-    protected long getMinEvictionTime() {
-      return minEvictionTime;
+    protected boolean isOldEnoughToBeEvicted(long now) {
+      return minEvictionTime - now <= 0;
     }
 
     @Override
@@ -678,7 +674,7 @@ public String toString() {
    * Implementation of {@link CacheEntry} contains error. This entry will transition to pending
    * status when the backoff time is expired.
    */
-  private final class BackoffCacheEntry extends CacheEntry {
+  private static final class BackoffCacheEntry extends CacheEntry {
 
     private final Status status;
     private final BackoffPolicy backoffPolicy;
@@ -841,7 +837,7 @@ private static final class RlsAsyncLruCache
 
     @Override
     protected boolean isExpired(RouteLookupRequest key, CacheEntry value, long nowNanos) {
-      return value.isExpired();
+      return value.isExpired(nowNanos);
     }
 
     @Override
@@ -851,8 +847,8 @@ protected int estimateSizeOf(RouteLookupRequest key, CacheEntry value) {
 
     @Override
     protected boolean shouldInvalidateEldestEntry(
-        RouteLookupRequest eldestKey, CacheEntry eldestValue) {
-      if (eldestValue.getMinEvictionTime() > now()) {
+        RouteLookupRequest eldestKey, CacheEntry eldestValue, long now) {
+      if (!eldestValue.isOldEnoughToBeEvicted(now)) {
         return false;
       }
 
diff --git a/rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java b/rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java
index 5a4a2dab452..c1cbb28f29e 100644
--- a/rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java
+++ b/rls/src/main/java/io/grpc/rls/LinkedHashLruCache.java
@@ -85,8 +85,8 @@ protected boolean removeEldestEntry(Map.Entry<K, SizedValue> eldest) {
         // first, remove at most 1 expired entry
         boolean removed = cleanupExpiredEntries(1, ticker.read());
         // handles size based eviction if necessary no expired entry
-        boolean shouldRemove =
-            !removed && shouldInvalidateEldestEntry(eldest.getKey(), eldest.getValue().value);
+        boolean shouldRemove = !removed
+            && shouldInvalidateEldestEntry(eldest.getKey(), eldest.getValue().value, ticker.read());
         if (shouldRemove) {
           // remove entry by us to make sure lruIterator and cache is in sync
           LinkedHashLruCache.this.invalidate(eldest.getKey(), EvictionType.SIZE);
@@ -102,7 +102,7 @@ protected boolean removeEldestEntry(Map.Entry<K, SizedValue> eldest) {
    * that LruCache is access level and the eldest is determined by access pattern.
    */
   @SuppressWarnings("unused")
-  protected boolean shouldInvalidateEldestEntry(K eldestKey, V eldestValue) {
+  protected boolean shouldInvalidateEldestEntry(K eldestKey, V eldestValue, long now) {
     return true;
   }
 
@@ -236,10 +236,6 @@ public final List<V> values() {
     }
   }
 
-  protected long now() {
-    return ticker.read();
-  }
-
   /**
    * Cleans up cache if needed to fit into max size bytes by
    * removing expired entries and removing oldest entries by LRU order.
@@ -253,13 +249,14 @@ protected final boolean fitToLimit() {
         return false;
       }
       // cleanup expired entries
-      cleanupExpiredEntries(now());
+      long now = ticker.read();
+      cleanupExpiredEntries(now);
 
       // cleanup eldest entry until new size limit
       Iterator<Map.Entry<K, SizedValue>> lruIter = delegate.entrySet().iterator();
       while (lruIter.hasNext() && estimatedMaxSizeBytes < this.estimatedSizeBytes.get()) {
         Map.Entry<K, SizedValue> entry = lruIter.next();
-        if (!shouldInvalidateEldestEntry(entry.getKey(), entry.getValue().value)) {
+        if (!shouldInvalidateEldestEntry(entry.getKey(), entry.getValue().value, now)) {
           break; // Violates some constraint like minimum age so stop our cleanup
         }
         lruIter.remove();