Merge pull request #1134 from bitwiseman/task/atomic

Minimize locking for rate limit

Author: bitwiseman

src/main/java/org/kohsuke/github/GHRateLimit.java

@@ -12,6 +12,7 @@
 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 static class UnknownLimitRecord extends Record {
         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 final AtomicReference<UnknownLimitRecord> current = new AtomicReference<>(DEFAULT);
 
         /**
          * Create a new unknown record that resets at the specified time.
@@ -356,18 +357,20 @@ private UnknownLimitRecord(long resetEpochSeconds) {
             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;
         }
     }

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

@@ -14,6 +14,7 @@
 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 @@ GHRateLimit getRateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExce
     @Nonnull
     @Deprecated
     GHRateLimit lastRateLimit() {
-        synchronized (rateLimitLock) {
-            return rateLimit;
-        }
+        return rateLimit.get();
     }
 
     /**
@@ -277,12 +274,19 @@ GHRateLimit lastRateLimit() {
      */
     @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 @@ GHRateLimit rateLimit(@Nonnull RateLimitTarget rateLimitTarget) throws IOExcepti
      *            {@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;
     }
 
     /**