-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
Codecov Report
@@ 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
|
@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! |
There was a problem hiding this 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 🎉
/// response in order to provide a best-effort estimate that is | ||
/// independant from eventual inaccuracies in the client's clock. | ||
/// </remarks> | ||
public TimeSpan GetRetryAfterTimeSpan() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Matching the style of other fields in the library Co-authored-by: Brendan Forster <brendan@github.com>
There was a problem hiding this 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!
release_notes: Add server time difference to |
TL;DR
ServerTimeDifference
of typeTimeSpan
to theApiInfo
class. An optional argument is added to the existing contructor.HttpClientAdapter
will inject a synthetic header namedX-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 theApiInfoParser
class.GetRetryAfterTimeSpan
toRateLimitExceededException
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
RateLimitExceededException
s. The exception instance as well as theApiInfo.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
is2020-06-01T12:00:00Z
. The client currently observes the time2020-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 in2020-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