From de55c0a1faf9cf01ed5e26780f7e011504c24eb3 Mon Sep 17 00:00:00 2001 From: rball11 Date: Thu, 19 Feb 2026 13:45:56 -0700 Subject: [PATCH] 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(); + } + } +} +