Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-129640: Add tests for reference cycle ingzip.GzipFile#130916

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
Mr-Sunglasses wants to merge13 commits intopython:main
base:main
Choose a base branch
Loading
fromMr-Sunglasses:fix/129640

Conversation

Mr-Sunglasses
Copy link
Contributor

@Mr-SunglassesMr-Sunglasses commentedMar 6, 2025
edited
Loading

@ZeroIntensityZeroIntensity added needs backport to 3.12only security fixes needs backport to 3.13bugs and security fixes labelsMar 6, 2025
Copy link
Member

@ZeroIntensityZeroIntensity left a comment

Choose a reason for hiding this comment

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

Some triage nits.

Mr-Sunglasses reacted with thumbs up emoji
Mr-Sunglassesand others added2 commitsMarch 6, 2025 21:50
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Copy link
ContributorAuthor

@Mr-SunglassesMr-Sunglasses left a comment
edited
Loading

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 😄

Copy link
Contributor

@hauntsaninjahauntsaninja left a 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?

@bedevere-app
Copy link

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

@cmaloney
Copy link
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

@Mr-Sunglasses
Copy link
ContributorAuthor

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 themain branch and3.12 branch and sorry while looking in this issue, I was testing in myPython 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 to3.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

@cmaloney
Copy link
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

@Mr-SunglassesMr-Sunglasses changed the titlegh-129640: Fix reference cycle ingzip.GzipFilegh-129640: Add tests for reference cycle ingzip.GzipFileMar 8, 2025
Copy link
ContributorAuthor

@Mr-SunglassesMr-Sunglasses left a 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 ingzip.GzipFile.

  • Also I'm not sure if it needs any NEWS entry, please let me know it need one.

cmaloney reacted with thumbs up emoji
@Mr-Sunglasses
Copy link
ContributorAuthor

Requesting@hauntsaninja for a review.

Copy link
Contributor

@hauntsaninjahauntsaninja left a 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?

@tomasr8tomasr8 removed the needs backport to 3.12only security fixes labelApr 10, 2025
@serhiy-storchakaserhiy-storchaka added the needs backport to 3.14bugs and security fixes labelMay 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ZeroIntensityZeroIntensityZeroIntensity left review comments

@hauntsaninjahauntsaninjahauntsaninja requested changes

@cmaloneycmaloneycmaloney approved these changes

Assignees
No one assigned
Labels
awaiting changesneeds backport to 3.13bugs and security fixesneeds backport to 3.14bugs and security fixesskip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

6 participants
@Mr-Sunglasses@cmaloney@hauntsaninja@ZeroIntensity@serhiy-storchaka@tomasr8

[8]ページ先頭

©2009-2025 Movatter.jp