Skip to content

Commit 7ef707d

Browse files
bitwisemanbitwiseman
committed
Simplify SanityCache
1 parent fefa40b commit 7ef707d

3 files changed

Lines changed: 43 additions & 68 deletions

File tree

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

Lines changed: 23 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -953,32 +953,29 @@ String getLogin() {
953953
GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {
954954
// Even when explicitly asking for rate limit, restrict to sane query frequency
955955
// return cached value if available
956-
GHRateLimit output = sanityCachedRateLimit.get(
957-
(currentValue) -> currentValue == null || currentValue.getRecord(rateLimitTarget).isExpired(),
958-
() -> {
959-
GHRateLimit result;
960-
try {
961-
final GitHubRequest request = GitHubRequest.newBuilder()
962-
.rateLimit(RateLimitTarget.NONE)
963-
.withApiUrl(getApiUrl())
964-
.withUrlPath("/rate_limit")
965-
.build();
966-
result = this
967-
.sendRequest(request,
968-
(connectorResponse) -> GitHubResponse.parseBody(connectorResponse,
969-
JsonRateLimit.class))
970-
.body().resources;
971-
} catch (FileNotFoundException e) {
972-
// For some versions of GitHub Enterprise, the rate_limit endpoint returns a 404.
973-
LOGGER.log(FINE, "(%s) /rate_limit returned 404 Not Found.", sendRequestTraceId.get());
974-
975-
// However some newer versions of GHE include rate limit header information
976-
// If the header info is missing and the endpoint returns 404, fill the rate limit
977-
// with unknown
978-
result = GHRateLimit.fromRecord(GHRateLimit.UnknownLimitRecord.current(), rateLimitTarget);
979-
}
980-
return result;
981-
});
956+
GHRateLimit output = sanityCachedRateLimit.get(() -> {
957+
GHRateLimit result;
958+
try {
959+
final GitHubRequest request = GitHubRequest.newBuilder()
960+
.rateLimit(RateLimitTarget.NONE)
961+
.withApiUrl(getApiUrl())
962+
.withUrlPath("/rate_limit")
963+
.build();
964+
result = this
965+
.sendRequest(request,
966+
(connectorResponse) -> GitHubResponse.parseBody(connectorResponse, JsonRateLimit.class))
967+
.body().resources;
968+
} catch (FileNotFoundException e) {
969+
// For some versions of GitHub Enterprise, the rate_limit endpoint returns a 404.
970+
LOGGER.log(FINE, "(%s) /rate_limit returned 404 Not Found.", sendRequestTraceId.get());
971+
972+
// However some newer versions of GHE include rate limit header information
973+
// If the header info is missing and the endpoint returns 404, fill the rate limit
974+
// with unknown
975+
result = GHRateLimit.fromRecord(GHRateLimit.UnknownLimitRecord.current(), rateLimitTarget);
976+
}
977+
return result;
978+
});
982979
return updateRateLimit(output);
983980
}
984981

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

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
import java.time.Instant;
66
import java.util.concurrent.locks.Lock;
77
import java.util.concurrent.locks.ReentrantReadWriteLock;
8-
import java.util.function.Function;
98

109
/**
1110
* GitHubSanityCachedValue limits queries for a particular value to once per second.
@@ -22,29 +21,24 @@ class GitHubSanityCachedValue<T> {
2221
/**
2322
* Gets the value from the cache or calls the supplier if the cache is empty or out of date.
2423
*
25-
* @param isExpired
26-
* a supplier that returns true if the cached value is no longer valid.
2724
* @param query
2825
* a supplier the returns an updated value. Only called if the cache is empty or out of date.
2926
* @return the value from the cache or the value returned from the supplier.
3027
* @throws E
3128
* the exception thrown by the supplier if it fails.
3229
*/
33-
<E extends Throwable> T get(Function<T, Boolean> isExpired, SupplierThrows<T, E> query) throws E {
30+
<E extends Throwable> T get(SupplierThrows<T, E> query) throws E {
3431
readLock.lock();
3532
try {
36-
boolean expired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult);
37-
if (!expired) {
33+
if (Instant.now().getEpochSecond() <= lastQueriedAtEpochSeconds) {
3834
return lastResult;
3935
}
4036
} finally {
4137
readLock.unlock();
4238
}
4339
writeLock.lock();
4440
try {
45-
boolean stillExpired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds
46-
|| isExpired.apply(lastResult);
47-
if (stillExpired) {
41+
if (Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds) {
4842
lastResult = query.get();
4943
lastQueriedAtEpochSeconds = Instant.now().getEpochSecond();
5044
}
@@ -53,17 +47,4 @@ <E extends Throwable> T get(Function<T, Boolean> isExpired, SupplierThrows<T, E>
5347
writeLock.unlock();
5448
}
5549
}
56-
57-
/**
58-
* Gets the value from the cache or calls the supplier if the cache is empty or out of date.
59-
*
60-
* @param query
61-
* a supplier the returns an updated value. Only called if the cache is empty or out of date.
62-
* @return the value from the cache or the value returned from the supplier.
63-
* @throws E
64-
* the exception thrown by the supplier if it fails.
65-
*/
66-
<E extends Throwable> T get(SupplierThrows<T, E> query) throws E {
67-
return get((value) -> Boolean.FALSE, query);
68-
}
6950
}

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

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public void concurrentCallersOnlyRefreshOnce() throws Exception {
7373
try {
7474
ready.countDown();
7575
start.await();
76-
String value = cachedValue.get((result) -> result == null, () -> {
76+
String value = cachedValue.get(() -> {
7777
calls.incrementAndGet();
7878
return "value";
7979
});
@@ -100,39 +100,36 @@ public void concurrentCallersOnlyRefreshOnce() throws Exception {
100100
}
101101

102102
/**
103-
* Tests that the {@code isExpired} predicate alone can force a cache refresh even when the cached value is still
104-
* current within the same second. This exercises the branch where the time-check condition ({@code A}) evaluates to
105-
* {@code false} but the {@code isExpired} predicate ({@code B}) evaluates to {@code true}, covering the
106-
* {@code A=false, B=true} path in both the read-lock check and the write-lock double-check inside
107-
* {@code GitHubSanityCachedValue}.
103+
* Tests that a result which is already expired on arrival — for example, the {@code GHRateLimit.UnknownLimitRecord}
104+
* returned when a GitHub Enterprise {@code /rate_limit} endpoint responds with 404 — is still held for one second.
105+
* Without the time-based TTL, re-checking expiry immediately after a refresh would cause every subsequent call to
106+
* re-query, creating a query storm.
108107
*
109108
* @throws Exception
110109
* if the test fails
111110
*/
112111
@Test
113-
public void isExpiredPredicateTriggersRefreshWithinSameSecond() throws Exception {
112+
public void doesNotReQueryWhenResultIsAlreadyExpiredOnArrival() throws Exception {
114113
alignToStartOfSecond();
115114
GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
116115
AtomicInteger calls = new AtomicInteger();
117116

118-
// Populate the cache within the current second using an isExpired predicate that never
119-
// expires on its own.
120-
String first = cachedValue.get(result -> false, () -> {
117+
// Supplier always returns a value that an isExpired() check would immediately reject,
118+
// e.g. GHRateLimit.UnknownLimitRecord when GitHub Enterprise returns 404 for /rate_limit.
119+
cachedValue.get(() -> {
121120
calls.incrementAndGet();
122-
return "stale";
121+
return "expired-on-arrival";
123122
});
124-
125-
// Within the same second, pass an isExpired predicate that always returns true. This forces
126-
// re-evaluation through the write lock even though the time has not elapsed, covering the
127-
// A=false, B=true branch in both compound conditions.
128-
String second = cachedValue.get(result -> true, () -> {
123+
cachedValue.get(() -> {
124+
calls.incrementAndGet();
125+
return "expired-on-arrival";
126+
});
127+
cachedValue.get(() -> {
129128
calls.incrementAndGet();
130-
return "fresh";
129+
return "expired-on-arrival";
131130
});
132131

133-
assertThat(first, equalTo("stale"));
134-
assertThat(second, equalTo("fresh"));
135-
assertThat(calls.get(), equalTo(2));
132+
assertThat(calls.get(), equalTo(1));
136133
}
137134

138135
/**

0 commit comments

Comments
 (0)