Simplify SanityCache

Author: bitwiseman

src/main/java/org/kohsuke/github/GitHubClient.java

@@ -953,32 +953,30 @@ String getLogin() {
     GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {
         // Even when explicitly asking for rate limit, restrict to sane query frequency
         // return cached value if available
-        GHRateLimit output = sanityCachedRateLimit.get(
-                (currentValue) -> currentValue == null || currentValue.getRecord(rateLimitTarget).isExpired(),
-                () -> {
-                    GHRateLimit result;
-                    try {
-                        final GitHubRequest request = GitHubRequest.newBuilder()
-                                .rateLimit(RateLimitTarget.NONE)
-                                .withApiUrl(getApiUrl())
-                                .withUrlPath("/rate_limit")
-                                .build();
-                        result = this
-                                .sendRequest(request,
-                                        (connectorResponse) -> GitHubResponse.parseBody(connectorResponse,
-                                                JsonRateLimit.class))
-                                .body().resources;
-                    } catch (FileNotFoundException e) {
-                        // For some versions of GitHub Enterprise, the rate_limit endpoint returns a 404.
-                        LOGGER.log(FINE, "(%s) /rate_limit returned 404 Not Found.", sendRequestTraceId.get());
-
-                        // However some newer versions of GHE include rate limit header information
-                        // If the header info is missing and the endpoint returns 404, fill the rate limit
-                        // with unknown
-                        result = GHRateLimit.fromRecord(GHRateLimit.UnknownLimitRecord.current(), rateLimitTarget);
-                    }
-                    return result;
-                });
+        GHRateLimit output = sanityCachedRateLimit.get(() -> {
+            GHRateLimit result;
+            try {
+                final GitHubRequest request = GitHubRequest.newBuilder()
+                        .rateLimit(RateLimitTarget.NONE)
+                        .withApiUrl(getApiUrl())
+                        .withUrlPath("/rate_limit")
+                        .build();
+                result = this
+                        .sendRequest(request,
+                                (connectorResponse) -> GitHubResponse.parseBody(connectorResponse,
+                                        JsonRateLimit.class))
+                        .body().resources;
+            } catch (FileNotFoundException e) {
+                // For some versions of GitHub Enterprise, the rate_limit endpoint returns a 404.
+                LOGGER.log(FINE, "(%s) /rate_limit returned 404 Not Found.", sendRequestTraceId.get());
+
+                // However some newer versions of GHE include rate limit header information
+                // If the header info is missing and the endpoint returns 404, fill the rate limit
+                // with unknown
+                result = GHRateLimit.fromRecord(GHRateLimit.UnknownLimitRecord.current(), rateLimitTarget);
+            }
+            return result;
+        });
         return updateRateLimit(output);
     }
 

src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java

@@ -5,7 +5,6 @@
 import java.time.Instant;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
-import java.util.function.Function;
 
 /**
  * GitHubSanityCachedValue limits queries for a particular value to once per second.
@@ -22,29 +21,24 @@ class GitHubSanityCachedValue<T> {
     /**
      * Gets the value from the cache or calls the supplier if the cache is empty or out of date.
      *
-     * @param isExpired
-     *            a supplier that returns true if the cached value is no longer valid.
      * @param query
      *            a supplier the returns an updated value. Only called if the cache is empty or out of date.
      * @return the value from the cache or the value returned from the supplier.
      * @throws E
      *             the exception thrown by the supplier if it fails.
      */
-    <E extends Throwable> T get(Function<T, Boolean> isExpired, SupplierThrows<T, E> query) throws E {
+    <E extends Throwable> T get(SupplierThrows<T, E> query) throws E {
         readLock.lock();
         try {
-            boolean expired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult);
-            if (!expired) {
+            if (Instant.now().getEpochSecond() <= lastQueriedAtEpochSeconds) {
                 return lastResult;
             }
         } finally {
             readLock.unlock();
         }
         writeLock.lock();
         try {
-            boolean stillExpired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds
-                    || isExpired.apply(lastResult);
-            if (stillExpired) {
+            if (Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds) {
                 lastResult = query.get();
                 lastQueriedAtEpochSeconds = Instant.now().getEpochSecond();
             }
@@ -53,17 +47,4 @@ <E extends Throwable> T get(Function<T, Boolean> isExpired, SupplierThrows<T, E>
             writeLock.unlock();
         }
     }
-
-    /**
-     * Gets the value from the cache or calls the supplier if the cache is empty or out of date.
-     *
-     * @param query
-     *            a supplier the returns an updated value. Only called if the cache is empty or out of date.
-     * @return the value from the cache or the value returned from the supplier.
-     * @throws E
-     *             the exception thrown by the supplier if it fails.
-     */
-    <E extends Throwable> T get(SupplierThrows<T, E> query) throws E {
-        return get((value) -> Boolean.FALSE, query);
-    }
 }

src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java

@@ -73,7 +73,7 @@ public void concurrentCallersOnlyRefreshOnce() throws Exception {
                 try {
                     ready.countDown();
                     start.await();
-                    String value = cachedValue.get((result) -> result == null, () -> {
+                    String value = cachedValue.get(() -> {
                         calls.incrementAndGet();
                         return "value";
                     });
@@ -100,39 +100,28 @@ public void concurrentCallersOnlyRefreshOnce() throws Exception {
     }
 
     /**
-     * Tests that the {@code isExpired} predicate alone can force a cache refresh even when the cached value is still
-     * current within the same second. This exercises the branch where the time-check condition ({@code A}) evaluates to
-     * {@code false} but the {@code isExpired} predicate ({@code B}) evaluates to {@code true}, covering the
-     * {@code A=false, B=true} path in both the read-lock check and the write-lock double-check inside
-     * {@code GitHubSanityCachedValue}.
+     * Tests that a result which is already expired on arrival — for example, the
+     * {@code GHRateLimit.UnknownLimitRecord} returned when a GitHub Enterprise {@code /rate_limit}
+     * endpoint responds with 404 — is still held for one second. Without the time-based TTL,
+     * re-checking expiry immediately after a refresh would cause every subsequent call to re-query,
+     * creating a query storm.
      *
      * @throws Exception
      *             if the test fails
      */
     @Test
-    public void isExpiredPredicateTriggersRefreshWithinSameSecond() throws Exception {
+    public void doesNotReQueryWhenResultIsAlreadyExpiredOnArrival() throws Exception {
         alignToStartOfSecond();
         GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
         AtomicInteger calls = new AtomicInteger();
 
-        // Populate the cache within the current second using an isExpired predicate that never
-        // expires on its own.
-        String first = cachedValue.get(result -> false, () -> {
-            calls.incrementAndGet();
-            return "stale";
-        });
+        // Supplier always returns a value that an isExpired() check would immediately reject,
+        // e.g. GHRateLimit.UnknownLimitRecord when GitHub Enterprise returns 404 for /rate_limit.
+        cachedValue.get(() -> { calls.incrementAndGet(); return "expired-on-arrival"; });
+        cachedValue.get(() -> { calls.incrementAndGet(); return "expired-on-arrival"; });
+        cachedValue.get(() -> { calls.incrementAndGet(); return "expired-on-arrival"; });
 
-        // Within the same second, pass an isExpired predicate that always returns true. This forces
-        // re-evaluation through the write lock even though the time has not elapsed, covering the
-        // A=false, B=true branch in both compound conditions.
-        String second = cachedValue.get(result -> true, () -> {
-            calls.incrementAndGet();
-            return "fresh";
-        });
-
-        assertThat(first, equalTo("stale"));
-        assertThat(second, equalTo("fresh"));
-        assertThat(calls.get(), equalTo(2));
+        assertThat(calls.get(), equalTo(1));
     }
 
     /**