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 reference cycling by the garbage collector during response reading #2932

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

hauntsaninja
Copy link
Contributor

Pull Request check-list

  • Does $ tox pass with this change (including linting)?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

Description of change

This won't meaningfully affect most users, so I won't feel bad if this is closed :-)

It's often important for performance to control when garbage collection runs and to reduce the need to run garbage collection in the first place.

During AbstractionConnection.on_connect, it's common to raise and catch ResponseError, since e.g. CLIENT SETINFO is very new. However, because response is in a local, it creates ref cycles that keep all locals in all calling stack frames alive.

The use of a local to store the exception is unfortunate, since exceptions hold references to their tracebacks, which hold references to the relevant frames, which holds a reference to the local exception. 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.

Fix suggested by JelleZijlstra

This won't meaningfully affect most users, so I won't feel bad if this
is closed :-)

It's often important for performance to control when garbage collection
runs and to reduce the need to run garbage collection in the first
place.

During AbstractionConnection.on_connect, it's common to raise and catch
ResponseError, since e.g. CLIENT SETINFO is very new. However, because
response is in a local, it creates ref cycles that keep all locals in
all calling stack frames alive.

The use of a local to store the exception is unfortunate, since
exceptions hold references to their tracebacks, which hold references to
the relevant frames, which holds a reference to the local exception.
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.
@dvora-h dvora-h added the maintenance Maintenance (CI, Releases, etc) label Sep 11, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03% ⚠️

Comparison is base (e3de026) 91.29% compared to head (f1df163) 91.27%.
Report is 5 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2932      +/-   ##
==========================================
- Coverage   91.29%   91.27%   -0.03%     
==========================================
  Files         126      126              
  Lines       32391    32393       +2     
==========================================
- Hits        29571    29566       -5     
- Misses       2820     2827       +7     
Files Changed Coverage Δ
redis/connection.py 81.17% <100.00%> (+0.05%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dvora-h dvora-h merged commit 578fb26 into redis:master Sep 14, 2023
53 of 54 checks passed
@hauntsaninja hauntsaninja deleted the deterministic-garbage branch September 14, 2023 17:14
@chayim chayim changed the title Avoid creating ref cycles Avoid reference cycling by the garbage collector during response reading Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance (CI, Releases, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants