This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Download Raw Diff

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

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

martinvonz created this revision.Nov 13 2020, 2:39 AM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptNov 13 2020, 2:39 AM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

martinvonz added a child revision: D9309: errors: set detailed exit code to 100 for some remote errors.Nov 13 2020, 2:39 AM

martinvonz mentioned this in D9309: errors: set detailed exit code to 100 for some remote errors.Nov 13 2020, 2:59 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

martinvonz added a commit: rHGae00e170f2d1: errors: catch urllib errors specifically instead of using safehasattr().Nov 17 2020, 1:09 AM

Closed by commit rHGae00e170f2d1: errors: catch urllib errors specifically instead of using safehasattr() (authored by martinvonz). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/scmutil.py (26 lines)

Commit	Parents	Author	Summary	Date
2019a1d3c5fb	f6cb4b20689a	Martin von Zweigbergk		Nov 13 2020, 12:56 AM

Status	Author	Revision
Closed	martinvonz	D9331 dispatch: move some helper functions down into scmutil
Closed	martinvonz	D9330 errors: raise more specific errors from rewriteutil
Closed	martinvonz	D9329 errors: use InputError for some errors on `hg clone`
Closed	martinvonz	D9328 errors: raise InputError when given non-existent paths etc
Closed	martinvonz	D9327 errors: use InputError for errors about bad label names (tags etc)
Closed	martinvonz	D9326 errors: use InputError for errors about bad paths
Closed	martinvonz	D9313 destutil: raise more specific error when histedit.defaultrev is empty
Closed	martinvonz	D9312 errors: raise more specific errors when default remote not configured
Closed	martinvonz	D9311 errors: set detailed exit code to 30 for config errors
Closed	martinvonz	D9310 errors: introduce StateError and use it from commands and cmdutil
Closed	martinvonz	D9309 errors: set detailed exit code to 100 for some remote errors
Closed	martinvonz	D9318 errors: catch urllib errors specifically instead of using safehasattr()
Closed	martinvonz	D9308 errors: raise InputError in fancyopts

Diff 23488

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(

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	23488		Nov 13 2020, 2:39 AM	★	★
Diff 2	23519	rHGae00e170f2d1c2af5ca6a89269474398681004ef	Nov 13 2020, 12:56 AM	★	★