This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9318

errors: catch urllib errors specifically instead of using safehasattr()
ClosedPublic

Authored by martinvonz on Nov 13 2020, 2:39 AM.

Details

Reviewers
Alphare
mharbison72
Group Reviewers
hg-reviewers
Commits
rHGae00e170f2d1: errors: catch urllib errors specifically instead of using safehasattr()
Summary

Before this patch, we would catch IOError and OSError and check if
the instance had a .code member (indicates HTTPError) or a
.reason member (indicates the more generic URLError). It seems to
me that can simply catch those exception specifically instead, so
that's what this code does. The existing code is from fbe8834923c5
(commands: report http exceptions nicely, 2005-06-17), so I suspect
it's just that there was no urllib2 (where URLError lives) back
then.

The old code mentioned SSLError in a comment. The new code does
*not* try to catch that. The documentation for ssl.SSLError says
that it has a .reason property, but `python -c 'import ssl;
print(dir(ssl.SSLError("foo", Exception("bar"))))` doesn't mention
that property on either Python 2 or Python 3 on my system. It also
seems that sslutil is pretty careful about converting ssl.SSLError
to error.Abort. It also is carefult to not assume that instances of
the exception have a .reason. So I at least don't want to catch
ssl.SSLError and handle it the same way as URLError because that
would likely result in a crash. I also wonder if we don't need to
handle it at all (because sslutil might handle all the cases). It's
now early in the release cycle, so perhaps we can just see how it
goes?

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

martinvonz created this revision.Nov 13 2020, 2:39 AM
Alphare accepted this revision.Nov 13 2020, 5:07 AM
mharbison72 added a subscriber: mharbison72.Nov 16 2020, 12:35 AM
mharbison72 added inline comments.
mercurial/scmutil.py
241

Don't we need to catch SSLError here too, since some of the code in this block deals with it? Otherwise it gets handled by the (IOError, OSError) block below, and (I guess?) gets re-raised.

martinvonz edited the summary of this revision. (Show Details)Nov 16 2020, 12:05 PM
martinvonz added inline comments.Nov 16 2020, 12:05 PM
mercurial/scmutil.py
241

Good question. I didn't even notice the mention of SSLError.

I wonder if the old code actually caught that. python -c 'import ssl; print(dir(ssl.SSLError("foo", Exception("bar"))))' doesn't list any reason property with either Python 2 or 3. mercurial/sslutil.py seems to try to convert SSLError to error.Abort.

I'm worried that if I try to catch SSLError here, it won't have a .reason and it'll crash instead. It's early in the cycle now, so maybe we should take the seemingly small risk of breaking things?

I've updated the commit message to include the above. Let me know what you think.

mharbison72 added inline comments.Nov 16 2020, 2:15 PM
mercurial/scmutil.py
241

I'm worried that if I try to catch SSLError here, it won't have a .reason and it'll crash instead.

I'm not sure what the try/catch immediately after this is trying to do here either. And since we technically support stuff earlier than 2.7.9, we probably can't just assume it exists. I wonder if it gets slapped on outside of the constructor.

https://docs.python.org/2/library/ssl.html#functions-constants-and-exceptions

It's early in the cycle now, so maybe we should take the seemingly small risk of breaking things?

I'm OK with that. I'm surprised that no tests changed if this is a problem, though admittedly IDK what this code was originally handling.

mharbison72 accepted this revision.Nov 17 2020, 12:55 AM
This revision is now accepted and ready to land.Nov 17 2020, 12:55 AM

Revision Contents
Changeset List

PathPackages
Mmercurial/scmutil.py (26 lines)

Diff 23519

mercurial/scmutil.py

return inst.status_code return inst.status_code
except ImportError as inst: except ImportError as inst:
ui.error(_(b"abort: %s!\n") % stringutil.forcebytestr(inst)) ui.error(_(b"abort: %s!\n") % stringutil.forcebytestr(inst))
m = stringutil.forcebytestr(inst).split()[-1] m = stringutil.forcebytestr(inst).split()[-1]
if m in b"mpatch bdiff".split(): if m in b"mpatch bdiff".split():
ui.error(_(b"(did you forget to compile extensions?)\n")) ui.error(_(b"(did you forget to compile extensions?)\n"))
elif m in b"zlib".split(): elif m in b"zlib".split():
ui.error(_(b"(is your Python install correct?)\n")) ui.error(_(b"(is your Python install correct?)\n"))
except (IOError, OSError) as inst: except util.urlerr.httperror as inst:
if util.safehasattr(inst, b"code"): # HTTPError
ui.error(_(b"abort: %s\n") % stringutil.forcebytestr(inst)) ui.error(_(b"abort: %s\n") % stringutil.forcebytestr(inst))
elif util.safehasattr(inst, b"reason"): # URLError or SSLError except util.urlerr.urlerror as inst:
mharbison72Unsubmitted
Not Done

Don't we need to catch SSLError here too, since some of the code in this block deals with it? Otherwise it gets handled by the (IOError, OSError) block below, and (I guess?) gets re-raised.

mharbison72: Don't we need to catch `SSLError` here too, since some of the code in this block deals with it?
martinvonzAuthorUnsubmitted
Not Done

Good question. I didn't even notice the mention of SSLError.

I wonder if the old code actually caught that. python -c 'import ssl; print(dir(ssl.SSLError("foo", Exception("bar"))))' doesn't list any reason property with either Python 2 or 3. mercurial/sslutil.py seems to try to convert SSLError to error.Abort.

I'm worried that if I try to catch SSLError here, it won't have a .reason and it'll crash instead. It's early in the cycle now, so maybe we should take the seemingly small risk of breaking things?

I've updated the commit message to include the above. Let me know what you think.

martinvonz: Good question. I didn't even notice the mention of `SSLError`. I wonder if the old code…
mharbison72Unsubmitted
Not Done

I'm worried that if I try to catch SSLError here, it won't have a .reason and it'll crash instead.

I'm not sure what the try/catch immediately after this is trying to do here either. And since we technically support stuff earlier than 2.7.9, we probably can't just assume it exists. I wonder if it gets slapped on outside of the constructor.

https://docs.python.org/2/library/ssl.html#functions-constants-and-exceptions

It's early in the cycle now, so maybe we should take the seemingly small risk of breaking things?

I'm OK with that. I'm surprised that no tests changed if this is a problem, though admittedly IDK what this code was originally handling.

mharbison72: > I'm worried that if I try to catch SSLError here, it won't have a .reason and it'll crash…
try: # usually it is in the form (errno, strerror) try: # usually it is in the form (errno, strerror)
reason = inst.reason.args[1] reason = inst.reason.args[1]
except (AttributeError, IndexError): except (AttributeError, IndexError):
# it might be anything, for example a string # it might be anything, for example a string
reason = inst.reason reason = inst.reason
if isinstance(reason, pycompat.unicode): if isinstance(reason, pycompat.unicode):
# SSLError of Python 2.7.9 contains a unicode # SSLError of Python 2.7.9 contains a unicode
reason = encoding.unitolocal(reason) reason = encoding.unitolocal(reason)
ui.error(_(b"abort: error: %s\n") % stringutil.forcebytestr(reason)) ui.error(_(b"abort: error: %s\n") % stringutil.forcebytestr(reason))
elif ( except (IOError, OSError) as inst:
if (
util.safehasattr(inst, b"args") util.safehasattr(inst, b"args")
and inst.args and inst.args
and inst.args[0] == errno.EPIPE and inst.args[0] == errno.EPIPE
): ):
pass pass
elif getattr(inst, "strerror", None): # common IOError or OSError elif getattr(inst, "strerror", None): # common IOError or OSError
if getattr(inst, "filename", None) is not None: if getattr(inst, "filename", None) is not None:
ui.error( ui.error(