TracebackException should not hold reference to the exception traceback

[iritkatriel]

BPO	42482
Nosy	@gvanrossum, @vstinner, @ericvsmith, @stevendaprano, @miss-islington, @iritkatriel
PRs	bpo-42482: remove reference to exc_traceback from TracebackException #23531 [3.9] bpo-42482: remove reference to exc_traceback from TracebackException (GH-23531) #23578 [3.8] bpo-42482: remove reference to exc_traceback from TracebackException (GH-23531) #23579

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-12-04.22:19:54.938>
created_at = <Date 2020-11-27.10:11:08.862>
labels = ['3.8', 'type-bug', 'library', '3.9', '3.10']
title = 'TracebackException should not hold reference to the exception traceback'
updated_at = <Date 2020-12-04.22:19:54.938>
user = 'https://github.com/iritkatriel'

bugs.python.org fields:

activity = <Date 2020-12-04.22:19:54.938>
actor = 'iritkatriel'
assignee = 'none'
closed = True
closed_date = <Date 2020-12-04.22:19:54.938>
closer = 'iritkatriel'
components = ['Library (Lib)']
creation = <Date 2020-11-27.10:11:08.862>
creator = 'iritkatriel'
dependencies = []
files = []
hgrepos = []
issue_num = 42482
keywords = ['patch']
message_count = 10.0
messages = ['381938', '381945', '381946', '381956', '381959', '381962', '381969', '382211', '382212', '382529']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'vstinner', 'eric.smith', 'steven.daprano', 'miss-islington', 'iritkatriel']
pr_nums = ['23531', '23578', '23579']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue42482'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

[iritkatriel]

TracebackException holds a reference to the exc_traceback, which is wrong because (1) it's supposed to capture the output without holding references to real things. (2) it makes comparison wrong for equivalent exceptions, as in this example:

------------------------------------------

import sys
import traceback

excs = []
for _ in range(2):
    try:
        1/0
    except:
        excs.append(traceback.TracebackException(*sys.exc_info()))

print('formats equal: ', list(excs[0].format()) == list(excs[0].format()))
print('excs equal: ', excs[0] == excs[1])
excs[0].exc_traceback = excs[1].exc_traceback = None
print('excs equal w/o exc_traceback: ', excs[0] == excs[1])

------------------------------------------
Output:

formats equal: True
excs equal: False
excs equal w/o exc_traceback: True

------------------------------------------

The good news is that it's only used to check for non-None (added here https://bugs.python.org/issue24695) so should be easy to remove.

[stevendaprano]

This would be a change of behaviour, and 3.8 and 3.9 are in feature freeze, so we could only add it in 3.10.

You say:

"it's supposed to capture the output without holding references to real things"

Is this requirement documented somewhere, or is it just your preference?

"it makes comparison wrong for equivalent exceptions"

If the tracebacks are different, they're not exactly equivalent.

If we did accept this patch, would that mean there would no longer be any need to delete the exception variable at the end of an except block?

[iritkatriel]

From the TracebackException docstring:

"The traceback module captures enough attributes from the original exception to this intermediary form to ensure that no references are held, while still being able to fully print or format it."

If the tracebacks are different, they're not exactly equivalent.

The formatted form is identical, so from TracebackException's POV they are equivalent.

If we did accept this patch, would that mean there would no longer be any need to delete the exception variable at the end of an except block?

No, it's not related to that.

[gvanrossum]

For background, see iritkatriel#3 (comment) -- it seems the link to exc_traceback was added with little concern for the original design of TracebackExceptionGroup.

The question is, can we get rid of it, even though it's been undocumented.

[vstinner]

I consider PR 23531 change as a bugfix and IMO it's fine to backport it.

TracebackException docstring is quite explicit:

The traceback module captures enough attributes from the original exception
to this intermediary form to ensure that no references are held, while
still being able to fully print or format it.

This issue shows that there was a bug in the implementation.

--

The unit test should check the ref count of the exception and the exception traceback, to check functionally that no strong reference is hold.

[iritkatriel]

I've added the refcount check that Victor suggested.

[stevendaprano]

Re-adding older versions.

[miss-islington]

New changeset 427613f by Irit Katriel in branch 'master':
bpo-42482: remove reference to exc_traceback from TracebackException (GH-23531)
427613f

[miss-islington]

New changeset 1cc5c94 by Miss Islington (bot) in branch '3.8':
bpo-42482: remove reference to exc_traceback from TracebackException (GH-23531)
1cc5c94

[miss-islington]

New changeset 40b92f1 by Miss Islington (bot) in branch '3.9':
[3.9] bpo-42482: remove reference to exc_traceback from TracebackException (GH-23531) (GH-23578)
40b92f1