classification
Title: Catching virtual subclasses in except clauses
Type: enhancement Stage: needs patch
Components: Interpreter Core Versions: Python 3.3
process
Status: open Resolution:
Dependencies: 22540 Superseder:
Assigned To: Nosy List: Antony.Lee, Jim.Jewett, Trundle, Yury.Selivanov, barry, benjamin.peterson, cvrebert, daniel.urban, eric.araujo, ethan.furman, gcbirzan, georg.brandl, gvanrossum, jamesh, jwilk, ncoghlan, pitrou, yorik.sar
Priority: normal Keywords: patch

Created on 2011-05-08 08:53 by acooke, last changed 2014-10-02 11:40 by georg.brandl.

Files
File name Uploaded Description Edit
issue12029.patch gcbirzan, 2012-05-12 10:48 review
exception_proper_subclass_matching_v2.patch georg.brandl, 2014-10-02 10:09 review
exception_proper_subclass_matching_v3.patch georg.brandl, 2014-10-02 10:20 review
pyobject_issubclass_isinstance_speedup.patch georg.brandl, 2014-10-02 11:29 review
exception_proper_subclass_matching_v4.patch georg.brandl, 2014-10-02 11:29 review
Messages (35)
msg135521 - (view) Author: andrew cooke (acooke) Date: 2011-05-08 08:53
Hi,

In general, registering a class with an ABC is equivalent to making it a subclass (isinstance and issubclass are patched through ABCMeta).  However, this does not work for exceptions (see example below, where exception is not caught).

This doesn't seem terribly surprising to me - I imagine that checking would slow down exception handling - but I couldn't find any documentation (and posting on c.l.p didn't turn up anything either).

So I thought I would raise it here - perhaps there is a possible fix (my obscure use case is that I have a backtracking search; backtracking occurs when a certain exception is encountered; making that exception an ABC and allowing existing exceptions to be registered with it allows the search to work with existing code without a wrapper that catches and translates exceptions that should trigger a backtrack).  Or perhaps the docs could be extended.  Or perhaps I've misunderstood something...

Cheers,
Andrew

Python 3.2 (r32:88445, Feb 27 2011, 13:00:05) 
[GCC 4.5.0 20100604 [gcc-4_5-branch revision 160292]] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from abc import ABCMeta
>>> class RootException(Exception,metaclass=ABCMeta): pass
... 
>>> class MyException(Exception): pass
... 
>>> RootException.register(MyException)
>>> try:
...     raise MyException
... except RootException:
...     print('caught')
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
__main__.MyException
msg136712 - (view) Author: Chris Rebert (cvrebert) * Date: 2011-05-24 02:10
Scouting around the CPython codebase a bit, I speculate that the cause of this behavior is that PyErr_GivenExceptionMatches() in errors.c uses PyType_IsSubtype() [which simply walks a class's __mro__ checking for pointer equality] rather than PyObject_IsSubclass()/PyObject_IsInstance() [which are smart enough to consult __subclasscheck__()/__instancecheck__() if they exist].

Of course, the more important issue here is whether this behavior is intended or not. I surmise python-dev needs to have a discussion about it?
msg136717 - (view) Author: Chris Rebert (cvrebert) * Date: 2011-05-24 02:45
Surveying the docs, the current behavior *is* /technically/ correct (in a suspiciously precise way) according to the Language Reference:
http://docs.python.org/dev/reference/compound_stmts.html#grammar-token-try_stmt :
"For an except clause with an expression [...] the clause matches the exception if the resulting object is 'compatible' with the exception. An object is compatible with an exception if it is the class or a base class of the exception object" (which exactly describes what PyType_IsSubtype() checks for)

The Tutorial is by contrast much more vague:
http://docs.python.org/dev/tutorial/errors.html#handling-exceptions :
"if [the raised exception's] type matches the exception named after the except keyword, the except clause is executed, and then execution continues after the try statement."
No definition of what it means for the types to "match" seems to be given.
msg160399 - (view) Author: James Henstridge (jamesh) Date: 2012-05-11 04:44
The documentation for ABCMeta.register() says that it makes the other class a "virtual subclass".  That would make the ABC a "virtual base class".

So whether the current behaviour is correct depends on whether you consider a "virtual base" to count as a base.  From the reasoning behind the introduction of ABCs, it certainly sounds like it should count.

Also, this is a feature that works correctly in Pyton 2.7, so could trip people up who are trying to move to Python 3.
msg160418 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-05-11 16:37
I agree it's a bug and should be fixed.  It's too confusing that there would be two slightly different interpretations of isinstance/issubclass where the isinstance() and issubclass() would be using the extended interpretation but the except clause would use the narrow interpretation.

The exception matching done by the except clause ought to be explainable in terms of issubclass/isinstance.
msg160426 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-05-11 17:04
I think being able to catch exception with ABCs is esssentially useless. The originally stated "usecase" can be simply solved by putting classes into a tuple and putting that in the except clause.

In general, the whole abc machinary causes lots of code which expects instance and subclass checks to be side-effect free to be able to execute arbitrary code, which creates messes.
msg160427 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-11 17:08
Perhaps ABCMeta could raise a UserWarning when creating an Exception subclass?
msg160428 - (view) Author: andrew cooke (acooke) Date: 2012-05-11 17:20
perhaps it could just work in a simple, consistent way?

in my original report i wondered whether there was a significant performance hit.  but so far the objections against fixing this seem to be (1) a lawyer could be convinced the current behaviour is consistent with the docs (2) python 3 should remain compatible with python 2 (3) abcmeta is the sucksorz.

those don't seem like great arguments against making it just work right, to me.
msg160430 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-05-11 17:32
> perhaps it could just work in a simple, consistent way?

That would be best obviously. But as Benjamin explained it's quite delicate to make it work while avoiding pitfalls where code involved in exception checking may itself fail with arbitrary errors - say, enter an infinite recursion. It's also why I think it would be a bad idea to fix it in 3.2 (the bugfix branch). In 3.3 we can take riskier decisions.
msg160432 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2012-05-11 17:37
Basically, someone needs to produce a patch and we can go from there.
msg160435 - (view) Author: George-Cristian Bîrzan (gcbirzan) Date: 2012-05-11 17:47
I posted on python dev that this would slow exception checking considerably so that is a concern. As for possible bugs, this has been working in the 2 branch for a while now, so I don't think that is the biggest issue. 
As for possible use cases, writing a wrapper around backend, each with its own exceptions and still being able to catch a 'base' exception in your code while still having the ability to catch specific exceptions, without doing awkward stuff like looking at __cause__ (let alone that you have to reraise that in 2 for code that has to run on both branches). Yes, you could patch the exceptions' bases but that is what Abc was created to avoid. 

Sorry for the mistakes and weird phrasing, posting this off my phone.
msg160436 - (view) Author: George-Cristian Bîrzan (gcbirzan) Date: 2012-05-11 17:49
I have a patch, with tests, but no Internet on my computer so going out, will post it when I get back/my Internet comes back
msg160461 - (view) Author: James Henstridge (jamesh) Date: 2012-05-12 00:54
Benjamin: if you are after a use case for this feature, see  https://code.djangoproject.com/ticket/15901

In Django, there are multiple database backends, each of which currently catch the adapter's DatabaseError and reraise it as Django's DatabaseError so that Django code can handle database errors in a standard way without having to care about which backend they came from.  Unfortunately, this loses some information from the exception.

My idea for solving that bug was to make Django's DatabaseError an ABC.  By registering the various adapter's DatabaseErrors with the ABC, it would not be necessary to catch and reraise them in the backends while still preserving the ability to catch the generic errors in the core.  This works fine in Python 2.x, but it was pointed out that it would cause compatibility problems when porting to Python 3.2.
msg160468 - (view) Author: George-Cristian Bîrzan (gcbirzan) Date: 2012-05-12 10:48
As promissed the patch. It doesn't break any tests, and it passes the ones I added. I have a pybench one as well, which even though trivial, does point to the fact that there is a degradation in performance, but not sure it's worth posting here.
msg161473 - (view) Author: Jim Jewett (Jim.Jewett) Date: 2012-05-24 02:26
When does the performance hit occur?

If it is only when an exception has been raised, and its own class is not listed by the except clause, then I personally wouldn't worry about it; tracing the MRO *could* get arbitrarily long already; it just doesn't in practice.  The same should be true of virtual subclassing.

On the other hand, if it adds another module or three to the required startup set, that might be a concern...
msg200418 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-19 13:54
The performance hit is that such a change would potentially make it more expensive to figure out that a raised exception *doesn't* match a given "except" clause, along with the complexity of introducing execution of arbitrary code while still unwinding the stack looking for an exception handler for the original exception.

As Benjamin noted above we already support dynamic exception handling through dynamically bound tuple lookups, so I don't think this feature is needed for the Django used case:

>>> caught_exceptions = ()
>>> def f(to_raise):
...     try:
...         raise to_raise
...     except caught_exceptions:
...         print("Caught the exception")
... 
>>> f(Exception)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in f
Exception
>>> caught_exceptions = (Exception,)
>>> f(Exception)
Caught the exception

I know Guido indicated above that he considers the current behaviour a bug, and I even agree that enshrining the two slightly different definitions of "issubclass" is ugly, but the complete lack of use cases without other solutions and the complex implications of unifying them mean that I think it may be worth accepting the additional complexity in the language definition instead.

That means the question in my mind is whether we can make it less surprising/user-hostile by issuing a warning at class definition time.

Since all exceptions are required to inherit from BaseException in order to be permitted in raise statements *or* except clauses, it seems to me that having a check in PyType ready that emits a warning when it detects the use of the virtual subclass machinery should suffice. That is, all of these should emit a warning:

  class BadExceptionABC_1(BaseException, metaclass=abc.ABCMeta): pass
  class BadExceptionABC_2(abc.ABC, BaseException): pass
  class BadExceptionABC_3(BaseException):
      def __instancecheck__(*args): return False
  class BadExceptionABC_4(BaseException):
      def __subclasscheck__(*args): return False

We could even go further and make it a DeprecationWarning intially and upgrade to a full TypeError in a later release (although we obviously can't do that if Guido would prefer to unify the behaviour instead).
msg200420 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-19 14:07
Actually, I take back the performance comment - Jim's right that in the normal case, the slowdown should be minimal (it's just some additional checks that certain slots aren't populated on each listed exception class), and types can already influence that by messing with the MRO.

That just leaves the complexity argument associated with running arbitrary code at an unexpected place in the eval loop, and for that the tests in the patch would need to be strengthened with some pathological examples like:

- __subclasscheck__ raising an exception
- __subclasscheck__ raising and then suppressing an exception
- __subclasscheck__ invoking the garbage collector
- __subclasscheck__ hitting the recursion limit
- __subclasscheck__ provoking MemoryError

The games the current patch already has to play with the recursion limit also bother me.

However, if an alternate approach could be found that avoids the adjustment of the recursion limit in the eval loop, that also sensibly survived the kinds of arbitrary code execution torture tests I mention above, then I'd be far more sanguine about the idea of actually resolving the discrepancy rather than formalising it as part of the general fact that the exception hierarchy isn't as flexible as most of the rest of the language.
msg200421 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-19 14:10
> Actually, I take back the performance comment - Jim's right that in
> the normal case, the slowdown should be minimal (it's just some
> additional checks that certain slots aren't populated on each listed
> exception class), and types can already influence that by messing with
> the MRO.

I think that the performance question can only really be answered by
running (micro-)benchmarks here.
msg200829 - (view) Author: Yuriy Taraday (yorik.sar) Date: 2013-10-21 19:34
Can someone please point out why do we have to do that dance with recursion limit?

I've came upon this problem as well. I had some (bad) API I had to work with. It always raised the same exception with the only difference in the message. So I thought I could do something like this:

def message_contains(msg):
    class _MyExc(object):
        def __instancecheck__(self, exc):
            return msg in exc.args[0]
    return _MyExc

But after I tried it in number of different ways I found out that it's not possible.

So here's another reason to change this behavior.
msg201329 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-10-26 08:28
Because context managers are closer to try/finally blocks than they are to exception handling, the class-based implementation for the contextlib.suppress API uses issubclass rather than emulating the CPython exception handling semantics: http://hg.python.org/cpython/file/09153a9a3bb9/Lib/contextlib.py#l202

The exception checking in the unittest module is similarly based on issubclass: http://hg.python.org/cpython/file/09153a9a3bb9/Lib/unittest/case.py#l129

I'm planning to add the catch() and ExitLabel() context managers to contextlib2 this evening, and those too will be based on issubclass().

Perhaps as a near term thing, we should put an "implementation detail" notice somewhere in the language reference, pointing that it's the code using issubclass that is considered correct here, and CPython's exception handling that is considered out of line?
msg202019 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-11-03 12:36
A point on the safety/correctness front: I remembered we already run arbitrary code at roughly this point in the eval loop, as we have to invoke __iter__ to get the exceptions to check when an iterable is used in except clause.

That means allowing the subclass check hooks to run here isn't as radical a change as I first thought.
msg205830 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-12-10 15:44
"I remembered we already run arbitrary code at roughly this point in the eval loop, as we have to invoke __iter__ to get the exceptions to check when an iterable is used in except clause."

Are you sure?  IIRC the except clause only accept exceptions and tuples of exceptions, not iterators.
msg205880 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-12-11 04:37
Ah, you're right - I found the example I was thinking of (Richard Jones's "Don't do this!" talk), and it was just demonstrating that the except clause accepts any expressions producing a tuple or BaseException instance, not that we call __iter__ at that point.

And since we do identity checks for the exception type matching (rather than equality checks), it looks like all the avenues for arbitrary code execution while checking if an exception handler matches a thrown an exception are closed off.
msg228158 - (view) Author: Antony Lee (Antony.Lee) * Date: 2014-10-02 04:53
"it looks like all the avenues for arbitrary code execution while checking if an exception handler matches a thrown an exception are closed off."

This seems to be directly contradicted by your previous sentence: "the except clause accepts any expressions producing a tuple or BaseException instance".

e.g.

===

>>> def f(): raise AttributeError
... 
>>> try: raise IndexError
... except f(): raise KeyError
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 1, in f
AttributeError

===

(note that f() is evaluated only if the body of "try" actually raises)
msg228159 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-10-02 05:10
ISTM Nick meant that the exception that was raised can't cause arbitrary
code execution.

On Wednesday, October 1, 2014, Antony Lee <report@bugs.python.org> wrote:

>
> Antony Lee added the comment:
>
> "it looks like all the avenues for arbitrary code execution while checking
> if an exception handler matches a thrown an exception are closed off."
>
> This seems to be directly contradicted by your previous sentence: "the
> except clause accepts any expressions producing a tuple or BaseException
> instance".
>
> e.g.
>
> ===
>
> >>> def f(): raise AttributeError
> ...
> >>> try: raise IndexError
> ... except f(): raise KeyError
> ...
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> IndexError
>
> During handling of the above exception, another exception occurred:
>
> Traceback (most recent call last):
>   File "<stdin>", line 2, in <module>
>   File "<stdin>", line 1, in f
> AttributeError
>
> ===
>
> (note that f() is evaluated only if the body of "try" actually raises)
>
> ----------
> nosy: +Antony.Lee
>
> _______________________________________
> Python tracker <report@bugs.python.org <javascript:;>>
> <http://bugs.python.org/issue12029>
> _______________________________________
>
msg228171 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-10-02 07:31
Right, I had a specific concern related to the way the C level code works.
On closer inspection, it turned out all the Python level code execution is
complete by the time we reach the point I was worried about.
msg228195 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-02 10:01
I'm attaching a patch that works without changing the recursion limit, and adds some tests for pathological cases.

Instead, PyErr_GivenExceptionMatches is changed so that if an exception was previously set, it is replaced by an exception that PyObject_IsSubclass raises.  In that way recursion errors should be propagated properly.

In exception matching, this means that exceptions (including recursion errors) from PyObject_IsSubclass are ignored.  There is already an explicit test for this behavior in test_exceptions.

This behavior *could* be changed if intended by introducing a variant of PyErr_GivenExceptionMatches that can set an exception even if none was set before, and calling that in cmp_outcome in ceval.
msg228196 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-02 10:09
New version including (I think) correct refcount handling.
msg228198 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-02 10:20
Clarifying some comments in this one.
msg228209 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-02 10:51
I'm worried about the runtime cost of this. Code like this is an extremely common idiom and should remain fast:

try:
    x = d[key]
except KeyError:
    # do something else
msg228213 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-02 11:09
Agreed.

Since type has __subclasscheck__ (why I don't know) this might result in a slowdown since all checks have to go through calling it in PyObject_IsSubclass.

Just noticed that given_exception_matches_inner can be simplified a bit since PyObject_IsSubclass already checks for tuples by itself.
msg228214 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-02 11:15
Quick microbenchmark:

try:
    {}["a"]                       
except KeyError:
    pass

original tip:  1.35 usec
with patch v3: 1.55 usec

so it's about 15% slowdown for catching a simple exception on my machine.
msg228215 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-02 11:19
> Since type has __subclasscheck__ (why I don't know)

Ouch, really? That means the PyType_IsSubtype path in PyObject_IsSubclass() is never taken?

I think we should add a tp_subclasscheck slot to minimize the general cost of this. Also, try to see if there aren't redundant type / tuple checks along the way.
msg228216 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-02 11:29
OK, with these two patches (speedup and v4) I can't see a significant slowdown anymore.
msg228219 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2014-10-02 11:40
IsSubclass speedup patch now tracked in #22540.
History
Date User Action Args
2014-10-02 11:40:06georg.brandlsetdependencies: + speed up isinstance and issubclass for the usual cases
messages: + msg228219
2014-10-02 11:29:16georg.brandlsetfiles: + exception_proper_subclass_matching_v4.patch
2014-10-02 11:29:10georg.brandlsetfiles: + pyobject_issubclass_isinstance_speedup.patch

messages: + msg228216
2014-10-02 11:19:50pitrousetmessages: + msg228215
2014-10-02 11:15:29georg.brandlsetmessages: + msg228214
2014-10-02 11:09:45georg.brandlsetmessages: + msg228213
2014-10-02 10:51:08pitrousetmessages: + msg228209
2014-10-02 10:21:02georg.brandlsetfiles: - exception_proper_subclass_matching.patch
2014-10-02 10:20:29georg.brandlsetfiles: + exception_proper_subclass_matching_v3.patch

messages: + msg228198
2014-10-02 10:09:37georg.brandlsetfiles: + exception_proper_subclass_matching_v2.patch

messages: + msg228196
2014-10-02 10:01:50georg.brandlsetfiles: + exception_proper_subclass_matching.patch
nosy: + georg.brandl
messages: + msg228195

2014-10-02 07:31:47ncoghlansetmessages: + msg228171
2014-10-02 05:10:07gvanrossumsetmessages: + msg228159
2014-10-02 04:53:05Antony.Leesetnosy: + Antony.Lee
messages: + msg228158
2013-12-11 04:37:45ncoghlansetmessages: + msg205880
2013-12-10 15:44:38gvanrossumsetmessages: + msg205830
2013-12-10 12:07:39jwilksetnosy: + jwilk
2013-11-03 12:36:15ncoghlansetmessages: + msg202019
2013-10-26 08:28:09ncoghlansetmessages: + msg201329
2013-10-21 19:34:25yorik.sarsetnosy: + yorik.sar
messages: + msg200829
2013-10-21 17:04:21ethan.furmansetnosy: + ethan.furman
2013-10-20 21:15:25barrysetnosy: + barry
2013-10-19 14:10:47pitrousetmessages: + msg200421
2013-10-19 14:07:14ncoghlansetmessages: + msg200420
2013-10-19 13:54:25ncoghlansetmessages: + msg200418
2013-10-19 13:33:32ncoghlansetnosy: + ncoghlan
2012-09-16 21:37:51Trundlesetnosy: + Trundle
2012-05-24 02:26:45Jim.Jewettsetnosy: + Jim.Jewett
messages: + msg161473
2012-05-12 10:48:57gcbirzansetfiles: + issue12029.patch
keywords: + patch
messages: + msg160468
2012-05-12 01:09:05jameshsettype: behavior -> enhancement
2012-05-12 00:54:20jameshsettype: enhancement -> behavior
messages: + msg160461
2012-05-11 20:17:09Yury.Selivanovsetnosy: + Yury.Selivanov
2012-05-11 17:49:48gcbirzansetmessages: + msg160436
2012-05-11 17:47:57gcbirzansetnosy: + gcbirzan
messages: + msg160435
2012-05-11 17:37:20benjamin.petersonsetmessages: + msg160432
2012-05-11 17:32:28pitrousetmessages: + msg160430
2012-05-11 17:22:13acookesetnosy: - acooke
2012-05-11 17:20:31acookesetmessages: + msg160428
2012-05-11 17:09:00pitrousetnosy: + pitrou
messages: + msg160427
2012-05-11 17:04:52benjamin.petersonsetmessages: + msg160426
2012-05-11 16:57:39pitrousetnosy: + benjamin.peterson

type: behavior -> enhancement
versions: - Python 3.2
2012-05-11 16:44:47eric.araujosetstage: needs patch
versions: + Python 3.2
2012-05-11 16:37:40gvanrossumsetnosy: + gvanrossum
messages: + msg160418
2012-05-11 11:31:55eric.araujosettitle: ABC registration of Exceptions -> Catching virtual subclasses in except clauses
versions: + Python 3.3, - Python 3.2
2012-05-11 04:44:47jameshsetnosy: + jamesh
messages: + msg160399
2011-07-27 15:08:12eric.araujosetfiles: - generictramadolhclonline.html
2011-07-27 15:08:10eric.araujosetfiles: - cheapcodfedextramadolvery.html
2011-07-27 12:48:12junior1971setfiles: + generictramadolhclonline.html
2011-07-27 12:04:51junior1971setfiles: + cheapcodfedextramadolvery.html
2011-05-24 02:45:08cvrebertsetmessages: + msg136717
2011-05-24 02:10:15cvrebertsetmessages: + msg136712
2011-05-13 17:07:20eric.araujosetnosy: + eric.araujo
2011-05-08 09:32:58daniel.urbansetnosy: + daniel.urban
2011-05-08 09:02:07cvrebertsetnosy: + cvrebert
2011-05-08 08:53:43acookecreate