Skip to content

fix: support RateLimit-Reset header #1895

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
Merged
merged 1 commit into from
Mar 10, 2022
Merged

fix: support RateLimit-Reset header #1895

merged 1 commit into from
Mar 10, 2022

Conversation

JohnVillalovos
Copy link
Member

@JohnVillalovos JohnVillalovos commented Feb 17, 2022 •
edited
Loading

Some endpoints are not returning the Retry-After header when
rate-limiting occurrs. In those cases use the RateLimit-Reset [1]
header, if available.

Closes: #1889

[1] https://docs.gitlab.com/ee/user/admin_area/settings/user_and_ip_rate_limits.html#response-headers

@JohnVillalovos JohnVillalovos requested a review from nejch February 17, 2022 00:42
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2022 •
edited by codecov bot
Loading

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.53%. Comparing base (85a734f) to head (4060146).
⚠️ Report is 1117 commits behind head on main.

Files with missing lines Patch % Lines
gitlab/client.py 50.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1895      +/-   ##
==========================================
- Coverage   92.55%   92.53%   -0.02%     
==========================================
  Files          78       78              
  Lines        4889     4891       +2     
==========================================
+ Hits         4525     4526       +1     
- Misses        364      365       +1
Flag Coverage Δ
cli_func_v4 81.68% <0.00%> (-0.04%) ⬇️
py_func_v4 80.20% <0.00%> (-0.04%) ⬇️
unit 83.41% <50.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
gitlab/client.py 90.36% <50.00%> (-0.20%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JohnVillalovos
fix: support RateLimit-Reset header
4060146 
Some endpoints are not returning the `Retry-After` header when
rate-limiting occurrs. In those cases use the `RateLimit-Reset` [1]
header, if available.

Closes: #1889

[1] https://docs.gitlab.com/ee/user/admin_area/settings/user_and_ip_rate_limits.html#response-headers
@nejch
Copy link
Member

nejch commented Feb 21, 2022

Thanks @JohnVillalovos! Would it make sense to add another case to

python-gitlab/tests/functional/api/test_gitlab.py

Lines 160 to 183 in 4cb7d92

def test_rate_limits(gl):
settings = gl.settings.get()
settings.throttle_authenticated_api_enabled = True
settings.throttle_authenticated_api_requests_per_period = 1
settings.throttle_authenticated_api_period_in_seconds = 3
settings.save()
projects = []
for i in range(0, 20):
projects.append(gl.projects.create({"name": f"{str(i)}ok"}))
with pytest.raises(gitlab.GitlabCreateError) as e:
for i in range(20, 40):
projects.append(
gl.projects.create(
{"name": f"{str(i)}shouldfail"}, obey_rate_limit=False
)
)
assert "Retry later" in str(e.value)
settings.throttle_authenticated_api_enabled = False
settings.save()
[project.delete() for project in projects]
with endpoints that don't return retry-after, e.g. user as mentioned in the issue? Although this might need 14.8 and not sure if it's enabled by default. Just wondering. https://docs.gitlab.com/ee/user/admin_area/settings/rate_limit_on_users_api.html#rate-limits-on-users-api

(If we do this, I would maybe turn the settings into a fixture to use in both tests, and do the cleanup after yield to make it more reusable, e.g.)

@pytest.fixture
def rate_limit_settings(gl):
    settings = gl.settings.get()
    settings.throttle_authenticated_api_enabled = True
    settings.throttle_authenticated_api_requests_per_period = 1
    settings.throttle_authenticated_api_period_in_seconds = 3
    settings.save()

    yield settings

    settings.throttle_authenticated_api_enabled = False
    settings.save()

PS: Interesting, looks like these headers are also useful to avoid 429s altogether, looking at go-gitlab with some fancy rate-limiting code: xanzy/go-gitlab@d8bb0b4

@nejch nejch mentioned this pull request Mar 10, 2022
Closed
@nejch nejch merged commit 114958e into main Mar 10, 2022
@nejch nejch deleted the jlvillal/rate-limit branch March 10, 2022 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nejch nejch nejch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Default value for max_retries is too short to "clear" all rate limit windows
3 participants
@JohnVillalovos @codecov-commenter @nejch