-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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
gh-129640: Add tests for reference cycle in gzip.GzipFile
#130916
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some triage nits.
Misc/NEWS.d/next/Library/2025-03-06-20-25-54.gh-issue-129640.iub32dw1.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ZeroIntensity for the suggestions, I've made the change 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the weakref not sufficient?
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I think just timing / this issue was added before, PR added after, didn't see this issue when I was working on the other one. Could you check if the issue is resolved with current main and/or 3.12 branch? I think #130055 should have fixed it as well as what it was directly trying to address (bad destruction order) by adding a weakref. I do like the testing for number of GCd object test / think that would be good to add regardless |
Thanks @cmaloney the issue is fixed in the cc. @hauntsaninja TiA. |
#130055 is backported to 3.12 and 3.13 (#130670 and #130669), so should be in the next minor release of both. According to the 3.13 Release Schedule (https://peps.python.org/pep-0719/), 3.13.3 bugfix release which should contain it is expected 2025-04-08. |
gzip.GzipFile
gzip.GzipFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmaloney @hauntsaninja as of our discussions, I changed this PR for the tests for reference cycle in gzip.GzipFile
.
- Also I'm not sure if it needs any NEWS entry, please let me know it need one.
Requesting @hauntsaninja for a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test passes on the commit prior to #130055, so I'm not sure it's testing what we want it to test. len(gc.get_objects())
is quite coarse, where does the magic 10 number come from? Can we instead get a weakref to the _WriteBufferStream
and confirm it gets collected?
Issue: #129640
Related to: #129726
gzip.GzipFile
creates reference cycle that requires a deep garbage collection cycle to cleanup. #129640