[PATCH 1 of 2] py3: protect from exceptions thrown by other meta-path finders
Gregory Szorc
gregory.szorc at gmail.com
Tue Apr 23 09:51:47 EDT 2019
> On Apr 23, 2019, at 05:41, Yuya Nishihara <yuya at 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.
More information about the Mercurial-devel
mailing list