Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The default unraisablehook implementation does not print chained exceptions or recurse into exception groups #95572

Closed
graingert opened this issue Aug 2, 2022 · 14 comments
Assignees
Labels
3.13 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@graingert
Copy link
Contributor

graingert commented Aug 2, 2022

here's a demo in the repl

>>> class Foo:
...     def __del__(self):
...         raise BaseExceptionGroup("the bad", [Exception("critical debug advice here")])
... 
>>> f = Foo()
>>> del f
Exception ignored in: <function Foo.__del__ at 0x7f2f59df2200>
Traceback (most recent call last):
  File "<stdin>", line 3, in __del__
ExceptionGroup: the bad (1 sub-exception)
>>> 

@vstinner @iritkatriel I'm not sure if this is an omission or by design, but if an exception with critical debug advice is raised in an exceptiongroup during __del__ I can't see the type or error message

I expected the output to include:

  + Exception Group Traceback (most recent call last):
  |   File "<stdin>", line 1, in <module>
  | ExceptionGroup: the bad (1 sub-exception)
  +-+---------------- 1 ----------------
    | Exception: critical debug advice here
    +------------------------------------
@graingert graingert added the type-bug An unexpected behavior, bug, or error label Aug 2, 2022
@iritkatriel
Copy link
Member

Looks like the default unraisable hook has its own implementation of exception printing. Why isn't it just calling _PyErr_Display?

@graingert
Copy link
Contributor Author

looks like the threading.excepthook does the right thing:

>>> t = threading.Thread(target=raise_, args=(BaseExceptionGroup("the bad", [Exception("critical debug advice here")]),))
>>> t.start()
Exception in thread Thread-4 (raise_):
  + Exception Group Traceback (most recent call last):
  |   File "/usr/lib/python3.11/threading.py", line 1038, in _bootstrap_inner
  |     self.run()
  |   File "/usr/lib/python3.11/threading.py", line 975, in run
  |     self._target(*self._args, **self._kwargs)
  |   File "<stdin>", line 1, in raise_
  | ExceptionGroup: the bad (1 sub-exception)
  +-+---------------- 1 ----------------
    | Exception: critical debug advice here
    +------------------------------------
>>> t.join()
>>> 

@graingert graingert added 3.11 only security fixes 3.12 bugs and security fixes labels Aug 2, 2022
@graingert
Copy link
Contributor Author

Looks like the default unraisable hook has its own implementation of exception printing. Why isn't it just calling _PyErr_Display?

if this is a bug in unraisable hook can we get the fix backported to 3.10 so the exceptiongroup backport can use it?

@iritkatriel
Copy link
Member

I don't know if this is a bug or an enhancement. In any case it's hookable, right?

@iritkatriel
Copy link
Member

Looks like the default unraisable hook has its own implementation of exception printing. Why isn't it just calling _PyErr_Display?

if this is a bug in unraisable hook can we get the fix backported to 3.10 so the exceptiongroup backport can use it?

I don't think back porting this even could affect the exception group backport - in 3.10 _PyErr_Display() doesn't know anything about exception groups.

@iritkatriel
Copy link
Member

The default unraisablehook doesn't print chained exceptions either.

I think this may be deliberate to avoid recursion when the interpreter could be in a delicate state (e.g. under memory pressure). This is definitely not a bugfix. I'm removing 3.11.

@iritkatriel iritkatriel removed type-bug An unexpected behavior, bug, or error 3.11 only security fixes labels Aug 3, 2022
@iritkatriel iritkatriel changed the title exception information missing from unraisable exceptiongroups The default unraisablehook implementation does not print chained exceptions or recurse into exception groups Aug 3, 2022
@iritkatriel iritkatriel added the type-feature A feature request or enhancement label Aug 3, 2022
@graingert
Copy link
Contributor Author

in the backport if I delete the custom __str__ on BaseExceptionGroup, I get closer to the desired behavior:

>>> del exceptiongroup.BaseExceptionGroup.__str__
>>> class Foo:
...     def __del__(self):
...         raise exceptiongroup.BaseExceptionGroup("the bad", [Exception("critical debug information")])
... 
>>> f = Foo()
>>> del f
Exception ignored in: <function Foo.__del__ at 0x7f65d65ade10>
Traceback (most recent call last):
  File "<stdin>", line 3, in __del__
exceptiongroup.ExceptionGroup: ('the bad', [Exception('critical debug information')])
>>> 

@iritkatriel
Copy link
Member

Yes, but then the normal tracebacks will probably have the strs of the nested exceptions twice.

@graingert
Copy link
Contributor Author

graingert commented Aug 3, 2022

I've seen a fair bit of code that does print(type(e)), print(e) for "logging" and results in output like "<class 'ExceptionGroup'>: exception in task group (1 sub-exception)"

I think the normal tracebacks should change how ExceptionGroup is printed and the ExceptionGroup.__str__ should keep the default __str__

@iritkatriel
Copy link
Member

We can't keep the default __str__ because the list of nested exceptions can be very large and we would need to limit the number of exceptions we include in the output. We could implement str that prints a few of the nested exceptions, but I'm not sure that's better (how do you know which ones have critical info on them?).

How are you proposing to change the normal tracebacks?

@graingert
Copy link
Contributor Author

graingert commented Aug 11, 2022

We could implement str that prints a few of the nested exceptions, but I'm not sure that's better (how do you know which ones have critical info on them?).

How many is too many? Ideally exceptions wouldn't be printed and instead go via the traceback formatter, but sometimes that's not possible, so the last resort is to print them all

How are you proposing to change the normal tracebacks?

The normal tracebacks wouldn't change in appearance, I'm suggesting they special case ExceptionGroup outright

Specifically they would not call ExceptionGroup.__str__ and would print {type_name}: {message} ({n} sub-exception{plural})

This is particularly handy because we can do this in the backport

@iritkatriel
Copy link
Member

iritkatriel commented Aug 11, 2022

What if you subclassed ExceptionGroup and redefined str?

@iritkatriel iritkatriel added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 27, 2023
@erlend-aasland erlend-aasland added 3.13 bugs and security fixes and removed 3.12 bugs and security fixes labels Jan 5, 2024
@gvanrossum
Copy link
Member

I would honestly close this as "won't fix". I don't see the point of spending the resources on trying to improve this corner case without making things more fragile.

@iritkatriel
Copy link
Member

I agree, this is hookable so anyone can implement what they need anyway.

@iritkatriel iritkatriel closed this as not planned Won't fix, can't repro, duplicate, stale Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants