◐ Shell
reader mode source ↗
Skip to content

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

Merged
serhiy-storchaka merged 4 commits into
python:mainfrom
serhiy-storchaka:tests-gc-collect
Aug 29, 2021
Merged

bpo-25130: Add calls of gc.collect() in tests to support PyPy#28005
serhiy-storchaka merged 4 commits into
python:mainfrom
serhiy-storchaka:tests-gc-collect

Conversation

@serhiy-storchaka

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

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka added tests Tests in the Lib/test dir needs backport to 3.9 needs backport to 3.10 only security fixes labels Aug 27, 2021
gpshead
gpshead requested changes Aug 27, 2021
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide 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

bedevere-bot commented Aug 27, 2021

Copy link
Copy Markdown

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

@serhiy-storchaka

serhiy-storchaka commented Aug 28, 2021

Copy link
Copy Markdown
Member Author

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

gpshead commented Aug 28, 2021

Copy link
Copy Markdown
Member

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

serhiy-storchaka commented Aug 28, 2021

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

@bedevere-bot

bedevere-bot commented Aug 28, 2021

Copy link
Copy Markdown

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 August 28, 2021 07:26
@serhiy-storchaka serhiy-storchaka merged commit 2a8127c into python:main Aug 29, 2021
@miss-islington

miss-islington commented Aug 29, 2021

Copy link
Copy Markdown
Contributor

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 August 29, 2021 11:04
@miss-islington

miss-islington commented Aug 29, 2021

Copy link
Copy Markdown
Contributor

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

miss-islington commented Aug 29, 2021

Copy link
Copy Markdown
Contributor

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 <storchaka@gmail.com>
@bedevere-bot

bedevere-bot commented Aug 29, 2021

Copy link
Copy Markdown

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 <storchaka@gmail.com>
@bedevere-bot

bedevere-bot commented Aug 29, 2021

Copy link
Copy Markdown

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 <storchaka@gmail.com>
@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 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

@ethanfurman ethanfurman Awaiting requested review from ethanfurman ethanfurman is a code owner

@rhettinger rhettinger Awaiting requested review from 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.

5 participants

@serhiy-storchaka @bedevere-bot @gpshead @miss-islington @the-knights-who-say-ni