Merge pull request #2201 from ravikumar2026/feature/enableLock

feat Enabled read/write lock for GitHubSanityCachedValue

Author: bitwiseman

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

@@ -953,32 +953,29 @@ 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

@@ -3,7 +3,8 @@
 import org.kohsuke.github.function.SupplierThrows;
 
 import java.time.Instant;
-import java.util.function.Function;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
  * GitHubSanityCachedValue limits queries for a particular value to once per second.
@@ -12,39 +13,38 @@ class GitHubSanityCachedValue<T> {
 
     private long lastQueriedAtEpochSeconds = 0;
     private T lastResult = null;
-    private final Object lock = new Object();
+    // Allow concurrent readers while a refresh is not needed.
+    private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+    private final Lock readLock = lock.readLock();
+    private final Lock writeLock = lock.writeLock();
 
     /**
      * 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 {
-        synchronized (lock) {
-            if (Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult)) {
+    <E extends Throwable> T get(SupplierThrows<T, E> query) throws E {
+        readLock.lock();
+        try {
+            if (Instant.now().getEpochSecond() <= lastQueriedAtEpochSeconds) {
+                return lastResult;
+            }
+        } finally {
+            readLock.unlock();
+        }
+        writeLock.lock();
+        try {
+            if (Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds) {
                 lastResult = query.get();
                 lastQueriedAtEpochSeconds = Instant.now().getEpochSecond();
             }
+            return lastResult;
+        } finally {
+            writeLock.unlock();
         }
-        return lastResult;
-    }
-
-    /**
-     * 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

@@ -0,0 +1,163 @@
+package org.kohsuke.github;
+
+import org.junit.Test;
+
+import java.time.Instant;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * The Class GitHubSanityCachedValueTest.
+ */
+public class GitHubSanityCachedValueTest {
+
+    private static void alignToStartOfSecond() {
+        while (Instant.now().getNano() > 100_000_000) {
+            Thread.yield();
+        }
+    }
+
+    /**
+     * Tests that the cache returns the same value without querying again when accessed multiple times within the same
+     * second.
+     *
+     * @throws Exception
+     *             if the test fails
+     */
+    @Test
+    public void cachesWithinSameSecond() throws Exception {
+        alignToStartOfSecond();
+        GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
+        AtomicInteger calls = new AtomicInteger();
+
+        String first = cachedValue.get(() -> {
+            calls.incrementAndGet();
+            return "value";
+        });
+        String second = cachedValue.get(() -> {
+            calls.incrementAndGet();
+            return "value";
+        });
+
+        assertThat(first, equalTo("value"));
+        assertThat(second, equalTo("value"));
+        assertThat(calls.get(), equalTo(1));
+    }
+
+    /**
+     * Tests that multiple concurrent callers only trigger a single refresh of the cached value, preventing redundant
+     * queries.
+     *
+     * @throws Exception
+     *             if the test fails
+     */
+    @Test
+    public void concurrentCallersOnlyRefreshOnce() throws Exception {
+        alignToStartOfSecond();
+        GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
+        AtomicInteger calls = new AtomicInteger();
+        List<String> results = Collections.synchronizedList(new ArrayList<>());
+        CountDownLatch ready = new CountDownLatch(5);
+        CountDownLatch start = new CountDownLatch(1);
+        CountDownLatch finished = new CountDownLatch(5);
+
+        for (int i = 0; i < 5; i++) {
+            Thread thread = new Thread(() -> {
+                try {
+                    ready.countDown();
+                    start.await();
+                    String value = cachedValue.get(() -> {
+                        calls.incrementAndGet();
+                        return "value";
+                    });
+                    results.add(value);
+                } catch (Exception ignored) {
+                    results.add(null);
+                } finally {
+                    finished.countDown();
+                }
+            });
+            thread.start();
+        }
+
+        ready.await();
+        start.countDown();
+        finished.await();
+
+        assertThat(calls.get(), equalTo(1));
+        assertThat(results.size(), equalTo(5));
+        for (String result : results) {
+            assertThat(result, notNullValue());
+            assertThat(result, equalTo("value"));
+        }
+    }
+
+    /**
+     * 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 doesNotReQueryWhenResultIsAlreadyExpiredOnArrival() throws Exception {
+        alignToStartOfSecond();
+        GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
+        AtomicInteger calls = new AtomicInteger();
+
+        // 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";
+        });
+
+        assertThat(calls.get(), equalTo(1));
+    }
+
+    /**
+     * Tests that the cache is refreshed after one second has elapsed, triggering a new query to retrieve the updated
+     * value.
+     *
+     * @throws Exception
+     *             if the test fails
+     */
+    @Test
+    public void refreshesAfterOneSecond() throws Exception {
+        GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
+        AtomicInteger calls = new AtomicInteger();
+
+        String first = cachedValue.get(() -> {
+            calls.incrementAndGet();
+            return "value";
+        });
+
+        Thread.sleep(1100);
+
+        String second = cachedValue.get(() -> {
+            calls.incrementAndGet();
+            return "value";
+        });
+
+        assertThat(first, equalTo("value"));
+        assertThat(second, equalTo("value"));
+        assertThat(calls.get(), equalTo(2));
+    }
+}