Page MenuHomePhabricator

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

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

Details

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.Fri, Nov 13, 2:39 AM
Alphare accepted this revision.Fri, Nov 13, 5:07 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)Mon, Nov 16, 12:05 PM
martinvonz added inline comments.Mon, Nov 16, 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.Mon, Nov 16, 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.Tue, Nov 17, 12:55 AM
This revision is now accepted and ready to land.Tue, Nov 17, 12:55 AM