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

Avoid creating ref cycles #408

Merged
merged 10 commits into from
Jan 15, 2024
Merged

Avoid creating ref cycles #408

merged 10 commits into from
Jan 15, 2024

Conversation

hauntsaninja
Copy link
Contributor

By storing previously raised exceptions inside a local, this code created ref cycles that kept all locals in all calling stack frames alive.

This is because exceptions hold references to their tracebacks, which hold references to the relevant frames, which holds a reference to the local errors dict that holds references to the exceptions. See https://peps.python.org/pep-0344/#open-issue-garbage-collection and https://peps.python.org/pep-3110/#rationale

This breaks the cycle by deleting the local when we raise, so frames are destroyed by the normal reference counting mechanism.

This fixes some resource exhaustion issues I encountered at work.

hauntsaninja and others added 2 commits September 22, 2023 23:57
By storing previously raised exceptions inside a local, this code created ref cycles that kept all locals in all calling stack frames alive.

This is because exceptions hold references to their tracebacks, which hold references to the relevant frames, which holds a reference to the local errors dict that holds references to the exceptions. See https://peps.python.org/pep-0344/#open-issue-garbage-collection and https://peps.python.org/pep-3110/#rationale

This breaks the cycle by deleting the local when we raise, so frames are destroyed by the normal reference counting mechanism.

This fixes some resource exhaustion issues I encountered at work.
@coveralls
Copy link

coveralls commented Sep 23, 2023

Coverage Status

coverage: 97.888% (+0.004%) from 97.884% when pulling 8b5d2c6 on hauntsaninja:patch-1 into b29e6f4 on agronholm:master.

@agronholm
Copy link
Owner

A things of note:

  1. Why not delete the errors before returning?
  2. I would like to see a test that ensures there won't be reference cycles

@hauntsaninja
Copy link
Contributor Author

Thanks for the review! I must have missed your response. I've addressed both points now :-)

@agronholm
Copy link
Owner

Would you mind also adding a note in the changelog (under **UNRELEASED**)? Then we're good.

Copy link
Contributor Author

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, added the changelog, a fix, and a test for the fix

docs/versionhistory.rst Outdated Show resolved Hide resolved
@agronholm agronholm merged commit fe8e3bc into agronholm:master Jan 15, 2024
10 checks passed
@agronholm
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants