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.

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

🛠 fix: Adapter overwrite passed-in retry #6395

Closed
wants to merge 1 commit into from
Closed

🛠 fix: Adapter overwrite passed-in retry #6395

wants to merge 1 commit into from

Conversation

tomy0000000
Copy link

@tomy0000000 tomy0000000 commented Mar 27, 2023

According to the API reference of max_retries of requests.adapters.HTTPAdapter

The maximum number of retries each connection should attempt. Note, this applies only to failed DNS lookups, socket connections and connection timeouts, never to requests where data has made it to the server. By default, Requests does not retry failed connections. If you need granular control over the conditions under which we retry a request, import urllib3’s Retry class and pass that instead.

A widely used example can be found in this StackOverflow answer and is also under review in #6258.

However, the current source code does not really accept the passed-in Retry instance. Instead, it will silently discard and replace it with the default Retry instance.

This 2-line fix intends to fix the bug and restore its behavior to expect.

@sigmavirus24
Copy link
Contributor

You haven't provided evidence that this isn't working. The from_int classmethod returns the Retry instance if it's already a Retry

@tomy0000000
Copy link
Author

My mistake. Turns out I used the wrong scheme (http:// should be https://) when mounting in my previous test.

Closing this PR, but do look into #6258 and get it merged. Took me a while to get this snippet that should've been in official docs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants