ConnectionResetError won't be identified as retryable

[akhenakh]

Some of our tests are failing with:

query_and_describe attempt 1 from PID 8 to GET /versions/us.shared_airspace failed with non-retryable ConnectionError: ('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer'))

#1310 is trying to fix an issue to catch ConnectionResetError so they can be retried.
But it seems it will miss this error anyway, it needs to also traverse exception.args - to find exceptions wrapped as constructor arguments, not just exceptions in the chain:

from http.client import RemoteDisconnected

def context_contains(exception, types) -> bool:
    if exception.__class__ in types:
        return True
    parent = getattr(exception, '__context__', None)
    if parent:
        return context_contains(parent, types)
    else:
        return False
# Simulate the actual error our the logs
inner = ConnectionResetError(104, 'Connection reset by peer')
e = ConnectionError('Connection aborted.', inner)
print(f'e.__context__: {e.__context__}')
print(f'e.args: {e.args}')
print(f'Retryable with code: {context_contains(e, (RemoteDisconnected, ConnectionResetError))}')

e.context: None
e.args: ('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer'))
Retryable with code: False

The ConnectionResetError is in args, not context.

from http.client import RemoteDisconnected

def context_contains_fixed(exception, types) -> bool:
    if exception.__class__ in types:
        return True
    parent = getattr(exception, '__context__', None)
    if parent:
        if context_contains_fixed(parent, types):
            return True
    # Also check args for wrapped exceptions
    for arg in getattr(exception, 'args', []):
        if isinstance(arg, BaseException):
            if context_contains_fixed(arg, types):
                return True
    return False
inner = ConnectionResetError(104, 'Connection reset by peer')
e = ConnectionError('Connection aborted.', inner)
print(f'e.__context__: {e.__context__}')
print(f'e.args: {e.args}')
print(f'Retryable with code: {context_contains_fixed(e, (RemoteDisconnected, ConnectionResetError))}')

e.context: None
e.args: ('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer'))
Retryable with code: True

[BenjaminPelletier]

@the-glu do you think you could take an initial look?

[the-glu]

I'm not 100% sure what is happening there, when testing on my side with a "real" error is does works:

Assuming a server with

with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
     s.bind(('0.0.0.0', 9999))
     s.listen()
     conn, addr = s.accept()
     conn.setsockopt(socket.SOL_SOCKET, socket.SO_LINGER, bytes([1, 0, 0, 0, 0, 0, 0, 0]))
     conn.close()

I'm getting the following with the original function:

try:
    requests.get("http://127.0.0.1:9999")
except Exception as e:
    print(f'e.__context__: {e.__context__}')
    print(f'e.args: {e.args}')
    print(f'Retryable with code: {context_contains(e, (RemoteDisconnected, ConnectionResetError))}')

e.__context__: ('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer'))
e.args: (ProtocolError('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer')),)
Retryable with code: True

Maybe there are different path to those connections reset? Do you have a way that I can reproduce that connection reset ?

[akhenakh]

I looked into it because of the error we received from the test conducted remotely, without knowing the current version was different.

Since we can't control the environment there, maybe it's a different version of requests which behaves differently 🤷‍♂️

[the-glu]

I looked into it because of the error we received from the test conducted remotely, without knowing the current version was different.

Since we can't control the environment there, maybe it's a different version of requests which behaves differently 🤷‍♂️

Python and requests' version are (in theory) locked to specific version, so that should hopefully not happens, but there may be an issue somewhere.

Witch commit are you using? (Latest release don't have #1310, it's only in main' branch) How to you run the tests? (Docker image?)

[akhenakh]

We are not the one conducting the tests, we are the one tested.

[akhenakh]

I think your test is different cause it is not reusing a connection from the pool since it did not complete a request first.

urllib3 raises ProtocolError requests catches it and raises ConnectionError

But in the real code the reset happens after the connection is established and pooled so the path to raise is different.

MaxRetryError is in context but ProtocolError (with ConnectionResetError) is in MaxRetryError.reason

the ProtocolError with ConnectionResetError inside is in MaxRetryError.reason, not in context.

[the-glu]

I'm not sure I understand: I don't think we use connection pooling in the test suite.

Can you please provide the version you do use? That would help a lot to narrow the issue.

If you have access to the tests report, it's recorded in the last two properties:

[akhenakh]

class UTMClientSession(requests.Session) requests.Session is creating a http connection pool

[the-glu]

True, but even in a reused pooled connection, the behavior is the same:

>>> import requests
>>> s = requests.Session()
>>> try:
...     s.get("http://127.0.0.1:9995")
... except Exception as e:
...     print(f'e.__context__: {e.__context__}')
...     print(f'e.args: {e.args}')
...     print(f'Retryable with code: {context_contains(e, (RemoteDisconnected, ConnectionResetError))}')
...     
<Response [200]>
>>> try:
...     s.get("http://127.0.0.1:9995")
... except Exception as e:
...     print(f'e.__context__: {e.__context__}')
...     print(f'e.args: {e.args}')
...     print(f'Retryable with code: {context_contains(e, (RemoteDisconnected, ConnectionResetError))}')
...     
e.__context__: ('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer'))
e.args: (ProtocolError('Connection aborted.', ConnectionResetError(104, 'Connection reset by peer')),)
Retryable with code: True

>>> with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
...     s.bind(('0.0.0.0', 9995))
...     s.listen()
...     conn, addr = s.accept()
...     print(conn.recv(1024))
...     conn.send(b"HTTP/1.1 200 OK\r\nConnection: keep-alive\r\nContent-Type: text/plain\r\nContent-Length: 2\r\n\r\nOK")
...     print(conn.recv(1024))
...     conn.setsockopt(socket.SOL_SOCKET, socket.SO_LINGER, bytes([1, 0, 0, 0, 0, 0, 0, 0]))
...     conn.close()
...     
b'GET / HTTP/1.1\r\nHost: 127.0.0.1:9995\r\nUser-Agent: python-requests/2.32.4\r\nAccept-Encoding: gzip, deflate, br\r\nAccept: */*\r\nConnection: keep-alive\r\n\r\n'
90
b'GET / HTTP/1.1\r\nHost: 127.0.0.1:9995\r\nUser-Agent: python-requests/2.32.4\r\nAccept-Encoding: gzip, deflate, br\r\nAccept: */*\r\nConnection: keep-alive\r\n\r\n'

[akhenakh]

Here is the version visible from the test:

Codebase version: interuss/monitoring/v0.25.0
Commit hash:

a8adabe

[the-glu]

Ok, so that normal, the fix from #1310 haven't been released in v0.25.0.

Can you switch to the main branch or request a new release (there is a template when you create an issue) to check if that indeed fix the issue in your case?

[akhenakh]

Yes we found out it was not running the right version but I was referring to this below because I'm not sure it will catch those:

https://github.com/urllib3/urllib3/blob/main/src/urllib3/connectionpool.py#L838-L839

This results in an exception object where .args is ('Connection aborted.', ConnectionResetError(...)).

Then the requests library wraps this ProtocolError inside a ConnectionError creating the nested structure:

https://github.com/psf/requests/blob/main/src/requests/adapters.py#L660-L661

it's wrapped twice (only checked on recent versions).