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.

Sign up for GitHub

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

gh-129280: Avoid cyclic reference in contextlib.contextmanager #129276

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+3 −1
Open

gh-129280: Avoid cyclic reference in contextlib.contextmanager #129276

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Lib/contextlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,9 @@ def __exit__(self, typ, value, traceback):
# Suppress StopIteration *unless* it's the same exception that
# was passed to throw(). This prevents a StopIteration
# raised inside the "with" statement from being suppressed.
return exc is not value
result = exc is not value
del exc, value # avoid cyclic reference
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to make the appropriate change in asynccontextmanager, and add a test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think that this is the right fix. Should not all local variables be cleared after leaving the function? If they are not, we have a larger issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the traceback of the exception refers to the local variables

Copy link
Contributor

@graingert graingert Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fix here is to wrap this whole __exit__ in a:

try:
    ...
finally:
    del value, traceback

exc should be cleared automatically by leaving the except block

With a test case this should be a lot clearer, (have a look at the cyclic exception tests in test_taskgroup, (the ones that call no_other_refs)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@graingert or maybe this should be done one lever higher? Do other __exit__s have the same problem?

I agree with @serhiy-storchaka, this fix is not correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be pretty magical to automatically delete value and traceback from every __exit__ and __aexit__

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this was not an issue in 3.11?

I agree that finally with dels is better solution, but we should look if there is more fundamental issue, which affects not only contextmanager.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't entirely know why it was not an issue in Python 3.11, but concretely, if you add print(self.__traceback__.tb_frame.f_back.f_locals) in __del__ method, in Python 3.12 it prints out { ... 'value': MyException() ... } while in Python 3.11 it prints out AttributeError: 'NoneType' object has no attribute 'f_locals'.

return result
except RuntimeError as exc:
# Don't re-raise the passed in exception. (issue27122)
if exc is value:
Expand Down
Loading