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

Bugfix: enable retry for requests #5400

Merged
merged 2 commits into from Jun 28, 2019

Conversation

Projects
None yet
4 participants
@SSE4
Copy link
Contributor

commented Jun 25, 2019

Changelog: Bugfix: enable retry for requests
Docs: omit
@tags: svn, slow

closes #5398

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

- enable retry for requests
Signed-off-by: SSE4 <tomskside@gmail.com>

@SSE4 SSE4 force-pushed the SSE4:fix_connection_reset branch from 57ad24d to c0a70cf Jun 25, 2019

@uilianries
Copy link
Member

left a comment

LGTM

Show resolved Hide resolved conans/client/rest/conan_requester.py Outdated
self._http_requester = http_requester
else:
self._http_requester = requests.Session()
adapter = HTTPAdapter(max_retries=config.retry)

This comment has been minimized.

Copy link
@lasote

lasote Jun 26, 2019

Contributor

Not sure if this should be associated with config.retry or not. This retry is about connection retry, right? Should we create a new config for ìt? retry_connection? @memsharded

This comment has been minimized.

Copy link
@SSE4

SSE4 Jun 26, 2019

Author Contributor

yes, about connection

This comment has been minimized.

Copy link
@memsharded

memsharded Jun 27, 2019

Contributor

This sounds a bit extreme. So far there hasn't been any other report that indicate that this is needed, and with this change it seems that now absolutely every connection will retry the specified number of times (the default is None), because of a edge case on the Azure platform.

We need to make sure that this is not affecting anything else in other ways. Maybe we want to restrict the retries to some conditions?

s = requests.Session()
retries = Retry(total=5, backoff_factor=1, status_forcelist=[ ... ])
s.mount('http://', HTTPAdapter(max_retries=retries))

Or maybe this makes conan more resilient to network failures for most cases, which is a great thing? In this case, I am good with using also general.retry to configure this, maybe it doesn't deserve a dedicated config, as sounds like too fine grained and requiring too much explanation.

Just trying to foresee any possible problems and understand the change, not saying yes or not.

This comment has been minimized.

Copy link
@SSE4

SSE4 Jun 27, 2019

Author Contributor

according to the kennethreitz/requests#4506, this also happens in AWS clouds. given the fact Azure and AWS are two most popular and wide-spread cloud providers, I think it should be good to enable this, as we make conan more robust. as we haven't any reports, it means retries will never happen in most cases, right? so change will be safe in this case.
detecting Azure specifically in conan requires additional code, and I doubt it's useful to add it, as it adds unnecessary complexity: https://gallery.technet.microsoft.com/scriptcenter/Detect-Windows-Azure-aed06d51 (there are also variants to rely on IP ranges, but I think IPs may change over the time).
I also prefer to use just general.retry, as it fits Occam's razor, if we already have config for retires, why would we need to have one more config for retries?

This comment has been minimized.

Copy link
@memsharded

memsharded Jun 27, 2019

Contributor

Sounds good to me! Thanks!

@lasote lasote added this to the 1.17 milestone Jun 26, 2019

@lasote lasote merged commit adf2799 into conan-io:develop Jun 28, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.