Skip to content

Commit de55c0a

Browse files
rballa3rballa3
committed
feat Enabled read/write lock for GitHubSanityCachedValue [2172](hub4j#2172)
1 parent 9152b37 commit de55c0a

2 files changed

Lines changed: 130 additions & 4 deletions

File tree

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

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
import org.kohsuke.github.function.SupplierThrows;
44

55
import java.time.Instant;
6+
import java.util.concurrent.locks.Lock;
7+
import java.util.concurrent.locks.ReentrantReadWriteLock;
68
import java.util.function.Function;
79

810
/**
@@ -12,7 +14,10 @@ class GitHubSanityCachedValue<T> {
1214

1315
private long lastQueriedAtEpochSeconds = 0;
1416
private T lastResult = null;
15-
private final Object lock = new Object();
17+
// Allow concurrent readers while a refresh is not needed.
18+
private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
19+
private final Lock readLock = lock.readLock();
20+
private final Lock writeLock = lock.writeLock();
1621

1722
/**
1823
* 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<T> {
2631
* the exception thrown by the supplier if it fails.
2732
*/
2833
<E extends Throwable> T get(Function<T, Boolean> isExpired, SupplierThrows<T, E> query) throws E {
29-
synchronized (lock) {
30-
if (Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult)) {
34+
readLock.lock();
35+
try {
36+
boolean expired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult);
37+
if (!expired) {
38+
return lastResult;
39+
}
40+
} finally {
41+
readLock.unlock();
42+
}
43+
44+
writeLock.lock();
45+
try {
46+
boolean stillExpired = Instant.now().getEpochSecond() > lastQueriedAtEpochSeconds || isExpired.apply(lastResult);
47+
if (stillExpired) {
3148
lastResult = query.get();
3249
lastQueriedAtEpochSeconds = Instant.now().getEpochSecond();
3350
}
51+
return lastResult;
52+
} finally {
53+
writeLock.unlock();
3454
}
35-
return lastResult;
3655
}
3756

3857
/**

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

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
package org.kohsuke.github;
2+
3+
import org.junit.Test;
4+
5+
import java.time.Instant;
6+
import java.util.ArrayList;
7+
import java.util.Collections;
8+
import java.util.List;
9+
import java.util.concurrent.CountDownLatch;
10+
import java.util.concurrent.atomic.AtomicInteger;
11+
12+
import static org.hamcrest.CoreMatchers.equalTo;
13+
import static org.hamcrest.CoreMatchers.notNullValue;
14+
import static org.hamcrest.MatcherAssert.assertThat;
15+
16+
public class GitHubSanityCachedValueTest {
17+
18+
@Test
19+
public void cachesWithinSameSecond() throws Exception {
20+
alignToStartOfSecond();
21+
GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
22+
AtomicInteger calls = new AtomicInteger();
23+
24+
String first = cachedValue.get(() -> {
25+
calls.incrementAndGet();
26+
return "value";
27+
});
28+
String second = cachedValue.get(() -> {
29+
calls.incrementAndGet();
30+
return "value";
31+
});
32+
33+
assertThat(first, equalTo("value"));
34+
assertThat(second, equalTo("value"));
35+
assertThat(calls.get(), equalTo(1));
36+
}
37+
38+
@Test
39+
public void refreshesAfterOneSecond() throws Exception {
40+
GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
41+
AtomicInteger calls = new AtomicInteger();
42+
43+
String first = cachedValue.get(() -> {
44+
calls.incrementAndGet();
45+
return "value";
46+
});
47+
48+
Thread.sleep(1100);
49+
50+
String second = cachedValue.get(() -> {
51+
calls.incrementAndGet();
52+
return "value";
53+
});
54+
55+
assertThat(first, equalTo("value"));
56+
assertThat(second, equalTo("value"));
57+
assertThat(calls.get(), equalTo(2));
58+
}
59+
60+
@Test
61+
public void concurrentCallersOnlyRefreshOnce() throws Exception {
62+
alignToStartOfSecond();
63+
GitHubSanityCachedValue<String> cachedValue = new GitHubSanityCachedValue<>();
64+
AtomicInteger calls = new AtomicInteger();
65+
List<String> results = Collections.synchronizedList(new ArrayList<>());
66+
CountDownLatch ready = new CountDownLatch(5);
67+
CountDownLatch start = new CountDownLatch(1);
68+
CountDownLatch finished = new CountDownLatch(5);
69+
70+
for (int i = 0; i < 5; i++) {
71+
Thread thread = new Thread(() -> {
72+
try {
73+
ready.countDown();
74+
start.await();
75+
String value = cachedValue.get((result) -> result == null, () -> {
76+
calls.incrementAndGet();
77+
return "value";
78+
});
79+
results.add(value);
80+
} catch (Exception ignored) {
81+
results.add(null);
82+
} finally {
83+
finished.countDown();
84+
}
85+
});
86+
thread.start();
87+
}
88+
89+
ready.await();
90+
start.countDown();
91+
finished.await();
92+
93+
assertThat(calls.get(), equalTo(1));
94+
assertThat(results.size(), equalTo(5));
95+
for (String result : results) {
96+
assertThat(result, notNullValue());
97+
assertThat(result, equalTo("value"));
98+
}
99+
}
100+
101+
private static void alignToStartOfSecond() {
102+
while (Instant.now().getNano() > 100_000_000) {
103+
Thread.yield();
104+
}
105+
}
106+
}
107+

0 commit comments

Comments
 (0)