The Wayback Machine - https://web.archive.org/web/20221223193145/https://github.com/python/cpython/pull/28005
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

Jump to bottom

bpo-25130: Add calls of gc.collect() in tests to support PyPy #28005

Merged
merged 4 commits into from Aug 29, 2021
Merged

bpo-25130: Add calls of gc.collect() in tests to support PyPy #28005

merged 4 commits into from Aug 29, 2021

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Aug 27, 2021 •
edited by bedevere-bot

gpshead
gpshead requested changes Aug 27, 2021
View changes
Copy link
Member

@gpshead gpshead left a comment

TL;DR - We should include comments on all of these stating why they exist and how to know when they're still relevant.

Calling gc.collect(), even multiple times, from any code, test or not, is usually a sign of fragility and something that isn't being done right. It can't guarantee any particular direct action.

When they don't make a difference in CPython if someone removes them, we can expect these to disappear from the codebase over time during maintenance.

If we want these, we should include a comment on all of them stating why it is here and how to determine if each is still relevant or not. Otherwise they become mysterious lore in the code and get copied around, deleted, changed, for no rhyme of reason over time.

@bedevere-bot
Copy link

bedevere-bot commented Aug 27, 2021

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@serhiy-storchaka
Copy link
Member Author

serhiy-storchaka commented Aug 28, 2021

What comments should I add? In most cases the pattern is the same:

wr = weakref.ref(obj)
del obj
gc.collect()
self.assertIsNone(wr())

It is all either to check that there are no other references to obj, or to trigger it's __del__.

@gpshead
Copy link
Member

gpshead commented Aug 28, 2021

Even just an end of line comment on the added calls such as

gc_collect()  # For pypy or other GCs.

So long as there's some indication of why the call is there given that today's CPython refcounting gc didn't need it.

@serhiy-storchaka
Copy link
Member Author

serhiy-storchaka commented Aug 28, 2021

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Aug 28, 2021

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from gpshead Aug 28, 2021
@serhiy-storchaka serhiy-storchaka merged commit 2a8127c into python:main Aug 29, 2021
12 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Aug 29, 2021

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the tests-gc-collect branch Aug 29, 2021
@miss-islington
Copy link
Contributor

miss-islington commented Aug 29, 2021

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2a8127cafe1d196f858a3ecabf5f1df3eebf9a12 3.10

@miss-islington
Copy link
Contributor

miss-islington commented Aug 29, 2021

Sorry @serhiy-storchaka, I had trouble checking out the 3.9 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 2a8127cafe1d196f858a3ecabf5f1df3eebf9a12 3.9

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Aug 29, 2021
@serhiy-storchaka
[3.10] bpo-25130: Add calls of gc.collect() in tests to support PyPy (p…
cf02804 
…ythonGH-28005).

(cherry picked from commit 2a8127c)

Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-bot
Copy link

bedevere-bot commented Aug 29, 2021

GH-28027 is a backport of this pull request to the 3.10 branch.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Aug 29, 2021
@serhiy-storchaka
[3.9] bpo-25130: Add calls of gc.collect() in tests to support PyPy (p…
d4f1241 
…ythonGH-28005).

(cherry picked from commit 2a8127c)

Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-bot
Copy link

bedevere-bot commented Aug 29, 2021

GH-28028 is a backport of this pull request to the 3.9 branch.

serhiy-storchaka added a commit that referenced this pull request Aug 29, 2021
@serhiy-storchaka
[3.9] bpo-25130: Add calls of gc.collect() in tests to support PyPy (G…
330aabb 
…H-28005). (GH-28028)

(cherry picked from commit 2a8127c)
ambv pushed a commit that referenced this pull request Sep 8, 2021
@serhiy-storchaka
[3.10] bpo-25130: Add calls of gc.collect() in tests to support PyPy (G…
462c1f0 
…H-28005) (GH-28027)

(cherry picked from commit 2a8127c)

Co-authored-by: Serhiy Storchaka <[email protected]>
@serhiy-storchaka serhiy-storchaka removed their assignment Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead

@1st1 1st1

@asvetlov asvetlov

@berkerpeksag berkerpeksag

@ethanfurman ethanfurman

@rhettinger rhettinger

Assignees
No one assigned
Labels
tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@serhiy-storchaka @bedevere-bot @gpshead @miss-islington @the-knights-who-say-ni