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 b82ae79e6c..db4b5c8aaa 100644 --- a/src/main/java/org/kohsuke/github/GitHubSanityCachedValue.java +++ b/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 { 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. */ - T get(Function isExpired, SupplierThrows query) throws E { - synchronized (lock) { - if (Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult)) { + T get(SupplierThrows 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. - */ - 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 new file mode 100644 index 0000000000..f4815cfc65 --- /dev/null +++ b/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 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 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(() -> { + 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 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 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)); + } +}