Incorrect error raised when max retries are exceeded

[jared-duo]

Describe the bug

When testing retry logic a urllib3.exceptions.MaxRetryError is raised instead of a requests.exceptions.RetryError.

Additional context

No response

Version of responses

0.21.0

Steps to Reproduce

from http.server import SimpleHTTPRequestHandler, ThreadingHTTPServer
from multiprocessing import Process
from tempfile import TemporaryDirectory
from time import sleep

import pytest
import requests
import responses
from requests.adapters import HTTPAdapter, Retry
from requests.exceptions import RetryError
from responses.registries import OrderedRegistry

_PORT = 8000

_SESSION = requests.session()
_SESSION.mount("http://", HTTPAdapter(max_retries=Retry(1, status_forcelist=[404])))


class CustomException(Exception):
    ...


def get(url: str):
    """System-under-test, catches retry errors and raises custom error"""
    try:
        return _SESSION.get(url)
    except RetryError:
        raise CustomException(url)


@responses.activate(registry=OrderedRegistry)
def test_get_responses():
    """Uses responses - fast, but has a bug"""
    url = f"http://localhost:{_PORT}/does_not_exist"
    rsp1 = responses.get(url, body="Error", status=404)
    rsp2 = responses.get(url, body="Error", status=404)

    with pytest.raises(CustomException):
        get(url)


def test_get_local_server(server: Process):
    """Uses a local server - slow, but correct"""
    url = f"http://localhost:{_PORT}/does_not_exist"
    with pytest.raises(CustomException):
        get(url)


### Fixtures to run a local server which serves 404s for everything but the root path


@pytest.fixture
def server():
    proc = Process(target=_run_server)
    proc.start()
    # Wait for the server to start, otherwise we may get a NewConnectionError as the connections
    # will be refused.
    sleep(1)
    yield proc
    proc.terminate()


def _run_server():
    """Creates a server that serves an empty directory 404'ing anything but the root path"""
    with TemporaryDirectory() as tempd:
        with ThreadingHTTPServer(
            ("", _PORT), lambda *args: SimpleHTTPRequestHandler(*args, directory=tempd)
        ) as httpd:
            httpd.serve_forever()

Expected Result

Both tests pass. More specifically that responses follows the logic (original - v2.5.0, most recent version - v2.28.1) introduced in requests v2.5.0 (the version that introduced the ability to use urllib3's Retry object with HTTPAdapters ) that converts urllib3.exceptions.MaxRetryErrors into requests.exceptions exceptions, specifically ResponseErrors becoming RetryErrors.

Actual Result

The responses based test does not pass, as it raises urllib3.exceptions.MaxRetryError instead of a requests.exceptions.RetryError.

[beliaev-maksim]

we just reraise the error from the urllib3.Retry lib.

responses/responses/__init__.py

Line 1036 in 1db612d

raise

basically, exception must include reason attribute, to raise RetryError, which looks to be not the part of the urllib3.

where do they append the reason?

[jared-duo]

we just reraise the error from the urllib3.Retry lib.

Yup, that is the suspected bug/deviation from the behavior of the requests package

where do they append the reason?

It looks like that logic lives in the urllib3 library. Here is the basic logic in the Retry.increment method and here is the caller's logic in the HTTPConnectionPool.urlopen method.

It appears that any HTTPExceptions (e.g. non-200s listed in the status force list) turn into ResponseErrors and proxy and ssl errors are more specific.

As linked before the requests package does just checks the reason that urllib3 attaches and does not attach a reason itself.

---

Perhaps a reasonable implementation would be to assume a ResponseError unless someone somehow adds a response of an SSLError or ConnectionError (not sure if you can do a responses.add(SSLError) or not, but maybe you can via the responses.add_callback(callback=lambda: raise SSLError())?

Though it could be reasonable to add support for HTTPException/ResponseErrors first since responses.add(method="GET", url="www.example.com", status=500) seems like basic functionality for the library and then maybe add a way for users to specify things like SSLErrors or ConnectionErrors later if it makes sense?

[1Mark]

I'm having this exact issue When testing retry logic a urllib3.exceptions.MaxRetryError is raised instead of a requests.exceptions.RetryError.

[beliaev-maksim]

@1Mark @jared-duo
please check #575

should fix the issue