Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose Rate Limit Headers #327

Merged
merged 3 commits into from
Jan 9, 2017
Merged

Expose Rate Limit Headers #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d03435
Expose Rate Limit Headers
stephenc Jan 5, 2017
dfea424
Expose the API url used by the GitHub
stephenc Jan 5, 2017
6fcddf4
Some usage patterns require more pro-active rate limit queries
stephenc Jan 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 70 additions & 18 deletions src/main/java/org/kohsuke/github/GitHub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -49,17 +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.
Expand Down Expand Up @@ -90,6 +90,10 @@ public class GitHub {

private HttpConnector connector = HttpConnector.DEFAULT;

private final Object headerRateLimitLock = new Object();
private GHRateLimit headerRateLimit = null;
private volatile GHRateLimit rateLimit = null;

/**
* Creates a client API root object.
*
Expand Down Expand Up @@ -254,6 +258,10 @@ public HttpConnector getConnector() {
return connector;
}

public String getApiUrl() {
return apiUrl;
}

/**
* Sets the custom connector used to make requests to GitHub.
*/
Expand Down Expand Up @@ -287,17 +295,61 @@ 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 );
Copy link

@xhumanoid xhumanoid Jan 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry, but what the crazy calculation do you do here?

hour - value in second
System.currentTimeMillis() - return millisecond

(millisecond + second)/ 1000L - strange unpredictable type of second, we can say this type of seconds as X.

By javadoc:
public Date(long date)
date the milliseconds since January 1, 1970, 00:00:00 GMT.

if we will use new Date( X ), then we can't predict what type of date we will receive.

@stephenc @KostyaSha @kohsuke
with this new code you broke reset time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! Read the comment in GHRateLimit... for crazy reasons that class stores the date in Date with seconds expressed as milliseconds.

Without this fixup the reset date will be incorrect as returned by GHRateLimit.getResetDate()

I have comments indicating that it be madness... but actually is the fix if least harm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack the adding of an hours seconds should be outside of the division, but this will still give a result in the future for GHE and as GHE does not have a rate limit...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read about "this is madness, storing the date as seconds in a Date object"
but very confused by the division.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/kohsuke/github-api/blob/4ee3086b6dcb46913f1341b2500a4790db5813d6/src/main/java/org/kohsuke/github/GHRateLimit.java#L31 is why we need to do the division... and that is madness... but fixing that madness is orthogonal to this enhancement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#329 will fix the algebra... but it is not strictly required as a limit of no limit expiring in 3.6 seconds or expiring in 1 h is still a limit of no limit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But without correcting for the 1000 fold factor then GHE would say it had a limit of 1000000 being refreshed on Tue Jun 20 01:41:44 IST 48997 which allows 21 API calls per year... so code that uses the rate limit to pre-throttle will run unnecessarily slow on GHE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With #329 code running against GHE and proactively throttling will make 277 API requests per second, while without #329 it might try and make 277777 requests per second as its upper limit... which is probably unatainable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or were you accessing the public field directly (oh how I love the way people make fields public in Java and make migration very difficult without breaking backwards compatibility)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is why we need to do the division... and that is madness... but fixing that madness is orthogonal to this enhancement

Yes, I agree with this, fixing this madness must doing not in this issue.

return rateLimit = r;
}
}

/*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 recently observed rate limit data or {@code null}.
*/
@CheckForNull
public GHRateLimit lastRateLimit() {
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;
}

/**
Expand Down
76 changes: 69 additions & 7 deletions src/main/java/org/kohsuke/github/Requester.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -42,23 +40,26 @@
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;
import java.util.List;
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.
Expand Down Expand Up @@ -281,6 +282,8 @@ private <T> T _to(String tailApiUrl, Class<T> type, T instance) throws IOExcepti
return result;
} catch (IOException e) {
handleApiError(e);
} finally {
noteRateLimit(tailApiUrl);
}
}
}
Expand All @@ -299,6 +302,8 @@ public int asHttpStatusCode(String tailApiUrl) throws IOException {
return uc.getResponseCode();
} catch (IOException e) {
handleApiError(e);
} finally {
noteRateLimit(tailApiUrl);
}
}
}
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -382,7 +440,7 @@ private boolean isMethodWithBody() {
}

try {
return new PagingIterator<T>(type, root.getApiURL(s.toString()));
return new PagingIterator<T>(type, tailApiUrl, root.getApiURL(s.toString()));
} catch (IOException e) {
throw new Error(e);
}
Expand All @@ -391,6 +449,7 @@ private boolean isMethodWithBody() {
class PagingIterator<T> implements Iterator<T> {

private final Class<T> type;
private final String tailApiUrl;

/**
* The next batch to be returned from {@link #next()}.
Expand All @@ -402,9 +461,10 @@ class PagingIterator<T> implements Iterator<T> {
*/
private URL url;

PagingIterator(Class<T> type, URL url) {
this.url = url;
PagingIterator(Class<T> type, String tailApiUrl, URL url) {
this.type = type;
this.tailApiUrl = tailApiUrl;
this.url = url;
}

public boolean hasNext() {
Expand Down Expand Up @@ -438,6 +498,8 @@ private void fetch() {
return;
} catch (IOException e) {
handleApiError(e);
} finally {
noteRateLimit(tailApiUrl);
}
}
} catch (IOException e) {
Expand Down