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

Use urllib3.util.retry from requests.packages [RHELDST-5671] #32

Merged

Conversation

MichalHaluza
Copy link
Contributor

Fixes
TypeError: unsupported operand type(s) for -=: 'Retry' and 'int'

Sufficiently new versions of requests module bundle its own urllib3,
which makes the whole Session-HTTPAdapter-Retry chain behave
unexpectedly. HTTPAdapter always calls Retry.from_int() regardless
of the max_retries parameter type. Retry.from_int() method either
creates a new instance of Retry or compares if the parameter already
is a Retry instance and returns it unchanged. The problem here is
that urllib3.util.retry.Retry is not the same as
requests.packages.urllib3.util.retry.Retry, so the non-bundled retry
object is wrapped into the bundled Retry object and is treated
as integer from then on, leading to the TypeError above.

RHEL-6 and RHEL-7 python-requests packages ship their own version of
requests.packages.urllib3.* which aliases the real top-level urllib3
module, so this code adjustment should work even on legacy systems
without urllib3 bundled with requests.

crungehottman
crungehottman previously approved these changes Aug 26, 2021
rohanpm
rohanpm previously approved these changes Aug 26, 2021
@rohanpm
Copy link
Member

rohanpm commented Aug 26, 2021

This repo hadn't received changes in a while and the CI was still wrongly pointing at travis-ci.org (now offline). I fixed it to point at travis-ci.com.

Could you please do an empty "git commit --amend" and push, or maybe close/reopen the PR, so that travis-ci.com starts a test run?

@MichalHaluza MichalHaluza force-pushed the requests-urllib-retry branch 3 times, most recently from f932ef6 to 4b70486 Compare August 27, 2021 07:24
@MichalHaluza
Copy link
Contributor Author

Hmm, so as it turns out Sufficiently new versions of requests module stopped bundling its own urllib3 again, but ship with a backwards-compatibility module which still provides requests.packages.urllib3.util.retry.Retry but it's once again an alias of urllib3.util.retry, however static analyzers such as pylint don't think it's a valid import, as seen in https://app.travis-ci.com/github/release-engineering/cdn-definitions/jobs/534128984#L217.

I'm compelled to leave requests.packages.urllib3.util.retry.Retry introduced by this PR as the most compatible variant, but annotating it with # pylint: disable=import-error feels weird. On the other hand, if this project targets python 3.8, then the original urllib3.util.retry is the correct one, but using it will cause the problems described in the linked ticket.

Fixes
TypeError: unsupported operand type(s) for -=: 'Retry' and 'int'

Requests module historically bundled its own urllib3,
which makes the whole Session-HTTPAdapter-Retry chain behave
unexpectedly. HTTPAdapter always calls Retry.from_int() regardless
of the max_retries parameter type. Retry.from_int() method either
creates a new instance of Retry or compares if the parameter already
is a Retry instance and returns it unchanged. The problem here is
that urllib3.util.retry.Retry is not the same as
requests.packages.urllib3.util.retry.Retry, so the non-bundled retry
object is wrapped into the bundled Retry object and is treated
as integer from then on, leading to the TypeError above.

Sufficiently new versions of requests module (>=2.16) stopped bundling
its own urllib3, but ship with a backwards-compatibility module which
still provides requests.packages.urllib3.util.retry.Retry which is
once again an alias of urllib3.util.retry

RHEL-6 and RHEL-7 python-requests packages ship their own version of
requests.packages.urllib3.* which aliases the real top-level urllib3
module, so this code adjustment should work even on legacy systems
without urllib3 bundled with requests.
@rohanpm
Copy link
Member

rohanpm commented Aug 30, 2021

A CI failure remains, I understand it's not introduced by this change but instead a pylint upgrade, but it'll have to be resolved nevertheless.

PEP-597 adds optional EncodingWarning when encoding parameter
to open() is omitted. In such case, the default locale-specific
encoding is used, which is acceptable in this case and also retains
backwards-compatibility with python 2, so only the pylint warning
needs to be suppressed.
@rohanpm rohanpm self-requested a review August 31, 2021 04:32
Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

Now we seem to have problems with coveralls.

@MichalHaluza
Copy link
Contributor Author

All checks have finally passed.
@rbikar: Please review.

@MichalHaluza MichalHaluza merged commit 728891c into release-engineering:master Sep 7, 2021
@MichalHaluza MichalHaluza deleted the requests-urllib-retry branch September 7, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants