From de55c0a1faf9cf01ed5e26780f7e011504c24eb3 Mon Sep 17 00:00:00 2001 From: rball11 Date: Thu, 19 Feb 2026 13:45:56 -0700 Subject: [PATCH 1/7] feat Enabled read/write lock for GitHubSanityCachedValue [2172](https://github.com/hub4j/github-api/issues/2172) --- .../github/GitHubSanityCachedValue.java | 27 ++++- .../github/GitHubSanityCachedValueTest.java | 107 ++++++++++++++++++ 2 files changed, 130 insertions(+), 4 deletions(-) create mode 100644 src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java diff --git a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java b/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java index b82ae79e6c..31475121a3 100644 --- a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java +++ b/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java @@ -3,6 +3,8 @@ import org.kohsuke.github.function.SupplierThrows; import java.time.Instant; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Function; /** @@ -12,7 +14,10 @@ class GitHubSanityCachedValue { 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. @@ -26,13 +31,27 @@ class GitHubSanityCachedValue { * the exception thrown by the supplier if it fails. */ T get(Function isExpired, SupplierThrows query) throws E { - synchronized (lock) { - if (Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult)) { + readLock.lock(); + try { + boolean expired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult); + if (!expired) { + return lastResult; + } + } finally { + readLock.unlock(); + } + + writeLock.lock(); + try { + boolean stillExpired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult); + if (stillExpired) { lastResult = query.get(); lastQueriedAtEpochSeconds = Instant.now().getEpochSecond(); } + return lastResult; + } finally { + writeLock.unlock(); } - return lastResult; } /** diff --git a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java new file mode 100644 index 0000000000..c1fe043eee --- /dev/null +++ b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java @@ -0,0 +1,107 @@ +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; + +public class GitHubSanityCachedValueTest { + + @Test + public void cachesWithinSameSecond() throws Exception { + alignToStartOfSecond(); + GitHubSanityCachedValue 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)); + } + + @Test + public void refreshesAfterOneSecond() throws Exception { + GitHubSanityCachedValue 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)); + } + + @Test + public void concurrentCallersOnlyRefreshOnce() throws Exception { + alignToStartOfSecond(); + GitHubSanityCachedValue cachedValue = new GitHubSanityCachedValue<>(); + AtomicInteger calls = new AtomicInteger(); + List 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((result) -> result == null, () -> { + 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")); + } + } + + private static void alignToStartOfSecond() { + while (Instant.now().getNano() > 100_000_000) { + Thread.yield(); + } + } +} + From cc772f459ffe41f461feb9922ccd65ca13c5ed7d Mon Sep 17 00:00:00 2001 From: rball11 Date: Fri, 27 Feb 2026 14:45:39 -0700 Subject: [PATCH 2/7] feat Enabled read/write lock for GitHubSanityCachedValue [2172](https://github.com/hub4j/github-api/issues/2172) --- .../github/GitHubSanityCachedValue.java | 1 - .../github/GitHubSanityCachedValueTest.java | 53 +++++++++---------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java b/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java index 31475121a3..acfdb094f1 100644 --- a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java +++ b/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java @@ -40,7 +40,6 @@ T get(Function isExpired, SupplierThrows } finally { readLock.unlock(); } - writeLock.lock(); try { boolean stillExpired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult); diff --git a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java index c1fe043eee..6facb6af21 100644 --- a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java +++ b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java @@ -15,6 +15,12 @@ public class GitHubSanityCachedValueTest { + private static void alignToStartOfSecond() { + while (Instant.now().getNano() > 100_000_000) { + Thread.yield(); + } + } + @Test public void cachesWithinSameSecond() throws Exception { alignToStartOfSecond(); @@ -35,28 +41,6 @@ public void cachesWithinSameSecond() throws Exception { assertThat(calls.get(), equalTo(1)); } - @Test - public void refreshesAfterOneSecond() throws Exception { - GitHubSanityCachedValue 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)); - } - @Test public void concurrentCallersOnlyRefreshOnce() throws Exception { alignToStartOfSecond(); @@ -98,10 +82,25 @@ public void concurrentCallersOnlyRefreshOnce() throws Exception { } } - private static void alignToStartOfSecond() { - while (Instant.now().getNano() > 100_000_000) { - Thread.yield(); - } + @Test + public void refreshesAfterOneSecond() throws Exception { + GitHubSanityCachedValue 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)); } } - From fe3f123f73671080f328ed0dd003375bd094e1af Mon Sep 17 00:00:00 2001 From: rball11 Date: Fri, 27 Feb 2026 14:47:55 -0700 Subject: [PATCH 3/7] feat Enabled read/write lock for GitHubSanityCachedValue [2172](https://github.com/hub4j/github-api/issues/2172) --- src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java b/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java index acfdb094f1..3e9eecd36d 100644 --- a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java +++ b/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java @@ -42,7 +42,8 @@ T get(Function isExpired, SupplierThrows } writeLock.lock(); try { - boolean stillExpired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult); + boolean stillExpired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds + || isExpired.apply(lastResult); if (stillExpired) { lastResult = query.get(); lastQueriedAtEpochSeconds = Instant.now().getEpochSecond(); From 2700ce7efaa00672bf2a4bf60b53808e133e7a3f Mon Sep 17 00:00:00 2001 From: rball11 Date: Fri, 27 Feb 2026 14:55:47 -0700 Subject: [PATCH 4/7] feat Enabled read/write lock for GitHubSanityCachedValue [2172](https://github.com/hub4j/github-api/issues/2172) --- .../github/GitHubSanityCachedValueTest.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java index 6facb6af21..a851ccec1e 100644 --- a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java +++ b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java @@ -21,6 +21,13 @@ private static void alignToStartOfSecond() { } } + /** + * 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(); @@ -41,6 +48,13 @@ public void cachesWithinSameSecond() throws Exception { 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(); @@ -82,6 +96,13 @@ public void concurrentCallersOnlyRefreshOnce() throws Exception { } } + /** + * 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 cachedValue = new GitHubSanityCachedValue<>(); From 34741e4731d43a37414b8544bd9cfe1a1e3d43c9 Mon Sep 17 00:00:00 2001 From: rball11 Date: Fri, 27 Feb 2026 15:01:30 -0700 Subject: [PATCH 5/7] feat Enabled read/write lock for GitHubSanityCachedValue [2172](https://github.com/hub4j/github-api/issues/2172) --- .../java/org/kohsuke/github/GitHubSanityCachedValueTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java index a851ccec1e..d47f14d4e3 100644 --- a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java +++ b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java @@ -13,6 +13,9 @@ import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; +/** + * The Class GitHubSanityCachedValueTest. + */ public class GitHubSanityCachedValueTest { private static void alignToStartOfSecond() { From 7ae40734a5b1355e915b9ea07df0e795e851c306 Mon Sep 17 00:00:00 2001 From: Ravi Kumar Balla Date: Tue, 24 Mar 2026 10:56:45 -0700 Subject: [PATCH 6/7] Update GitHubSanityCachedValueTest.java added more test cases --- .../github/GitHubSanityCachedValueTest.java | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java index d47f14d4e3..9acb87e1b7 100644 --- a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java +++ b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java @@ -99,6 +99,42 @@ 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}. + * + * @throws Exception + * if the test fails + */ + @Test + public void isExpiredPredicateTriggersRefreshWithinSameSecond() throws Exception { + alignToStartOfSecond(); + GitHubSanityCachedValue 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"; + }); + + // 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)); + } + /** * Tests that the cache is refreshed after one second has elapsed, triggering a new query to retrieve the updated * value. From 7ef707d9fde0f57f2701c93f018b7f2d08582557 Mon Sep 17 00:00:00 2001 From: Liam Newman Date: Sat, 11 Jul 2026 01:36:44 -0700 Subject: [PATCH 7/7] Simplify SanityCache --- .../java/org/kohsuke/github/GitHubClient.java | 49 +++++++++---------- .../github/GitHubSanityCachedValue.java | 25 ++-------- .../github/GitHubSanityCachedValueTest.java | 37 +++++++------- 3 files changed, 43 insertions(+), 68 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GitHubClient.java b/src/main/java/org/kohsuke/github/GitHubClient.java index 7963de57e2..3b8c55c4ab 100644 --- a/src/main/java/org/kohsuke/github/GitHubClient.java +++ b/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); } diff --git a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java b/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java index 3e9eecd36d..db4b5c8aaa 100644 --- a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java +++ b/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,19 +21,16 @@ class GitHubSanityCachedValue { /** * 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. */ - T get(Function isExpired, SupplierThrows query) throws E { + T get(SupplierThrows 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 { @@ -42,9 +38,7 @@ T get(Function isExpired, SupplierThrows } 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 @@ T get(Function isExpired, SupplierThrows 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. - */ - T get(SupplierThrows query) throws E { - return get((value) -> Boolean.FALSE, query); - } } diff --git a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java b/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java index 9acb87e1b7..f4815cfc65 100644 --- a/src/test/java/org/kohsuke/github/GitHubSanityCachedValueTest.java +++ b/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,36 @@ 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 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, () -> { + // 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 "stale"; + 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, () -> { + cachedValue.get(() -> { + calls.incrementAndGet(); + return "expired-on-arrival"; + }); + cachedValue.get(() -> { calls.incrementAndGet(); - return "fresh"; + return "expired-on-arrival"; }); - assertThat(first, equalTo("stale")); - assertThat(second, equalTo("fresh")); - assertThat(calls.get(), equalTo(2)); + assertThat(calls.get(), equalTo(1)); } /**