Do not assume server time is in sync with local machine time on rate limit path #1972
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
holly-cummins
force-pushed
the
correct-rate-limit-wait-parsing
branch
5 times, most recently
from
0fd8887 to
2973e57
Compare
holly-cummins
force-pushed
the
correct-rate-limit-wait-parsing
branch
from
2973e57 to
50b967e
Compare
bitwiseman
reviewed
Oct 14, 2024
Comment on lines
+88
to
+103
| long parseWaitTime(GitHubConnectorResponse connectorResponse) { | ||
| String v = connectorResponse.header("X-RateLimit-Reset"); | ||
| if (v == null) | ||
| return Duration.ofMinutes(1).toMillis(); // can't tell, return 1 min | ||
|
|
||
| return Math.max(1000, Long.parseLong(v) * 1000 - System.currentTimeMillis()); | ||
| // 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(); | ||
| } | ||
| }; | ||
| return Math.max(MINIMUM_RATE_LIMIT_RETRY_MILLIS, (Long.parseLong(v) - now.toInstant().getEpochSecond()) * 1000); |
There was a problem hiding this comment.
This is now mostly the same as the abuse limit wait parse code you submitted. Would it make sense to extract this into a shared method? Perhaps even make it public so others could use it? Opened #1973 to track this.
bitwiseman
approved these changes
Oct 14, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1972 +/- ##
=========================================
Coverage 83.09% 83.10%
- Complexity 2329 2331 +2
=========================================
Files 231 231
Lines 7168 7172 +4
Branches 377 378 +1
=========================================
+ Hits 5956 5960 +4
Misses 974 974
Partials 238 238 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See discussion in #1909 and companion PR in #1971. On this test, I did try and cover the guard for missing dates with tests.
Description
Before submitting a PR:
@linkJavaDoc entries to the relevant documentation on https://docs.github.com/en/rest .mvn -D enable-ci clean install sitelocally. If this command doesn't succeed, your change will not pass CI.main. You will create your PR from that branch.When creating a PR: