Patchwork [1,of,2] py3: protect from exceptions thrown by other meta-path finders

login
register
mail settings
Submitter Ludovic Chabant
Date April 19, 2019, 2:34 p.m.
Message ID <96a51193678400abf9d0.1555684453@devahi>
Download mbox | patch
Permalink /patch/39788/
State New
Headers show

Comments

Ludovic Chabant - April 19, 2019, 2:34 p.m.
# HG changeset patch
# User Ludovic Chabant <ludovic@chabant.com>
# Date 1555683918 0
#      Fri Apr 19 14:25:18 2019 +0000
# Branch stable
# Node ID 96a51193678400abf9d04af65e60a8dccf540cd7
# Parent  4a8d9ed864754837a185a642170cde24392f9abf
py3: protect from exceptions thrown by other meta-path finders
Yuya Nishihara - April 22, 2019, 11:21 p.m.
On Fri, 19 Apr 2019 14:34:13 +0000, Ludovic Chabant wrote:
> # HG changeset patch
> # User Ludovic Chabant <ludovic@chabant.com>
> # Date 1555683918 0
> #      Fri Apr 19 14:25:18 2019 +0000
> # Branch stable
> # Node ID 96a51193678400abf9d04af65e60a8dccf540cd7
> # Parent  4a8d9ed864754837a185a642170cde24392f9abf
> py3: protect from exceptions thrown by other meta-path finders
> 
> diff --git a/mercurial/__init__.py b/mercurial/__init__.py
> --- a/mercurial/__init__.py
> +++ b/mercurial/__init__.py
> @@ -54,9 +54,14 @@
>                  if finder == self:
>                      continue
>  
> -                spec = finder.find_spec(fullname, path, target=target)
> -                if spec:
> -                    break
> +                try:
> +                    spec = finder.find_spec(fullname, path, target=target)
> +                    if spec:
> +                        break
> +                except:
> +                    # Some finders don't implement `find_spec`, like
> +                    # `_SixMetaPathImporter`.
> +                    pass

Is there any list of exceptions that are known to be safely suppressed?
Catching AttributeError, TypeError, etc. seems bad.
Ludovic Chabant - April 23, 2019, 1:22 a.m.
>
> Is there any list of exceptions that are known to be safely
> suppressed? Catching AttributeError, TypeError, etc. seems bad.
>

In my case, it's just a missing method so AttributeError is what's being
raised I think (see
https://github.com/benjaminp/six/blob/master/six.py#L164).

My rationale here was that there could be any 3rd party finder in there,
so it might make sense to protect Mercurial from crashing because of
exceptions thrown from other packages.

However, now that I think about it, another maybe more correct way to
fix this specific bug might be to check for `find_spec` and, if missing,
call `find_module` instead. As per the Python docs (see
https://docs.python.org/3/library/importlib.html#importlib.abc.MetaPathFinder),
the latter was deprecated in 3.4, the former is new in 3.4. So my guess
is that _SixMetaPathImporter uses the old API so that it's compatible
with all versions of Python 3 (as Python probably does this fallback on
the old API if the new doesn't exist).

Would that be better?
Yuya Nishihara - April 23, 2019, 12:41 p.m.
On Mon, 22 Apr 2019 21:22:31 -0400, Ludovic Chabant wrote:
> >
> > Is there any list of exceptions that are known to be safely
> > suppressed? Catching AttributeError, TypeError, etc. seems bad.
> >
> 
> In my case, it's just a missing method so AttributeError is what's being
> raised I think (see
> https://github.com/benjaminp/six/blob/master/six.py#L164).
> 
> My rationale here was that there could be any 3rd party finder in there,
> so it might make sense to protect Mercurial from crashing because of
> exceptions thrown from other packages.
> 
> However, now that I think about it, another maybe more correct way to
> fix this specific bug might be to check for `find_spec` and, if missing,
> call `find_module` instead. As per the Python docs (see
> https://docs.python.org/3/library/importlib.html#importlib.abc.MetaPathFinder),
> the latter was deprecated in 3.4, the former is new in 3.4. So my guess
> is that _SixMetaPathImporter uses the old API so that it's compatible
> with all versions of Python 3 (as Python probably does this fallback on
> the old API if the new doesn't exist).
> 
> Would that be better?

Sounds good to me.

CC indygreg as he should know the importer API better.
Gregory Szorc - April 23, 2019, 1:51 p.m.
> On Apr 23, 2019, at 05:41, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Mon, 22 Apr 2019 21:22:31 -0400, Ludovic Chabant wrote:
>>> 
>>> Is there any list of exceptions that are known to be safely
>>> suppressed? Catching AttributeError, TypeError, etc. seems bad.
>>> 
>> 
>> In my case, it's just a missing method so AttributeError is what's being
>> raised I think (see
>> https://github.com/benjaminp/six/blob/master/six.py#L164).
>> 
>> My rationale here was that there could be any 3rd party finder in there,
>> so it might make sense to protect Mercurial from crashing because of
>> exceptions thrown from other packages.
>> 
>> However, now that I think about it, another maybe more correct way to
>> fix this specific bug might be to check for `find_spec` and, if missing,
>> call `find_module` instead. As per the Python docs (see
>> https://docs.python.org/3/library/importlib.html#importlib.abc.MetaPathFinder),
>> the latter was deprecated in 3.4, the former is new in 3.4. So my guess
>> is that _SixMetaPathImporter uses the old API so that it's compatible
>> with all versions of Python 3 (as Python probably does this fallback on
>> the old API if the new doesn't exist).
>> 
>> Would that be better?
> 
> Sounds good to me.
> 
> CC indygreg as he should know the importer API better.


Gracefully handling missing find_spec() should be fine. But we should use hasattr() or getattr() for that (Mercurial’s linter may insist on getattr() because I think we still ban hasattr()) because bare “except:” is bad.

The custom module importer has known issues with some module importers. E.g. it won’t work if the importer doesn’t make the module source available. Removing the source transforming bit of our module importer is likely a prerequisite to us getting Mercurial working with e.g. py2exe on Python 3. Or it will require gross workarounds. This should be outside the scope of this patch. But since you are encountering a custom meta path importer, it’s something you may need to be aware of. What I’m trying to say is our custom importer is still a bit fragile and I expect it to fail in a lot of cases at present.
Ludovic Chabant - April 23, 2019, 3:15 p.m.
>
> Gracefully handling missing find_spec() should be fine. But we should
> use hasattr() or getattr() for that (Mercurial’s linter may insist on
> getattr() because I think we still ban hasattr()) because bare
> “except:” is bad.

Thanks, I sent a v2 of my patches yesterday evening with that
change in them.

Patch

diff --git a/mercurial/__init__.py b/mercurial/__init__.py
--- a/mercurial/__init__.py
+++ b/mercurial/__init__.py
@@ -54,9 +54,14 @@ 
                 if finder == self:
                     continue
 
-                spec = finder.find_spec(fullname, path, target=target)
-                if spec:
-                    break
+                try:
+                    spec = finder.find_spec(fullname, path, target=target)
+                    if spec:
+                        break
+                except:
+                    # Some finders don't implement `find_spec`, like
+                    # `_SixMetaPathImporter`.
+                    pass
 
             # This is a Mercurial-related module but we couldn't find it
             # using the previously-registered finders. This likely means