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

Rate Limit Handling #2787

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Rate Limit Handling #2787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions Octokit/Helpers/ResponseHeaders.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
namespace Octokit
{
public static class ResponseHeaders
{
public const string RateLimitLimit = "x-ratelimit-limit";
public const string RateLimitRemaining = "x-ratelimit-remaining";
public const string RateLimitUsed = "x-ratelimit-used";
public const string RateLimitReset = "x-ratelimit-reset";
}
}
6 changes: 4 additions & 2 deletions Octokit/Http/Connection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,9 +783,11 @@ static Exception GetExceptionForUnauthorized(IResponse response)

static Exception GetExceptionForForbidden(IResponse response)
{
string body = response.Body as string ?? "";
var body = response.Body as string ?? "";

if (body.Contains("rate limit exceeded"))
if (body.Contains("rate limit exceeded")
|| (response.Headers.ContainsKey(ResponseHeaders.RateLimitRemaining)
&& response.Headers[ResponseHeaders.RateLimitRemaining] == "0"))
{
return new RateLimitExceededException(response);
}
Expand Down
24 changes: 23 additions & 1 deletion Octokit/Http/HttpClientAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,28 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, Can
var response = await _http.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false);
// Need to determine time on client computer as soon as possible.
var receivedTime = DateTimeOffset.Now;

if (response.StatusCode == HttpStatusCode.Forbidden
&& response.Headers.Contains(ResponseHeaders.RateLimitRemaining)
&& response.Headers.GetValues(ResponseHeaders.RateLimitRemaining).FirstOrDefault() == "0")
{
var nowInEpochSecondsUtc = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
var resetTimeInEpochSecondsUtc = long.Parse(response.Headers.GetValues(ResponseHeaders.RateLimitReset).First());

var timeToWait = resetTimeInEpochSecondsUtc > nowInEpochSecondsUtc
? TimeSpan.FromSeconds(resetTimeInEpochSecondsUtc - nowInEpochSecondsUtc)
: TimeSpan.Zero;

// Could rate limit refreshes be up to an hour in the future?
// Should we conditionally retry only if it's a short period in the future
if (timeToWait < TimeSpan.FromSeconds(30))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite a long time to wait in the case of microservices. Places where I use Octokit would have a request timeout in the region of a few seconds at most, so this would be redundant in those cases as the CancellationToken would always fire and prevent another attempt ever happening while masking the HTTP request exception from the 403(429?) with a timeout-related exception instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I do agree. From the documentation, unless I'm wrong, it sounded like rate limits reset hourly. In which case this functionality might not be particularly useful as it would very rarely be hit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah from my experience, once you hit a limit then you just have to wait, unless it's a secondary rate-limit - those reset much sooner (I don't know how often though, I think it's the order of minutes).

{
await Task.Delay(timeToWait, cancellationToken);

return await SendAsync(clonedRequest, cancellationToken);
}
}

// Since Properties are stored as objects, serialize to HTTP round-tripping string (Format: r)
// Resolution is limited to one-second, matching the resolution of the HTTP Date header
request.Properties[ApiInfoParser.ReceivedTimeHeaderName] =
Expand Down Expand Up @@ -251,7 +273,7 @@ public async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, Can

return response;
}

public static async Task<HttpRequestMessage> CloneHttpRequestMessageAsync(HttpRequestMessage oldRequest)
{
var newRequest = new HttpRequestMessage(oldRequest.Method, oldRequest.RequestUri);
Expand Down