Exception with broken __getattr__ causes error rendering tracebacks

[perrinjerome]

Bug report

Bug description:

Running this code:

import logging

class X(Exception):
    def __getattr__(self, a):
        raise KeyError(a)  # !!!

try:
    raise X()
except X:
    logging.getLogger().exception('')

print('ok')

does not print "ok" but instead:

# python3.11 repro.py 
--- Logging error ---
Traceback (most recent call last):
  File "/workspaces/cpython/repro.py", line 8, in <module>
    raise X()
object address  : 0x7f3007c67e80
object refcount : 2
object type     : 0x7f3015bf0b20
object type name: KeyError
object repr     : KeyError('__notes__')
lost sys.stderr

CPython versions tested on:

3.11, CPython main branch

Operating systems tested on:

Linux

Linked PRs

• gh-113358: Fix rendering tracebacks with exceptions with a broken __getattr__ #113359
• [3.12] gh-113358: Fix rendering tracebacks with exceptions with a broken __getattr__ (GH-113359) #114105
• [3.11] gh-113358: Fix rendering tracebacks with exceptions with a broken __getattr__ (GH-113359) #114109
• [3.11] gh-113358: Fix rendering tracebacks with exceptions with a broken __getattr__ (GH-113359) #114118
• [3.12] gh-113358: Fix rendering tracebacks with exceptions with a broken __getattr__ (GH-113359) #114173
• [3.11] gh-113358 Fix rendering tracebacks with exceptions with a broken __getattr__ : Normalize exception  #114379

[terryjreedy]

To me, this looks like a bug in your code, not in CPython.

[perrinjerome]

In this example, the user code is (of course) wrong, but it's not good that after such an error python is not able to output a proper traceback.

This example causes an error during the logging of error, which is unexpected from the logging system, as mentioned in https://docs.python.org/3.13/library/logging.html#logging.Handler.handleError :

(...) exceptions get silently ignored. This is what is mostly wanted for a logging system - most users will not care about errors in the logging system, they are more interested in application errors.

this documentation is for a different context, but it indicates the general idea that the logging module is trying to prevent its internal errors from propagating.

The traceback module is also generally careful about custom exceptions with broken behaviors and has several try / except, for example when printing an exception, or printing the notes attached to an exception (this is done by using the _safe_string utility function)

I guess it's not so uncommon to have exceptions with a __getattr__ that may raise something else than an AttributeError when __notes__ is accessed. urllib.error.HTTPError was a case from the standard library that was reported in #98778 . The fix has been to fix the underlying error, but in that issue it was also suggested to ignore exceptions while getting __notes__ in #98778 (comment)

Another case where this behavior is problematic is when such an exception is raised in an unittest, this crashes the testrunner :

import unittest

class X(Exception):
    def __getattr__(self, a):
        raise KeyError(a)

class T(unittest.TestCase):
    def test(self):
        raise X()

# python3.11 -m unittest repro.py 
Traceback (most recent call last):
  File "/usr/lib64/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib64/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/usr/lib64/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
       ^^^^^^^^
  File "/workspaces/cpython/repro.py", line 9, in test
    raise X()
object address  : 0x7f070f738d60
object refcount : 2
object type     : 0x7f071da27b20
object type name: KeyError
object repr     : KeyError('__notes__')
lost sys.stderr

I don't see explicit reference in the docs that unittest guarantees it would properly report all exceptions, but as a user I expect the testrunner to handle this and properly report an error.

[perrinjerome]

I forgot to mention that this is a new behaviour from python 3.11, in older versions there was no error.

[terryjreedy]

Thanks for providing more info on the issue. The above satisfies my question here and on the PR. I will leave it to others, though, to decide what to do.
@vsajip Still involved with logging?

[vsajip]

@terryjreedy Yes, I still maintain logging 🙂

[vsajip]

OK, it's not really a logging issue, but I'll review. The reasoning behind the attached change is that swallowing exceptions isn't ideal, but can be tolerated here since if an exception class is broken such that its __notes__ can't be retrieved, with this change it's as if there were no notes, and traceback printing can proceed. The exception swallowing would only affect the case where notes were added and couldn't be retrieved because of the broken __notes__ attribute access; since the notes can't be retrieved, they couldn't be put into the traceback anyway.

However, one thing still bugs me - if the exception class has a broken __getattr__, aren't a whole bunch of other things going to break too? For example, some exception handling user code that doesn't print a traceback, but accesses some other exception attribute such as an error code? This change won't do anything for that case - that would require handling in some other way.

Note that as far as logging is concerned, logging can be configured to swallow exceptions in production environments by setting logging.raiseExceptions to False (documented here). With that change, the script in the OP prints ok as expected. The default setting of logging.raiseExceptions is True as that makes sense in development/staging environments.

Regarding the mention of failures in test runners, I think that's a matter for refinements in test runners.

Summary: this change would perhaps be OK if we only wanted to fix traceback printing when exception attribute access is broken, but it seems not worth doing in isolation because in practice, broken attribute access in exceptions would lead to a host of other problems/failures during exception handling, which are not addressed by this change.

[vsajip]

I've marked this as pending again - if no feedback is received soon to the contrary, I'll close this issue and the PR as not planned, based on my analysis above.

[perrinjerome]

Thank you for the analysis, I also believe it's not a problem with logging module, but with traceback module. I used logging as an example because it's where I observed the problem.

That's a good point that such buggy exception cause problems in other places, but in my experience it has been different because when this causes error in user exception handling code, there is a traceback clearly showing the user where the problem, so it's just a normal error, but when such error happens internally in traceback module, it's hard to understand where the problem is, especially because it looks like something is wrong with python itself and not with user code.

I read again the discussion from the pep, there was this comment where it was suggested to catch errors while handling __notes__ - handling, not accessing, so it's arguably slightly different, but this seems the general idea that "we should not break traceback printing code".

traceback module already has some protection against many cases broken __repr__ or malformed __notes__, but since python 3.11, traceback module accesses __notes__ attribute, which does not exist in Exception base class, so if the exception class defines a __getattr__ it will always be called in this getattr(exc_value, '__notes__', None). I was thinking traceback should also not blindly "trust" that this getattr from user code, for same reasons that it does not trust repr from user code.

In a sense, the question here "how much traceback module tolerate errors while producing a traceback ?".

We can maybe ask @Zac-HD ( I hope you don't mind 🙇 ) who was author of the pep and addressed a similar problem, but with a different solution in agronholm/exceptiongroup#58 .

[vsajip]

Your points are reasonable, @perrinjerome - let's see if @Zac-HD will express an opinion.

[Zac-HD]

I want to be clear that this is not my decision to make, but as requested I'm happy to share my personal opinion. If a maintainer chimes in, I'd support their call either way.

The related change that I made in exceptiongroup is a temporary workaround for #98778. I considered this reasonable because the upstream issue had already been fixed, so we were just backporting the intended behavior for users of older Python versions.

In this case, we're actually faced with a decision about what to do in the standard library. My preferred approach (e.g. in Hypothesis and Pytest, which I maintain) is to always preserve as much information about errors as possible - they're testing libraries, and the tradeoffs are pretty clearly worth it there. #113359 (comment) would be the implementation of that approach in this case. However, my impression is that the standard library tends to be intentionally less tolerant of broken user code, partly to encourage users to fix such problems instead of leaving them. In this case I think I'd still favor the try/except/note-explanation proposal, but as above I'd be OK with either.

[vsajip]

Thanks, @Zac-HD,  for your input. I agree that while not covering all possible error cases caused by a broken __getattr__, this is a small improvement in the traceback module. However, in the spirit of "Errors should never pass silently.", I think, like @Zac-HD, that the PR should be modified as per the snippet in this comment. But then, shouldn't we also deal with an exception possibly raised in __setattr__ in the line

self.__notes__ = [f'Ignored exception getting __notes__: {_safe_string(e, '__notes__', repr)}']

So perhaps use object.__setattr__(self, '__notes__', [...])?

[Zac-HD]

Empirically, I've seen several broken __getattr__ instances but never a __setattr__ issue; it's much more rarely implemented. I don't think it's worth writing defensive code for that before we've ever observed the problem.

[vsajip]

Fair enough, though it's a small price to pay now to avoid another issue / PR / review cycle if it ever does happen.

[perrinjerome]

I might be missing something, but with the self.__notes__ we are talking about, self is TracebackException from traceback module, not the broken exception class, so I don't think we need to worry that it might have a broken __setattr__.

Thanks a lot for the feedback so far ! I'll try to update the linked pull request.

[vsajip]

we are talking about, self is TracebackException from traceback module, not the broken exception class

Ah, yes, of course. My mistake.

[vsajip]

@perrinjerome Backports seem to be still failing, would you please take a look? I'm a little short of time at the moment.

[perrinjerome]

sorry for the bad suggestion, I'll take a closer look at the backport now

[perrinjerome]

The backport for 3.11 is not straightforward, there was first a trivial conflict with #103895 that was not backported, but the hard part is that 3.11 branch does not have #110702 and there is also another C implementation of exception formatting in Python/pythonrun.c.

I made a new pull request #114118 in which I also adapted that C code with equivalent changes to also ignore exceptions while getting notes, would it be the correct way to proceed ?

[vsajip]

Will take a look when I get a chance. What about 3.12?

[perrinjerome]

Thank you @vsajip , 3.12 seems the same as 3.11 . I made a draft pull request with the same patch as 3.11 #114173

[iritkatriel]

Thank you!

[perrinjerome]

Good, thank you all !