Using deprecated method_whitelist breaks upon actual retry

[vytas7]

Subject

method_whitelist parameter was apparently replaced by a racially-neutral (and actually easier to understand in this case) allowed_methods. The old parameter was deprecated.

However, it seems that continuing to use method_whitelist breaks the machinery upon actual retry, at least in some usage patterns.

It is easy for me to change to allowed_methods for new use, but I would love to support some older installations requiring urllib3==1.25.10. The new parameter name is not supported on 1.25.10.
There is also risk that existing code would break if urllib3 was upgraded in an existing environment.

Environment

>>> print("OS", platform.platform())
OS Linux-5.4.0-52-generic-x86_64-with-glibc2.29
>>> print("Python", platform.python_version())
Python 3.8.5
>>> print("urllib3", urllib3.__version__)
urllib3 1.26.2

Steps to Reproduce

Consider the following test case, named test.py:

import pytest
import urllib3
import urllib3.exceptions
import urllib3.util.retry


class CustomRetry(urllib3.util.retry.Retry):
    TEST_STATUS_FORCELIST = frozenset({502, 503, 504})
    TEST_ALLOWED_METHODS = frozenset({'HEAD', 'GET', 'TRACE'})

    def __init__(self, *args, **kwargs):
        kwargs['method_whitelist'] = self.TEST_ALLOWED_METHODS
        kwargs['status_forcelist'] = self.TEST_STATUS_FORCELIST
        super().__init__(*args, **kwargs)


def test_retry():
    retries = CustomRetry(total=5)
    http = urllib3.PoolManager(retries=retries)
    with pytest.raises(urllib3.exceptions.MaxRetryError):
        http.request('GET', 'https://httpstat.us/502')

The ubiquitous pytest is needed to run it:

pytest test.py

Expected Behavior

Test attached test case is expected to pass.

Actual Behavior

💥

<my venv>/lib/python3.8/site-packages/urllib3/request.py:74: in request
    return self.request_encode_url(
<my venv>/lib/python3.8/site-packages/urllib3/request.py:96: in request_encode_url
    return self.urlopen(method, url, **extra_kw)
<my venv>/lib/python3.8/site-packages/urllib3/poolmanager.py:375: in urlopen
    response = conn.urlopen(method, u.request_uri, **kw)
<my venv>/lib/python3.8/site-packages/urllib3/connectionpool.py:836: in urlopen
    retries = retries.increment(method, url, response=response, _pool=self)
<my venv>/lib/python3.8/site-packages/urllib3/util/retry.py:562: in increment
    new_retry = self.new(
<my venv>/lib/python3.8/site-packages/urllib3/util/retry.py:319: in new
    return type(self)(**params)
test.py:14: in __init__
    super().__init__(*args, **kwargs)

ValueError: Using both 'allowed_methods' and 'method_whitelist' together is not allowed. Instead only use 'allowed_methods'

[sethmlarson]

Thanks for reporting this, will investigate and get a fix out asap.

[hramezani]

I investigate the problem and here is my understanding:

You pass method_whitelist to Retry in CustomRetry.__init__ and internally Retry convert method_whitelist to allowed_methods. So, in the second try CustomRetry.__init__ adds method_whitelist in CustomRetry.__init__ since kwargs already has allowed_methods

A possible workaround for the test could be to avoid adding method_whitelist if allowed_methods exists in kwargs, like:

    def __init__(self, *args, **kwargs):
        if 'allowed_methods' not in kwargs:
            kwargs['method_whitelist'] = self.TEST_ALLOWED_METHODS
        kwargs['status_forcelist'] = self.TEST_STATUS_FORCELIST
        super().__init__(*args, **kwargs)

[vytas7]

Thanks for looking at this @hramezani ,
In the (closed source) code base which this test is loosely based upon, I worked it around along the lines of #2057 , i.e., I'm now using neither 'method_whitelist' nor 'allowed_methods' directly, but instead the key name is coming from a variable. Which is seeded according to hasattr(urllib3.util.Retry.DEFAULT, 'allowed_methods').

However, the biggest concern is a breaking change for existing code / deployed services which are not pinning urllib3, or are not pinning the minor version. This can also creep in unnoticed unless people are exercising an actual retry in their tests.

[sethmlarson]

We discussed this a bit on Discord, the decision was at least for now we'll wait to see if this problem is more widespread before taking action. Preferably we'd like to keep things as they are to push the ecosystem towards removing the deprecated options since these code paths will continue to fail in v2.

Leaving this issue open in case others have this same issue.

[sethmlarson]

Since this hasn't had any hits in over 2 years I'm going to close this as not a widespread issue. If this comes up again we can open a separate issue to handle it.

[Mustafahubs]

@vytas7 Thanks for sharing this. I downgraded the urllib3 to 1.26.2 in a separate virtual environment. Now it works fine for me.

[vytas7]

You're welcome @Mustafahubs. If I were to perform this change myself, I would have only added a deprecation warning, and made the actual breaking change in urllib3 2.x, but it seems that despite ~10 issues cross-referencing this one, that was not the chosen course of action. But regardless, I think it is now time to upgrade to 2.x and move on.