Skip to content

Commit

Permalink
Do not assume server time is in sync with local machine time
Browse files Browse the repository at this point in the history
  • Loading branch information
holly-cummins committed Oct 9, 2024
1 parent 768c715 commit 218d4f1
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/main/java/org/kohsuke/github/GHRateLimit.java
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ && getRemaining() <= other.getRemaining())) {
return this;
} else if (!(other instanceof UnknownLimitRecord)) {
// If the above is not the case that means other has a later reset
// or the same resent and fewer requests remaining.
// or the same reset and fewer requests remaining.
// If the other record is not an unknown record, the other is more recent
return other;
} else if (this.isExpired() && !other.isExpired()) {
Expand Down
24 changes: 21 additions & 3 deletions src/main/java/org/kohsuke/github/GitHubAbuseLimitHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import java.io.IOException;
import java.io.InterruptedIOException;
import java.time.Duration;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoUnit;
Expand All @@ -23,6 +24,11 @@
*/
public abstract class GitHubAbuseLimitHandler extends GitHubConnectorResponseErrorHandler {

/**
* On a wait, even if the response suggests a very short wait, wait for a minimum duration.
*/
private static final int MINIMUM_ABUSE_RETRY_MILLIS = 1000;

/**
* Create default GitHubAbuseLimitHandler instance
*/
Expand Down Expand Up @@ -143,7 +149,7 @@ public void onError(GitHubConnectorResponse connectorResponse) throws IOExceptio
};

// If "Retry-After" missing, wait for unambiguously over one minute per GitHub guidance
static long DEFAULT_WAIT_MILLIS = 61 * 1000;
static long DEFAULT_WAIT_MILLIS = Duration.ofSeconds(61).toMillis();

/*
* Exposed for testability. Given an http response, find the retry-after header field and parse it as either a
Expand All @@ -156,11 +162,23 @@ static long parseWaitTime(GitHubConnectorResponse connectorResponse) {
}

try {
return Math.max(1000, Long.parseLong(v) * 1000);
return Math.max(MINIMUM_ABUSE_RETRY_MILLIS, Duration.ofSeconds(Long.parseLong(v)).toMillis());
} catch (NumberFormatException nfe) {
// The retry-after header could be a number in seconds, or an http-date
// We know it was a date if we got a number format exception :)

// Don't use ZonedDateTime.now(), because the local and remote server times may not be in sync
// Instead, we can take advantage of the Date field in the response to see what time the remote server
// thinks it is
String dateField = connectorResponse.header("Date");
ZonedDateTime now;
if (dateField != null) {
now = ZonedDateTime.parse(dateField, DateTimeFormatter.RFC_1123_DATE_TIME);
} else {
now = ZonedDateTime.now();

Check warning on line 178 in src/main/java/org/kohsuke/github/GitHubAbuseLimitHandler.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/kohsuke/github/GitHubAbuseLimitHandler.java#L178

Added line #L178 was not covered by tests
}
ZonedDateTime zdt = ZonedDateTime.parse(v, DateTimeFormatter.RFC_1123_DATE_TIME);
return ChronoUnit.MILLIS.between(ZonedDateTime.now(), zdt);
return Math.max(MINIMUM_ABUSE_RETRY_MILLIS, ChronoUnit.MILLIS.between(now, zdt));
}
}

Expand Down
3 changes: 1 addition & 2 deletions src/test/java/org/kohsuke/github/AbuseLimitHandlerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -481,9 +481,8 @@ public void onError(@NotNull GitHubConnectorResponse connectorResponse) throws I
"You have exceeded a secondary rate limit. Please wait a few minutes before you try again");

long waitTime = parseWaitTime(connectorResponse);
// The exact value here will depend on when the test is run
assertThat(waitTime, Matchers.lessThan(GitHubAbuseLimitHandler.DEFAULT_WAIT_MILLIS));
assertThat(waitTime, Matchers.greaterThan(3 * 1000l));
assertThat(waitTime, equalTo(8 * 1000l));

GitHubAbuseLimitHandler.WAIT.onError(connectorResponse);
}
Expand Down

0 comments on commit 218d4f1

Please sign in to comment.