GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-129640: Add tests for reference cycle in `gzip.GzipFile` by Mr-Sunglasses · Pull Request #130916 · python/cpython

Conversation

@Mr-Sunglasses

@Mr-Sunglasses Mr-Sunglasses commented Mar 6, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Issue: #129640

Related to: #129726

  • Issue: gzip.GzipFile creates reference cycle that requires a deep garbage collection cycle to cleanup. #129640

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Mr-Sunglasses added 2 commits March 6, 2025 20:22
@Mr-Sunglasses
pythongh-129640: Fix reference cycle in gzip.GzipFile
06999db
@Mr-Sunglasses
test: Add test for reference cycle in gzip.GzipFile
79b7f6c
@bedevere-app bedevere-app Bot added the awaiting review label Mar 6, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Mar 6, 2025
gzip.GzipFile creates reference cycle that requires a deep garbage collection cycle to cleanup. #129640
Closed
@Mr-Sunglasses
chore: add news entry
327f877
@ZeroIntensity ZeroIntensity added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Mar 6, 2025
ZeroIntensity
ZeroIntensity reviewed Mar 6, 2025
View reviewed changes

@ZeroIntensity ZeroIntensity left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mr-Sunglasses reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Misc/NEWS.d/next/Library/2025-03-06-20-25-54.gh-issue-129640.iub32dw1.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_gzip.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Mr-Sunglasses and others added 2 commits March 6, 2025 21:50
@Mr-Sunglasses @ZeroIntensity
docs: Add refence to the function in the NEWS
eefc287
Co-authored-by: Peter Bierma <[email protected]>
@Mr-Sunglasses @ZeroIntensity
test: Add refence to the issue in the test comment.
ee93a71
Co-authored-by: Peter Bierma <[email protected]>
Mr-Sunglasses
Mr-Sunglasses commented Mar 6, 2025
View reviewed changes

@Mr-Sunglasses Mr-Sunglasses left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Mr-Sunglasses
Merge branch 'main' into fix/129640
c17135e
hauntsaninja
hauntsaninja requested changes Mar 7, 2025
View reviewed changes

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting review labels Mar 7, 2025
@bedevere-app

bedevere-app Bot commented Mar 7, 2025

Copy link
Copy Markdown

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 have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cmaloney

cmaloney commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Mr-Sunglasses

Mr-Sunglasses commented Mar 7, 2025

Copy link
Copy Markdown
Contributor Author

Why is the weakref not sufficient?

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 main branch and 3.12 branch and sorry while looking in this issue, I was testing in my Python 3.13 and there it is reproducible, that is why i opened this PR and I did not see #130055 ( my bad 😅 ), So I think we need to backport #130055 it to 3.13.2 ? Also I'll remove the change, but I think as @cmaloney suggested we can add the tests.

cc. @hauntsaninja

TiA.

cmaloney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Mr-Sunglasses
Merge branch 'main' into fix/129640
0dbcd08
@cmaloney

cmaloney commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

#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.

Mr-Sunglasses reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mr-Sunglasses added 3 commits March 8, 2025 22:52
@Mr-Sunglasses
REVERT the changes in gzip module
5b9f6e6
@Mr-Sunglasses
Merge branch 'fix/129640' of github.com:Mr-Sunglasses/cpython into fi…
8826606
…x/129640
@Mr-Sunglasses
chore: removed news entry
67b4dd2
cmaloney
cmaloney approved these changes Mar 8, 2025
View reviewed changes
@Mr-Sunglasses
docs: Add the reference of pythonGH-129726 in tests
e6aa09d
@Mr-Sunglasses Mr-Sunglasses changed the title gh-129640: Fix reference cycle in gzip.GzipFile gh-129640: Add tests for reference cycle in gzip.GzipFile Mar 8, 2025
@Mr-Sunglasses
Merge branch 'main' into fix/129640
94fa2a8
Mr-Sunglasses
Mr-Sunglasses commented Mar 8, 2025
View reviewed changes

@Mr-Sunglasses Mr-Sunglasses left a comment

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cmaloney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ZeroIntensity ZeroIntensity added the skip news label Mar 8, 2025
@Mr-Sunglasses

Mr-Sunglasses commented Mar 9, 2025

Copy link
Copy Markdown
Contributor Author

Requesting @hauntsaninja for a review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Mr-Sunglasses
Mr-Sunglasses requested a review from hauntsaninja March 9, 2025 17:46
hauntsaninja
hauntsaninja requested changes Mar 9, 2025
View reviewed changes

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Mr-Sunglasses
Merge branch 'main' into fix/129640
13f1006
@tomasr8 tomasr8 removed the needs backport to 3.12 only security fixes label Apr 10, 2025
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.14 bugs and security fixes label May 8, 2025
@github-actions

github-actions Bot commented Apr 22, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 22, 2026
@serhiy-storchaka

serhiy-storchaka commented May 21, 2026

Copy link
Copy Markdown
Member

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.

Mr-Sunglasses and cmaloney reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka closed this May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity left review comments
@hauntsaninja hauntsaninja hauntsaninja requested changes
@cmaloney cmaloney cmaloney approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes skip news stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@Mr-Sunglasses @cmaloney @serhiy-storchaka @hauntsaninja @ZeroIntensity @tomasr8
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.