GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||
GeezerVison is showing github.com — gh-129640: Add tests for reference cycle in `gzip.GzipFile` by Mr-Sunglasses · Pull Request #130916 · python/cpython Conversation
|
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Thanks @cmaloney the issue is fixed in the cc. @hauntsaninja TiA. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
#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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
gzip.GzipFilegzip.GzipFile
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Requesting @hauntsaninja for a review. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The fix was merged long time ago, so now it is difficult to check that the new test catches the bug. Tests that depend on details of the garbage collector are not very reliable, I am not sure it will work on other implementations, like PyPI. So I think that it is not worth to add it now. #130055 has it's own test. Anyway, thank you for your contribution @Mr-Sunglasses. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.