#1203 Fix `is_conn_error()` for Python 3.3+ change to `socket.error`
Merged 2 months ago by mikem. Opened 2 months ago by adamwill.
adamwill/koji py3-conn-error  into  master

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

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

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

2 months ago

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

wow. That seems like a terrible way to handle a deprecation

Thanks for the patch :smile:

"wow. That seems like a terrible way to handle a deprecation"

yeah, I kinda thought the same!

Note, I've backported this for Fedora 28, 29 and Rawhide, and submitted updates for F28 and F29. Karma would be welcome, thanks!

Commit a224695 fixes this pull-request

Pull-Request has been merged by mikem

2 months ago