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.

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

Fix #6629: linkcheck: Handle rate-limiting #8467

Merged
merged 1 commit into from
Nov 25, 2020
Merged

Fix #6629: linkcheck: Handle rate-limiting #8467

merged 1 commit into from
Nov 25, 2020

Conversation

francoisfreitag
Copy link
Contributor

@francoisfreitag francoisfreitag commented Nov 21, 2020 •
edited

Feature or Bugfix

  • Feature

Purpose

Follow the Retry-After header if present, otherwise use an exponential
back-off.

Relates

Issue #6629
Issue #7388

@francoisfreitag
Copy link
Contributor Author

francoisfreitag commented Nov 21, 2020 •
edited

The Windows checks are failing on tests test_too_many_requests_user_retry_float ad test_too_many_requests_retry_exponential_backoff. I could not think of a way to meaningfully test the retrying other than actually sleep and measure the wait time. I am happy to try your suggestions.

I can increase the margin on Windows to accommodate for the seemingly longer time needed to start a thread, or skip the test on Windows. I would prefer not to remove these tests.

I ended up implementing a version with the Requests library, because of #8391 (comment). I prefer to avoid forcing the use of async when not absolutely mandatory. That way, the project can move on without async, until we decide the time is ripe.

I am happy to pursue the async way, I am convinced it will yield better performance and possibly simplify the implementation, but at the expense of:

tk0miya
tk0miya reviewed Nov 21, 2020
View reviewed changes
doc/usage/configuration.rst Outdated Show resolved Hide resolved
sphinx/builders/linkcheck.py Outdated Show resolved Hide resolved
sphinx/builders/linkcheck.py Outdated Show resolved Hide resolved
sphinx/builders/linkcheck.py Outdated Show resolved Hide resolved
tests/test_build_linkcheck.py Outdated Show resolved Hide resolved
@francoisfreitag
Copy link
Contributor Author

francoisfreitag commented Nov 22, 2020 •
edited

Thanks for the review!

I believe I addressed all feedback and the patch is ready for another look.
The test suite is now passing with the changes suggested in #8467 (comment).

@francoisfreitag francoisfreitag mentioned this pull request Nov 22, 2020
Closed
tk0miya
tk0miya approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

CHANGES Outdated Show resolved Hide resolved
doc/usage/configuration.rst Outdated Show resolved Hide resolved
@tk0miya tk0miya added type:enhancement enhance or introduce a new feature builder:linkcheck labels Nov 25, 2020
@tk0miya tk0miya added this to the 3.4.0 milestone Nov 25, 2020
@francoisfreitag
Fix sphinx-doc#6629: linkcheck: Handle rate-limiting
6b90a63 
Follow the Retry-After header if present, otherwise use an exponential
back-off.
@francoisfreitag
Copy link
Contributor Author

Rebased, applied suggestions (removed duplicate mention of linkcheck in the CHANGES) and squashed the commits.

@tk0miya tk0miya merged commit 5a0e123 into sphinx-doc:3.x Nov 25, 2020
@tk0miya
Copy link
Member

tk0miya commented Nov 25, 2020

Merged! Thank you for your great work!

@stsewd stsewd mentioned this pull request Nov 25, 2020
Closed
@tk0miya tk0miya mentioned this pull request Nov 25, 2020
Closed
@francoisfreitag francoisfreitag deleted the rate-limit branch January 27, 2021 18:48
tk0miya added a commit to tk0miya/sphinx that referenced this pull request Feb 6, 2021
@tk0miya
refactor: linkcheck: Remove next_check from Hyperlink object
53855f9 
To separate hyperlink itself and checking (intermediate) state, this
removes `next_check` attribute from Hyperlink object and add a new
named-tuple `CheckRequest`.  It was rejected idea on implementing
rate-limiting first (see sphinx-doc#8467).  But it's better way to separate
large builder module into small components and make whole of linkcheck
builder simple.

After this change, `Hyperlink` object represents a hyperlink on the
document.  It has a URI and location info (docname and lineno).
@tk0miya tk0miya mentioned this pull request Feb 6, 2021
Merged
tk0miya added a commit to tk0miya/sphinx that referenced this pull request Feb 6, 2021
@tk0miya
refactor: linkcheck: Remove next_check from Hyperlink object
cb1ee43 
To separate hyperlink itself and checking (intermediate) state, this
removes `next_check` attribute from Hyperlink object and add a new
named-tuple `CheckRequest`.  It was rejected idea on implementing
rate-limiting first (see sphinx-doc#8467).  But it's better way to separate
large builder module into small components and make whole of linkcheck
builder simple.

After this change, `Hyperlink` object represents a hyperlink on the
document.  It has a URI and location info (docname and lineno).
tk0miya added a commit to tk0miya/sphinx that referenced this pull request Feb 7, 2021
@tk0miya
refactor: linkcheck: Remove next_check from Hyperlink object
5c809e9 
To separate hyperlink itself and checking (intermediate) state, this
removes `next_check` attribute from Hyperlink object and add a new
named-tuple `CheckRequest`.  It was rejected idea on implementing
rate-limiting first (see sphinx-doc#8467).  But it's better way to separate
large builder module into small components and make whole of linkcheck
builder simple.

After this change, `Hyperlink` object represents a hyperlink on the
document.  It has a URI and location info (docname and lineno).
tk0miya added a commit to tk0miya/sphinx that referenced this pull request Feb 12, 2021
@tk0miya
refactor: linkcheck: Remove next_check from Hyperlink object
df5c795 
To separate hyperlink itself and checking (intermediate) state, this
removes `next_check` attribute from Hyperlink object and add a new
named-tuple `CheckRequest`.  It was rejected idea on implementing
rate-limiting first (see sphinx-doc#8467).  But it's better way to separate
large builder module into small components and make whole of linkcheck
builder simple.

After this change, `Hyperlink` object represents a hyperlink on the
document.  It has a URI and location info (docname and lineno).
tk0miya added a commit to tk0miya/sphinx that referenced this pull request Feb 12, 2021
@tk0miya
refactor: linkcheck: Remove next_check from Hyperlink object
8d4ba32 
To separate hyperlink itself and checking (intermediate) state, this
removes `next_check` attribute from Hyperlink object and add a new
named-tuple `CheckRequest`.  It was rejected idea on implementing
rate-limiting first (see sphinx-doc#8467).  But it's better way to separate
large builder module into small components and make whole of linkcheck
builder simple.

After this change, `Hyperlink` object represents a hyperlink on the
document.  It has a URI and location info (docname and lineno).
tk0miya added a commit to tk0miya/sphinx that referenced this pull request Feb 12, 2021
@tk0miya
refactor: linkcheck: Remove next_check from Hyperlink object
725f74f 
To separate hyperlink itself and checking (intermediate) state, this
removes `next_check` attribute from Hyperlink object and add a new
named-tuple `CheckRequest`.  It was rejected idea on implementing
rate-limiting first (see sphinx-doc#8467).  But it's better way to separate
large builder module into small components and make whole of linkcheck
builder simple.

After this change, `Hyperlink` object represents a hyperlink on the
document.  It has a URI and location info (docname and lineno).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tk0miya tk0miya tk0miya approved these changes

Assignees
No one assigned
Labels
builder:linkcheck type:enhancement enhance or introduce a new feature
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@francoisfreitag @tk0miya