From 9d03435aa14703c1a573371919e4151ad84a0848 Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Thu, 5 Jan 2017 09:21:32 +0000 Subject: [PATCH 1/3] Expose Rate Limit Headers Exposes the rate limit header responses so that consumers of the API can proactively tune their usage --- src/main/java/org/kohsuke/github/GitHub.java | 27 +++++++ .../java/org/kohsuke/github/Requester.java | 76 +++++++++++++++++-- 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java index 5d613fe985..fc258e8a02 100644 --- a/src/main/java/org/kohsuke/github/GitHub.java +++ b/src/main/java/org/kohsuke/github/GitHub.java @@ -50,6 +50,8 @@ import java.util.Set; import java.util.TimeZone; +import java.util.logging.Level; +import javax.annotation.CheckForNull; import org.apache.commons.codec.Charsets; import org.apache.commons.codec.binary.Base64; @@ -90,6 +92,9 @@ public class GitHub { private HttpConnector connector = HttpConnector.DEFAULT; + private final Object headerRateLimitLock = new Object(); + private GHRateLimit headerRateLimit = null; + /** * Creates a client API root object. * @@ -300,6 +305,28 @@ public GHRateLimit getRateLimit() throws IOException { } } + /*package*/ void updateRateLimit(@Nonnull GHRateLimit observed) { + synchronized (headerRateLimitLock) { + if (headerRateLimit == null + || headerRateLimit.getResetDate().getTime() < observed.getResetDate().getTime() + || headerRateLimit.remaining > observed.remaining) { + headerRateLimit = observed; + LOGGER.log(Level.INFO, "Rate limit now: {0}", headerRateLimit); + } + } + } + + /** + * Returns the most recently observed rate limit data or {@code null} if either there is no rate limit + * (for example GitHub Enterprise) or if no requests have been made. + * + * @return the most recentlt observed rate limit data or {@code null}. + */ + @CheckForNull + public GHRateLimit lastRateLimit() { + return headerRateLimit; + } + /** * Gets the {@link GHUser} that represents yourself. */ diff --git a/src/main/java/org/kohsuke/github/Requester.java b/src/main/java/org/kohsuke/github/Requester.java index 70c48940c0..21b3afd150 100644 --- a/src/main/java/org/kohsuke/github/Requester.java +++ b/src/main/java/org/kohsuke/github/Requester.java @@ -25,8 +25,6 @@ import com.fasterxml.jackson.databind.JsonMappingException; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; -import org.apache.commons.io.IOUtils; - import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; @@ -42,6 +40,7 @@ import java.net.URLEncoder; import java.util.ArrayList; import java.util.Collection; +import java.util.Date; import java.util.HashMap; import java.util.Iterator; import java.util.LinkedHashMap; @@ -49,16 +48,18 @@ import java.util.Locale; import java.util.Map; import java.util.NoSuchElementException; +import java.util.logging.Level; import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.zip.GZIPInputStream; - import javax.annotation.WillClose; +import org.apache.commons.io.IOUtils; +import org.apache.commons.lang.StringUtils; import static java.util.Arrays.asList; import static java.util.logging.Level.FINE; -import static org.kohsuke.github.GitHub.*; +import static org.kohsuke.github.GitHub.MAPPER; /** * A builder pattern for making HTTP call and parsing its output. @@ -281,6 +282,8 @@ private T _to(String tailApiUrl, Class type, T instance) throws IOExcepti return result; } catch (IOException e) { handleApiError(e); + } finally { + noteRateLimit(tailApiUrl); } } } @@ -299,6 +302,8 @@ public int asHttpStatusCode(String tailApiUrl) throws IOException { return uc.getResponseCode(); } catch (IOException e) { handleApiError(e); + } finally { + noteRateLimit(tailApiUrl); } } } @@ -313,6 +318,59 @@ public InputStream asStream(String tailApiUrl) throws IOException { return wrapStream(uc.getInputStream()); } catch (IOException e) { handleApiError(e); + } finally { + noteRateLimit(tailApiUrl); + } + } + } + + private void noteRateLimit(String tailApiUrl) { + if ("/rate_limit".equals(tailApiUrl)) { + // the rate_limit API is "free" + return; + } + if (tailApiUrl.startsWith("/search")) { + // the search API uses a different rate limit + return; + } + String limit = uc.getHeaderField("X-RateLimit-Limit"); + if (StringUtils.isBlank(limit)) { + // if we are missing a header, return fast + return; + } + String remaining = uc.getHeaderField("X-RateLimit-Remaining"); + if (StringUtils.isBlank(remaining)) { + // if we are missing a header, return fast + return; + } + String reset = uc.getHeaderField("X-RateLimit-Reset"); + if (StringUtils.isBlank(reset)) { + // if we are missing a header, return fast + return; + } + GHRateLimit observed = new GHRateLimit(); + try { + observed.limit = Integer.parseInt(limit); + } catch (NumberFormatException e) { + if (LOGGER.isLoggable(Level.FINEST)) { + LOGGER.log(Level.FINEST, "Malformed X-RateLimit-Limit header value " + limit, e); + } + return; + } + try { + observed.remaining = Integer.parseInt(remaining); + } catch (NumberFormatException e) { + if (LOGGER.isLoggable(Level.FINEST)) { + LOGGER.log(Level.FINEST, "Malformed X-RateLimit-Remaining header value " + remaining, e); + } + return; + } + try { + observed.reset = new Date(Long.parseLong(reset)); // this is madness, storing the date as seconds + root.updateRateLimit(observed); + } catch (NumberFormatException e) { + if (LOGGER.isLoggable(Level.FINEST)) { + LOGGER.log(Level.FINEST, "Malformed X-RateLimit-Reset header value " + reset, e); } } } @@ -382,7 +440,7 @@ private boolean isMethodWithBody() { } try { - return new PagingIterator(type, root.getApiURL(s.toString())); + return new PagingIterator(type, tailApiUrl, root.getApiURL(s.toString())); } catch (IOException e) { throw new Error(e); } @@ -391,6 +449,7 @@ private boolean isMethodWithBody() { class PagingIterator implements Iterator { private final Class type; + private final String tailApiUrl; /** * The next batch to be returned from {@link #next()}. @@ -402,9 +461,10 @@ class PagingIterator implements Iterator { */ private URL url; - PagingIterator(Class type, URL url) { - this.url = url; + PagingIterator(Class type, String tailApiUrl, URL url) { this.type = type; + this.tailApiUrl = tailApiUrl; + this.url = url; } public boolean hasNext() { @@ -438,6 +498,8 @@ private void fetch() { return; } catch (IOException e) { handleApiError(e); + } finally { + noteRateLimit(tailApiUrl); } } } catch (IOException e) { From dfea424b9412f4162ea8b94114c15f92897f5baf Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Thu, 5 Jan 2017 09:36:58 +0000 Subject: [PATCH 2/3] Expose the API url used by the GitHub --- src/main/java/org/kohsuke/github/GitHub.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java index fc258e8a02..4d9d04c089 100644 --- a/src/main/java/org/kohsuke/github/GitHub.java +++ b/src/main/java/org/kohsuke/github/GitHub.java @@ -259,6 +259,10 @@ public HttpConnector getConnector() { return connector; } + public String getApiUrl() { + return apiUrl; + } + /** * Sets the custom connector used to make requests to GitHub. */ From 6fcddf4a475e5b59b8e7644d44e857126635a056 Mon Sep 17 00:00:00 2001 From: Stephen Connolly Date: Thu, 5 Jan 2017 10:24:16 +0000 Subject: [PATCH 3/3] Some usage patterns require more pro-active rate limit queries --- src/main/java/org/kohsuke/github/GitHub.java | 61 +++++++++++++------- 1 file changed, 41 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java index 4d9d04c089..df9353e032 100644 --- a/src/main/java/org/kohsuke/github/GitHub.java +++ b/src/main/java/org/kohsuke/github/GitHub.java @@ -23,12 +23,10 @@ */ package org.kohsuke.github; -import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.ANY; -import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE; -import static java.util.logging.Level.FINE; -import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED; -import static org.kohsuke.github.Previews.DRAX; - +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.introspect.VisibilityChecker.Std; +import com.infradna.tool.bridge_method_injector.WithBridgeMethods; import java.io.ByteArrayInputStream; import java.io.FileNotFoundException; import java.io.IOException; @@ -49,19 +47,19 @@ import java.util.Map; import java.util.Set; import java.util.TimeZone; - +import java.util.concurrent.TimeUnit; import java.util.logging.Level; +import java.util.logging.Logger; import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; import org.apache.commons.codec.Charsets; import org.apache.commons.codec.binary.Base64; -import com.fasterxml.jackson.databind.DeserializationFeature; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.introspect.VisibilityChecker.Std; -import com.infradna.tool.bridge_method_injector.WithBridgeMethods; - -import javax.annotation.Nonnull; -import java.util.logging.Logger; +import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.ANY; +import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE; +import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED; +import static java.util.logging.Level.FINE; +import static org.kohsuke.github.Previews.DRAX; /** * Root of the GitHub API. @@ -94,6 +92,7 @@ public class GitHub { private final Object headerRateLimitLock = new Object(); private GHRateLimit headerRateLimit = null; + private volatile GHRateLimit rateLimit = null; /** * Creates a client API root object. @@ -296,16 +295,16 @@ public void setConnector(HttpConnector connector) { */ public GHRateLimit getRateLimit() throws IOException { try { - return retrieve().to("/rate_limit", JsonRateLimit.class).rate; + return rateLimit = retrieve().to("/rate_limit", JsonRateLimit.class).rate; } catch (FileNotFoundException e) { // GitHub Enterprise doesn't have the rate limit, so in that case // return some big number that's not too big. // see issue #78 GHRateLimit r = new GHRateLimit(); r.limit = r.remaining = 1000000; - long hours = 1000L * 60 * 60; - r.reset = new Date(System.currentTimeMillis() + 1 * hours ); - return r; + long hour = 60L * 60L; // this is madness, storing the date as seconds in a Date object + r.reset = new Date((System.currentTimeMillis() + hour) / 1000L ); + return rateLimit = r; } } @@ -324,11 +323,33 @@ public GHRateLimit getRateLimit() throws IOException { * Returns the most recently observed rate limit data or {@code null} if either there is no rate limit * (for example GitHub Enterprise) or if no requests have been made. * - * @return the most recentlt observed rate limit data or {@code null}. + * @return the most recently observed rate limit data or {@code null}. */ @CheckForNull public GHRateLimit lastRateLimit() { - return headerRateLimit; + synchronized (headerRateLimitLock) { + return headerRateLimit; + } + } + + /** + * Gets the current rate limit while trying not to actually make any remote requests unless absolutely necessary. + * + * @return the current rate limit data. + * @throws IOException if we couldn't get the current rate limit data. + */ + @Nonnull + public GHRateLimit rateLimit() throws IOException { + synchronized (headerRateLimitLock) { + if (headerRateLimit != null) { + return headerRateLimit; + } + } + GHRateLimit rateLimit = this.rateLimit; + if (rateLimit == null || rateLimit.getResetDate().getTime() < System.currentTimeMillis()) { + rateLimit = getRateLimit(); + } + return rateLimit; } /**