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.

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

Add Server Time Difference to ApiInfo #2196

Merged
merged 5 commits into from
Jun 7, 2020
Merged

Conversation

fredrikhr
Copy link
Contributor

TL;DR

  • Adds new read-only property ServerTimeDifference of type TimeSpan to the ApiInfo class. An optional argument is added to the existing contructor.
  • The HttpClientAdapter will inject a synthetic header named X-Octokit-ReceivedDate into the response headers providing an RFC1123 date-time-string specifying the time as obsoverved locally on the client when the response was received. The synthetic header name is added as a publically visible constant on the ApiInfoParser class.
  • Adds a new instance method GetRetryAfterTimeSpan to RateLimitExceededException that allows clients to obtain the amount of time to wait until the RateLimit-Window resets.

Description

In some scenarios it is useful to be able to calculate the appearent difference between the clock on a client computer and the clock on the server. While it is probably safe to assume that GitHub's server have a reasonable accurate clock, the same cannot be assumed for a client.

As part of the standard HTTP response headers, the GitHub API server responds with a Date response header that gives a best-effort estimate for the current server with a 1-second accuracy.
By storing the current time at the client as soon as possible after a response is received, these two times can be compared to calculate a reasonable estimate for the difference between the client and the server clocks.

The server time difference is useful whenever the client deals with point-in-time information provided by the server and needs to perfom calculations on that time.

An example of this is when dealing with RateLimitExceededExceptions. The exception instance as well as the ApiInfo.RateLimit instance for the last request both provide the time when the next Rate-Limit window starts. If a client does not have an accurate clock, it will not be able to calculate how long it needs to wait until the next rate-limit-window starts.

By exposing the server time difference in the ApiInfo class, clients can easily use that information in other scenarios as well.

Details

In order to keep fluctuations of response processing as small as possible, the received time must be captured as early as possible, preferably immediately after the HTTP headers have been received.

In order to keep the changes to existing types and methods as small as possible, the information is stored as a synthetic HTTP header in the response.

If for some reason the new synthetic header value does not exist, or if the server did not repost a server time (by omitting the Date header), the server time difference is assumed to be 0.

If both the client and the server have reasonably accurate clocks and agree on the current time, the server time difference will be close to or exactly equal to TimeSpan.Zero. In such cases, the only deviations are caused by network processing and propagation delays.

Example

For the following examples, both the server and the client are assumed to observe their local time in the UTC-timezone. All calculations use round-tripping time formats, automatically accounting for differences in time zones. Furthermore, this example assumes a network request to be instantaneous.

The current time as observed by the sever api.github.com is 2020-06-01T12:00:00Z. The client currently observes the time 2020-06-01T08:00:00Z (being 4 hours behind GitHub).

The client receives a rate-limit exceeded response GitHub states that the window will reset at 2020-06-01T13:00:00Z.

If the client uses its own clock, it will now have to assume that it needs to wait for 5 hours until the next request can be made.
Instead the client can extract the server time from the HTTP response (the Date reponse header) and compare that time against its own clock. The result of the comparison will result in the time-span value -04:00:00. This difference can now be added to the Reset time of the exception, resulting in 2020-06-01T09:00:00Z. Using that time, the client will correctly assume that it needs to wait 1 hour until the next requst can be made.

Code Example

var client = new GitHubClient();
while (true)
{
    try
    {
        var meta = await client.Miscellaneous.GetMetadata();
        break;
    }
    catch (RateLimitExceededException rateLimitExcept)
    {
        var retryAfter = rateLimitExcept.GetRetryAfterTimeSpan();
        await Task.Delay(retryAfter);
    }
}

@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #2196 into master will decrease coverage by 0.75%.
The diff coverage is 72.18%.

@@            Coverage Diff             @@
##           master    #2196      +/-   ##
==========================================
- Coverage   65.88%   65.12%   -0.76%     
==========================================
  Files         546      553       +7     
  Lines       14293    14426     +133     
  Branches      838      840       +2     
==========================================
- Hits         9417     9395      -22     
- Misses       4718     4756      +38     
- Partials      158      275     +117     
Impacted Files Coverage Δ
Octokit/Models/Request/NewProjectCard.cs 27.27% <0.00%> (ø)
Octokit/Models/Response/OrganizationHook.cs 0.00% <0.00%> (ø)
Octokit/Models/Response/Repository.cs 5.68% <0.00%> (-0.14%) ⬇️
Octokit/Models/Response/CheckRunRequestedAction.cs 42.85% <42.85%> (ø)
Octokit/Models/Request/EditOrganizationHook.cs 50.00% <50.00%> (ø)
Octokit/Models/Request/NewOrganizationHook.cs 63.63% <63.63%> (ø)
Octokit/Http/HttpClientAdapter.cs 76.61% <72.72%> (-1.65%) ⬇️
....Reactive/Clients/ObservableOrganizationsClient.cs 73.33% <75.00%> (-1.67%) ⬇️
...ctive/Clients/ObservableOrganizationHooksClient.cs 100.00% <100.00%> (ø)
Octokit/Clients/ChecksClient.cs 100.00% <100.00%> (ø)
... and 53 more

@shiftkey shiftkey self-assigned this Jun 1, 2020
@shiftkey
Copy link
Member

shiftkey commented Jun 1, 2020

@fredrikhr thanks for the contribution.

Given the detailed context behind it, it's gonna take me a few days to properly understand what we're trying to achieve here - I hope that's not a problem!

Copy link
Member

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

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

@fredrikhr I'm liking where this is going, and had a couple of small changes to make before we land this 🎉

Octokit/Exceptions/RateLimitExceededException.cs Outdated Show resolved Hide resolved
/// response in order to provide a best-effort estimate that is
/// independant from eventual inaccuracies in the client's clock.
/// </remarks>
public TimeSpan GetRetryAfterTimeSpan()
Copy link
Member

@shiftkey shiftkey Jun 7, 2020

Choose a reason for hiding this comment

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

Can you add a check to the existing HandlesMissingHeaderValues test inside Octokit.Tests/Exceptions/RateLimitExceededExceptionTests.cs, as well as a new test using the expected headers to ensure we're not erroring somehow when doing the parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shiftkey I added new Assertions to the existing test and added new unit tests for both the Exception and the ApiInfoParser class.

@shiftkey shiftkey removed their assignment Jun 7, 2020
Matching the style of other fields in the library

Co-authored-by: Brendan Forster <brendan@github.com>
@shiftkey shiftkey self-assigned this Jun 7, 2020
Copy link
Member

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @fredrikhr!

@shiftkey shiftkey merged commit af74ae8 into octokit:master Jun 7, 2020
@shiftkey
Copy link
Member

shiftkey commented Jun 7, 2020

release_notes: Add server time difference to ApiInfo API

@nickfloyd nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants