Adjust GHRateLimit to system time instead of depending on synchronization #595

bitwiseman · 2019-11-07T01:55:25Z

Fixes #383
Related to #591

@martinvanzijl @PauloMigAlmeida
if you have a little time to look at this over, any feedback would be appreciated.

Summary

If the client time is out of sync with global time, GHRateLimit could report a resetDate that was completely wrong - accurate to global time, but disconnected from local system time. This wouldn't be so bad, but some clients use the resetDate to budget API calls. If they believe they have 10 hours until reset and only 4000 calls available, they effective lock up.

Further, public fields and having a "Date" field that is holding unconverted EpochSeconds just bothers me.

I deprecated the old public fields and added private fields with getters. I then added awareness of the Date header returned with the Http response. Using that we can generate calculatedSecondsUntilReset and the create the resetDate by adding that to System-time that his instance was created.

As a final step, I made it so rateLimit() will update automatically when then GHRateLimit instance expires.

Improves the way reset date is calculated - uses server date if possible. Fixes hub4j#383

PauloMigAlmeida · 2019-11-07T20:56:38Z

Cool! I will take a look at it during lunchtime :)

Sometimes adding tests finds broken product code. Sometimes it finds broken test environment. This was the latter. The wiremock templating returns dates in UTC format not RFC-1123 format.

Introduces hub4j#597 This does not appear to be a bug in github-api but in OkHttp 2.7.5 (and/or the Cache implementation in that version).

PauloMigAlmeida · 2019-11-09T12:42:13Z

Hi @bitwiseman,

If the client time is out of sync with global time, GHRateLimit could report a resetDate that was completely wrong

Good catch! That is a nice way of fixing the problem. 👍

[Re: refactoring/improvements you made]
The only thing I would possibly change is the way that the content from /rate_limit response is parsed.

Currently, we're using the JsonRateLimit containing only the rate attribute which has been deprecated. Ideally, we should add the newer JSON attributes listed at https://developer.github.com/v3/rate_limit/#response and refactor all references to the rate attribute to resources.core.

In case you want to go down that road, the Requester.noteRateLimit() method will have to be reworked a bit. https://github.com/github-api/github-api/blob/f28edbcf8fcb57a291aaa537bc57ea79c9bcda64/src/main/java/org/kohsuke/github/Requester.java#L354-L362

same here https://github.com/github-api/github-api/blob/f28edbcf8fcb57a291aaa537bc57ea79c9bcda64/src/main/java/org/kohsuke/github/GitHub.java#L329-L338

PauloMigAlmeida · 2019-11-09T12:50:40Z

src/main/java/org/kohsuke/github/GitHub.java

-                    || headerRateLimit.getResetDate().getTime() < observed.getResetDate().getTime()
-                    || headerRateLimit.remaining > observed.remaining) {
+                    || headerRateLimit.getRemaining() > observed.getRemaining()
+                    || headerRateLimit.getResetEpochSeconds() < observed.getResetEpochSeconds()) {


I must be missing something really simple but... I took a look at the git history and the issues used to discuss the original implementation (back in 2017) and I still don't get why we wouldn't update the headerRateLimit variable every time updateRateLimit method is invoked.

Would you mind sharing with me your understanding of that if statement and/or any idea of when this would be applicable?

Hm, good question. Ah, I remember now. Multi-threading.
If you have 10 threads all make queries and then all try to update the rate limit. We want to pick the one with the lowest remaining count, of if a reset occurs we want to pick the one with the later reset date.

I need to write a test that covers this and add comments to the code explaining.

ahhhhh, that makes sense :)

bitwiseman · 2019-11-09T22:19:04Z

@PauloMigAlmeida
Deprectacted: https://developer.github.com/v3/rate_limit/#deprecation-notice - good catch.

bitwiseman · 2019-11-10T07:34:10Z

@PauloMigAlmeida
So, I'm making some changes but it looks like making a new design that really includes the other rate limit info aside from core will be another bit of work. I think the change I've made is sufficient for this PR. And we can add an issue to do some further deprecation and redesign to expose the other limit information.

PauloMigAlmeida · 2019-11-10T10:13:01Z

I agree with you, all of that seems a bit too much for a single PR.

If you don't mind, after you finish this refactor I would love to try implementing the newer rate limits myself. That should be fun 😄

On the other hand, if you want to implement it, please count on me for reviewing it too

bitwiseman · 2019-11-11T21:51:25Z

src/main/java/org/kohsuke/github/GitHub.java

+        } else {
+            // records of the same type compare to each other as normal.
+            return current.getResetEpochSeconds() < candidate.getResetEpochSeconds()
+                || (current.getResetEpochSeconds() == candidate.getResetEpochSeconds() && current.getRemaining() > candidate.getRemaining());


@PauloMigAlmeida
This is the check that your question got me to fix.
A recorder with a later reset is for sure newer that one with an earlier one.
Only if they have the same reset should we take the one with the lower remaining count.
Previous implementation would take the lower remaining count candidate even if it had an earlier (older) reset date.
Thanks!

It makes sense! I like the train of thought you used for solving it.

bitwiseman · 2019-11-11T21:53:49Z

@PauloMigAlmeida
I've created a task #605 for exposing the new limit records and integrating the header update behavior.
Assigning to you. Enjoy!

Created GHRateLimit.Record Add the four rate limit records to GHRateLimit Moved getLimt(), getRemaining(), and so on to point to core record for ease of use. Fixed update check for header to not replace existing with older when remaining count is lower. NOTE: Did not expose records other than core and did not resolve header update behavior to respect non-core records.

bitwiseman · 2019-11-12T16:28:19Z

@PauloMigAlmeida How does this look to you now?

PauloMigAlmeida

Looks great! Can't wait to see it on the next version of the library 😃

PauloMigAlmeida · 2019-11-12T20:26:42Z

src/main/java/org/kohsuke/github/GitHub.java

+        } else {
+            // records of the same type compare to each other as normal.
+            return current.getResetEpochSeconds() < candidate.getResetEpochSeconds()
+                || (current.getResetEpochSeconds() == candidate.getResetEpochSeconds() && current.getRemaining() > candidate.getRemaining());


It makes sense! I like the train of thought you used for solving it.

bitwiseman added 2 commits November 6, 2019 11:11

Clean up GHRateLimit

7b7445b

Improves the way reset date is calculated - uses server date if possible. Fixes hub4j#383

Handle GHRateLimit expiration and server time inconsistency

1ecad70

bitwiseman force-pushed the issue/rate-limit branch from 647f06d to a6f2360 Compare November 7, 2019 01:59

Various clean up and fixes for GHRateLimit

9c80b7c

bitwiseman force-pushed the issue/rate-limit branch from a6f2360 to 9c80b7c Compare November 7, 2019 04:38

Merge remote-tracking branch 'github-api/master' into issue/rate-limit

bb3a492

Add tests for server time ahead and behind local time

37de3a4

Sometimes adding tests finds broken product code. Sometimes it finds broken test environment. This was the latter. The wiremock templating returns dates in UTC format not RFC-1123 format.

bitwiseman force-pushed the issue/rate-limit branch from 303960e to 37de3a4 Compare November 7, 2019 21:26

bitwiseman added 2 commits November 7, 2019 13:50

Update cache test Date and Limit values

319d2d1

Updated Last-Modified headers

57ac6e9

Introduces hub4j#597 This does not appear to be a bug in github-api but in OkHttp 2.7.5 (and/or the Cache implementation in that version).

PauloMigAlmeida reviewed Nov 9, 2019

View reviewed changes

PauloMigAlmeida mentioned this pull request Nov 9, 2019

Document this library's approach to GitHub API rate limit #601

Open

bitwiseman mentioned this pull request Nov 11, 2019

Integrate full rate limit information #605

Closed

bitwiseman force-pushed the issue/rate-limit branch from 426abda to 0d0b687 Compare November 11, 2019 21:47

bitwiseman commented Nov 11, 2019

View reviewed changes

bitwiseman force-pushed the issue/rate-limit branch from 0d0b687 to 03e9623 Compare November 11, 2019 22:29

PauloMigAlmeida approved these changes Nov 12, 2019

View reviewed changes

bitwiseman merged commit 7036423 into hub4j:master Nov 12, 2019

bitwiseman deleted the issue/rate-limit branch November 12, 2019 21:02

bitwiseman mentioned this pull request Dec 19, 2019

Fixes for related to github-api v1.100 jenkinsci/github-branch-source-plugin#260

Merged

jglick mentioned this pull request Mar 30, 2020

GHRepository.createCheckRun #753

Merged

4 tasks

bitwiseman mentioned this pull request Mar 30, 2020

Fix flaky tests #760

Closed

samrocketman mentioned this pull request Dec 19, 2022

Time sync issue for github api samrocketman/jervis#123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust GHRateLimit to system time instead of depending on synchronization #595

Adjust GHRateLimit to system time instead of depending on synchronization #595

bitwiseman commented Nov 7, 2019 •

edited

Loading

PauloMigAlmeida commented Nov 7, 2019

PauloMigAlmeida commented Nov 9, 2019

PauloMigAlmeida Nov 9, 2019 •

edited

Loading

bitwiseman Nov 9, 2019 •

edited

Loading

PauloMigAlmeida Nov 10, 2019

bitwiseman commented Nov 9, 2019

bitwiseman commented Nov 10, 2019 •

edited

Loading

PauloMigAlmeida commented Nov 10, 2019

bitwiseman Nov 11, 2019 •

edited

Loading

PauloMigAlmeida Nov 12, 2019

bitwiseman commented Nov 11, 2019

bitwiseman commented Nov 12, 2019 •

edited

Loading

PauloMigAlmeida left a comment

PauloMigAlmeida Nov 12, 2019

Adjust GHRateLimit to system time instead of depending on synchronization #595

Adjust GHRateLimit to system time instead of depending on synchronization #595

Conversation

bitwiseman commented Nov 7, 2019 • edited Loading

Summary

PauloMigAlmeida commented Nov 7, 2019

PauloMigAlmeida commented Nov 9, 2019

PauloMigAlmeida Nov 9, 2019 • edited Loading

Choose a reason for hiding this comment

bitwiseman Nov 9, 2019 • edited Loading

Choose a reason for hiding this comment

PauloMigAlmeida Nov 10, 2019

Choose a reason for hiding this comment

bitwiseman commented Nov 9, 2019

bitwiseman commented Nov 10, 2019 • edited Loading

PauloMigAlmeida commented Nov 10, 2019

bitwiseman Nov 11, 2019 • edited Loading

Choose a reason for hiding this comment

PauloMigAlmeida Nov 12, 2019

Choose a reason for hiding this comment

bitwiseman commented Nov 11, 2019

bitwiseman commented Nov 12, 2019 • edited Loading

PauloMigAlmeida left a comment

Choose a reason for hiding this comment

PauloMigAlmeida Nov 12, 2019

Choose a reason for hiding this comment

bitwiseman commented Nov 7, 2019 •

edited

Loading

PauloMigAlmeida Nov 9, 2019 •

edited

Loading

bitwiseman Nov 9, 2019 •

edited

Loading

bitwiseman commented Nov 10, 2019 •

edited

Loading

bitwiseman Nov 11, 2019 •

edited

Loading

bitwiseman commented Nov 12, 2019 •

edited

Loading