Minimize locking for rate limit

Rather than locking to ensure ordered updates to rate limit, use AtomicReference. This reduces
the need for locking to only when rate limit has expired and we have to call getRateLimit().
This commit is contained in:
Liam Newman
2021-05-04 12:10:59 -07:00
parent 886887913c
commit f2a70a46ad
2 changed files with 27 additions and 26 deletions

View File

@@ -12,6 +12,7 @@ import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;
import java.util.Date;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicReference;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;
@@ -344,7 +345,7 @@ public class GHRateLimit {
private static final UnknownLimitRecord DEFAULT = new UnknownLimitRecord(Long.MIN_VALUE);
// The starting current UnknownLimitRecord is an expired record.
private static UnknownLimitRecord current = DEFAULT;
private static AtomicReference<UnknownLimitRecord> current = new AtomicReference<>(DEFAULT);
/**
* Create a new unknown record that resets at the specified time.
@@ -356,18 +357,20 @@ public class GHRateLimit {
super(unknownLimit, unknownRemaining, resetEpochSeconds);
}
static synchronized Record current() {
if (current.isExpired()) {
current = new UnknownLimitRecord(System.currentTimeMillis() / 1000L + unknownLimitResetSeconds);
static Record current() {
Record result = current.get();
if (result.isExpired()) {
current.set(new UnknownLimitRecord(System.currentTimeMillis() / 1000L + unknownLimitResetSeconds));
result = current.get();
}
return current;
return result;
}
/**
* Reset the current UnknownLimitRecord. For use during testing only.
*/
static synchronized void reset() {
current = DEFAULT;
static void reset() {
current.set(DEFAULT);
unknownLimitResetSeconds = defaultUnknownLimitResetSeconds;
}
}

View File

@@ -14,6 +14,7 @@ import java.time.Instant;
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoUnit;
import java.util.*;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;
import java.util.logging.Logger;
@@ -52,10 +53,8 @@ abstract class GitHubClient {
private HttpConnector connector;
private final Object rateLimitLock = new Object();
@Nonnull
private GHRateLimit rateLimit = GHRateLimit.DEFAULT;
private final AtomicReference<GHRateLimit> rateLimit = new AtomicReference<>(GHRateLimit.DEFAULT);
private static final Logger LOGGER = Logger.getLogger(GitHubClient.class.getName());
@@ -255,9 +254,7 @@ abstract class GitHubClient {
@Nonnull
@Deprecated
GHRateLimit lastRateLimit() {
synchronized (rateLimitLock) {
return rateLimit;
}
return rateLimit.get();
}
/**
@@ -277,12 +274,19 @@ abstract class GitHubClient {
*/
@Nonnull
GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOException {
synchronized (rateLimitLock) {
if (rateLimit.getRecord(rateLimitTarget).isExpired()) {
getRateLimit(rateLimitTarget);
GHRateLimit result = rateLimit.get();
// Most of the time rate limit is not expired, so try to avoid locking.
if (result.getRecord(rateLimitTarget).isExpired()) {
// if the rate limit is expired, synchronize to ensure
// only one call to getRateLimit() is made to refresh it.
synchronized (this) {
if (rateLimit.get().getRecord(rateLimitTarget).isExpired()) {
getRateLimit(rateLimitTarget);
}
}
return rateLimit;
result = rateLimit.get();
}
return result;
}
/**
@@ -295,15 +299,9 @@ abstract class GitHubClient {
* {@link GHRateLimit.Record} constructed from the response header information
*/
private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) {
synchronized (rateLimitLock) {
observed = rateLimit.getMergedRateLimit(observed);
if (rateLimit != observed) {
rateLimit = observed;
LOGGER.log(FINE, "Rate limit now: {0}", rateLimit);
}
return rateLimit;
}
GHRateLimit result = rateLimit.accumulateAndGet(observed, (current, x) -> current.getMergedRateLimit(x));
LOGGER.log(FINEST, "Rate limit now: {0}", rateLimit.get());
return result;
}
/**