PR#1203: Fix `is_conn_error()` for Python 3.3+ change to `socket.error` - koji

koji

#1203 Fix `is_conn_error()` for Python 3.3+ change to `socket.error`

Merged 4 years ago by mikem. Opened 4 years ago by adamwill.

adamwill/koji py3-conn-error into master

Fix `is_conn_error()` for Python 3.3+ change to `socket.error`

Adam Williamson • 4 years ago

d20928c

koji/__init__.py

file modified

+10 -9

		`@@ -1978,11 +1978,13 @@`

		`def is_conn_error(e):`
		`"""Determine if an error seems to be from a dropped connection"""`
		`- if isinstance(e, socket.error):`
		`- if getattr(e, 'errno', None) in (errno.ECONNRESET, errno.ECONNABORTED, errno.EPIPE):`
		`- return True`
		`- # else`
		`- return False`
		`+ # This is intended for the case where e is a socket error.`
		+ # as `socket.error` is just an alias for `OSError` in Python 3
		+ # there is no value to an `isinstance` check here; let's just
		`+ # assume that if the exception has an 'errno' and it's one of`
		`+ # these values, this is a connection error.`
		`+ if getattr(e, 'errno', None) in (errno.ECONNRESET, errno.ECONNABORTED, errno.EPIPE):`
		`+ return True`
		`if isinstance(e, six.moves.http_client.BadStatusLine):`
		`return True`
		`try:`
		`@@ -1994,10 +1996,9 @@`
		`e3 = getattr(e2, 'args', [None, None])[1]`
		`if isinstance(e3, six.moves.http_client.BadStatusLine):`
		`return True`
		`- if isinstance(e2, socket.error):`
		`- # same check as unwrapped socket error`
		`- if getattr(e, 'errno', None) in (errno.ECONNRESET, errno.ECONNABORTED, errno.EPIPE):`
		`- return True`
		`+ # same check as unwrapped socket error`
		`+ if getattr(e2, 'errno', None) in (errno.ECONNRESET, errno.ECONNABORTED, errno.EPIPE):`
		`+ return True`
		`except (TypeError, AttributeError):`
		`pass`
		`# otherwise`

adamwill commented 4 years ago

In Python 3.3+, socket.error is no longer a distinct exception.
It is - as the docs say - "A deprecated alias of OSError". This
means that this check:

isinstance(e, socket.error)

is effectively equivalent to:

isinstance(e, OSError)

This is a problem, because requests.exceptions.ConnectionError
(another exception type we handle later in is_conn_error()) is
a subclass of OSError - so on Python 3 we never actually reach
the block that's intended to handle that exception type. We hit
the isinstance(e, socket.error) block at the start instead, and
of course the exception doesn't have an errno attribute, so we
just return False at that point.

There are a few different ways we could fix this; this commit
does it by ditching the isinstance checks, and dropping the
shortcut return False bailout from the early block. We'll still
ultimately return False unless the error is actually one of the
other types we handle; it just costs a couple more isinstance
checks.

I don't think replacing the isinstance socket.error checks is
really necessary at all. We can just check for an errno attr,
and if there is one and it's one of the values we check for...
that seems safe enough to treat as a connection error.

This also changes the second check to be a check of e2, not
e - I'm fairly sure this is what's actually intended, and
the current code was a copy-paste error.

Signed-off-by: Adam Williamson awilliam@redhat.com

adamwill commented 4 years ago

Note, it's pleasantly simple to demonstrate the fundamental problem here for yourself:

[adamw@adam koji (master)]$ python3
Python 3.7.2 (default, Jan  2 2019, 11:32:51) 
[GCC 8.2.1 20181215 (Red Hat 8.2.1-6)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests.exceptions
>>> import socket
>>> err = requests.exceptions.ConnectionError()
>>> isinstance(err, socket.error)
True
>>> 
[adamw@adam koji (master)]$ python2
Python 2.7.15 (default, Sep 21 2018, 17:53:38) 
[GCC 8.2.1 20180905 (Red Hat 8.2.1-3)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests.exceptions
>>> import socket
>>> err = requests.exceptions.ConnectionError()
>>> isinstance(err, socket.error)
False
>>>

you can easily see that on Python 3, an instance of requests.exceptions.ConnectionError "is" an instance of socket.error; on Python 2, it isn't.

rebased onto d20928c

4 years ago