gh-102251: add missing cleanups for `test_import` #104796

sunmy2019 · 2023-05-23T10:26:56Z

Leak reasons are described here.

For 1, the fix is straightforward.

For 2, I added 2 clean-up functions and trigger them manually.

For 3, I haven't figured out a clean solution, but I would prefer to skip it under ref leak modes only. So I introduce a new method because AFAIK there isn't a way to skip only ref leak tests.

Issue: Fix Refleak in test_import #102251

Eclips4 · 2023-05-23T13:13:59Z

As I understand, this doesn't solve problem, just "shuts up" the noise from known refleaks on test_import, right?

sunmy2019 · 2023-05-23T13:42:43Z

As I understand, this doesn't solve problem, just "shuts up" the noise from known refleaks on test_import, right?

That is not true. I do think I did resolve all ref leaks on Linux.

But I did not test it on Windows.

Eclips4 · 2023-05-23T13:47:13Z

As I understand, this doesn't solve problem, just "shuts up" the noise from known refleaks on test_import, right?

That is not true. I do think I did resolve all ref leaks on Linux.

But I did not test it on Windows.

Oh, seems on Windows problem doesn't solved. I rebuilt intepreter with your changes and got it:

 ./python -m test -R 3:3 test_import
Running Debug|x64 interpreter...
0:00:00 Run tests sequentially
0:00:00 [1/1] test_import
beginning 6 repetitions
123456
......
test_import leaked [41, 43, 40] references, sum=124
test_import leaked [41, 41, 40] memory blocks, sum=122
test_import failed (reference leak)

== Tests result: FAILURE ==

1 test failed:
    test_import

Total duration: 28.7 sec
Tests result: FAILURE

sunmy2019 · 2023-05-23T14:03:43Z

Lib/test/test_import/__init__.py

-        if '-R' in sys.argv or '--huntrleaks' in sys.argv:
-            # https://github.com/python/cpython/issues/102251
-            raise unittest.SkipTest('unresolved refleaks (see gh-102251)')


This trick cannot be used with ./python -m test -j n, thus causing failures on CI.

sunmy2019 · 2023-05-23T14:12:07Z

All ref leaks on Linux come from SinglephaseInitTests, but running

 .\python_d.exe -m test test_import -R 3:3 -v -i *SinglephaseInitTests*

on Windows does give ref leaks

OK (skipped=5)
.
test_import leaked [41, 43, 40] references, sum=124
test_import leaked [41, 41, 40] memory blocks, sum=122

sunmy2019 · 2023-05-23T14:15:25Z

@Eclips4 Can you bisect which test leaks ref?

Eclips4 · 2023-05-23T14:50:45Z

@Eclips4 Can you bisect which test leaks ref?

Sure!
If we delete ImportTests/test_concurrency refleak tests are passed. Seems that problem in this test

Eclips4 · 2023-05-23T20:25:11Z

Also, I tried to found commit which introduce this reference leak ( in test_concurrency)
and again bisected to #94504..

Eclips4 · 2023-05-26T21:37:30Z

Oh, seems your solution also works on Windows. ( I forgot to add -j 1 parameter 🤦‍♂️ , sorry)
But problem with test_concurrency still there, and that's interesting, because it doesn't reproducible on Linux.

Probably, it should be a another PR (at the current moment I still haven't found a solution).
My some notes about it:
If we delete these lines from test_concurrency:

sys.modules.pop('package', None)
sys.modules.pop('package.submodule', None)

Refleak tests will pass.
Also, this problem confirms that Windows runner in buildbots fleet really needed.

erlend-aasland

The libregrtest modification seems way too hacky for my taste. What happens if you apply my suggestion for _testsinglephase.c, and revert all other changes introduced in this PR?

Modules/_testsinglephase.c

sunmy2019 · 2023-05-30T03:22:07Z

revert all other changes introduced in this PR?

There are 3 leak points when running in Linux. Applying your suggestions only fixes one of them.

erlend-aasland · 2023-05-30T07:08:30Z

revert all other changes introduced in this PR?

There are 3 leak points when running in Linux. Applying your suggestions only fixes one of them.

I suggest you turn this PR¹ into a fix for this one issue, so we can land that fix. The fix is obvious; I can land that for you today. The rest of the changes are controversial; I think it would be wise to separate them out.

Also: please don't tag people with in commit messages. Those commits ends up as noise in the GitHub notifications page.

or create a new PR for this ↩

sunmy2019 · 2023-05-30T08:51:16Z

or create a new PR for this ↩

I will create a new one.

#105082

erlend-aasland · 2023-05-30T10:38:02Z

Status for buildbot run for commit 20307d0:

buildbot/AMD64 Fedora Stable Refleaks PR: test_peg_generator leaked
buildbot/AMD64 RHEL7 Refleaks PR: test_peg_generator leaked
buildbot/AMD64 RHEL8 Refleaks PR: test_peg_generator leaked
buildbot/PPC64LE RHEL8 Refleaks PR: test_peg_generator leaked
buildbot/aarch64 RHEL8 Refleaks PR: test_peg_generator leaked
buildbot/s390x Fedora Refleaks PR: test_peg_generator leaked
buildbot/s390x RHEL7 Refleaks PR: test_peg_generator leaked
buildbot/s390x RHEL8 Refleaks PR: test_peg_generator leaked

ericsnowcurrently

LGTM

ericsnowcurrently · 2023-05-31T14:44:16Z

GitHub took me straight to "Files changed", so I didn't see the other comments before I gave an approving review. The change looks fine to me, but please be sure to address any other comments before this is merged.

sunmy2019 · 2023-05-31T14:55:01Z

The second part is split to #105085

I will make this PR focus on the 3rd
part of the problem.

sunmy2019 · 2023-06-01T02:43:36Z

Now this PR only has changes related to the 3rd part.

test_basic_multiple_interpreters_deleted_no_reset deliberately leaks things. And we have a "no-leak" version test right after this: test_basic_multiple_interpreters_reset_each.

I think the correct thing to do here is to skip this specific test when hunting ref leaks.

However, the previous approach does not work under -j. And the test main has no way to pass the information to the actual test. So I came up with the name trick.

I am open to any other methods.

Lib/test/libregrtest/main.py

bedevere-bot · 2023-06-23T04:28:17Z

🤖 New build scheduled with the buildbot fleet by @sunmy2019 for commit 5934e50 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

Lib/test/test_import/__init__.py

bedevere-bot · 2023-06-23T04:36:40Z

🤖 New build scheduled with the buildbot fleet by @sunmy2019 for commit 60b4921 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

erlend-aasland · 2023-06-23T12:56:53Z

Closing in favour of gh-106013. Thanks for the effort you've put into this @sunmy2019; highly appreciated!

Also, thanks for keeping up with me; sorry for pushing you in different directions, but my opinions changed.

add some cleanups

db000e8

bedevere-bot mentioned this pull request May 23, 2023

Fix Refleak in test_import #102251

Closed

sunmy2019 marked this pull request as ready for review May 23, 2023 10:27

sunmy2019 requested review from brettcannon, encukou, ericsnowcurrently, ncoghlan and warsaw as code owners May 23, 2023 10:27

bedevere-bot added the awaiting review label May 23, 2023

sunmy2019 added the skip news label May 23, 2023

remove the sys.argv trick

e6bc99c

sunmy2019 commented May 23, 2023

View reviewed changes

Merge branch 'main' into pythongh-102251

ea8e894

brettcannon removed their request for review May 29, 2023 17:06

erlend-aasland reviewed May 29, 2023

View reviewed changes

Modules/_testsinglephase.c Outdated Show resolved Hide resolved

erlend-aasland reviewed May 29, 2023

View reviewed changes

Modules/_testsinglephase.c Outdated Show resolved Hide resolved

sunmy2019 added 2 commits May 30, 2023 11:36

Merge branch 'main' of github.com:python/cpython into pythongh-102251

5dbb8c3

follow @erlend-aasland's advice

c6b42ee

revert this as this would be intended at pythongh-104796

2ee60e2

This comment was marked as outdated.

# to view

ericsnowcurrently approved these changes May 31, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels May 31, 2023

erlend-aasland mentioned this pull request May 31, 2023

gh-102251: Explicitly free state for test modules with state in test_import #105085

Merged

sunmy2019 added 2 commits June 1, 2023 10:26

Merge branch 'main' of github.com:python/cpython into pythongh-102251

75230b0

rebase after changes to python#105085

695fd16

erlend-aasland mentioned this pull request Jun 2, 2023

gh-104614: Make Sure ob_type is Always Set Correctly by PyType_Ready() #105122

Merged

erlend-aasland reviewed Jun 5, 2023

View reviewed changes

Lib/test/libregrtest/main.py Outdated Show resolved Hide resolved

This was referenced Jun 20, 2023

gh-105310: add decorator to skip rerunning tests #105311

Closed

test_basic_multiple_interpreters_deleted_no_reset() of test_imp/test_import leaks references #106000

Closed

sunmy2019 added 2 commits June 23, 2023 11:33

Merge branch 'main' of github.com:python/cpython into pythongh-102251

0480053

collect tests in a single class

5934e50

sunmy2019 removed the request for review from encukou June 23, 2023 04:27

sunmy2019 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023

sunmy2019 commented Jun 23, 2023

View reviewed changes

Lib/test/test_import/__init__.py Outdated Show resolved Hide resolved

Update Lib/test/test_import/__init__.py

60b4921

sunmy2019 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 23, 2023

erlend-aasland mentioned this pull request Jun 23, 2023

gh-102251: Disable non-rerunnable test in test_import #106013

Merged

erlend-aasland closed this Jun 23, 2023

sunmy2019 mentioned this pull request Jun 28, 2023

test_import.test_concurrency leaks ref on Windows #106176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-102251: add missing cleanups for `test_import` #104796

gh-102251: add missing cleanups for `test_import` #104796

sunmy2019 commented May 23, 2023 •

edited by bedevere-bot

Loading

Eclips4 commented May 23, 2023

sunmy2019 commented May 23, 2023

Eclips4 commented May 23, 2023

sunmy2019 May 23, 2023

sunmy2019 commented May 23, 2023 •

edited

Loading

sunmy2019 commented May 23, 2023

Eclips4 commented May 23, 2023

Eclips4 commented May 23, 2023 •

edited

Loading

Eclips4 commented May 26, 2023 •

edited

Loading

erlend-aasland left a comment

sunmy2019 commented May 30, 2023 •

edited

Loading

erlend-aasland commented May 30, 2023

sunmy2019 commented May 30, 2023 •

edited

Loading

erlend-aasland commented May 30, 2023 •

edited

Loading

This comment was marked as outdated.

ericsnowcurrently left a comment

ericsnowcurrently commented May 31, 2023

sunmy2019 commented May 31, 2023

sunmy2019 commented Jun 1, 2023

bedevere-bot commented Jun 23, 2023

bedevere-bot commented Jun 23, 2023

erlend-aasland commented Jun 23, 2023

gh-102251: add missing cleanups for test_import #104796

gh-102251: add missing cleanups for test_import #104796

Conversation

sunmy2019 commented May 23, 2023 • edited by bedevere-bot Loading

Eclips4 commented May 23, 2023

sunmy2019 commented May 23, 2023

Eclips4 commented May 23, 2023

sunmy2019 May 23, 2023

Choose a reason for hiding this comment

sunmy2019 commented May 23, 2023 • edited Loading

sunmy2019 commented May 23, 2023

Eclips4 commented May 23, 2023

Eclips4 commented May 23, 2023 • edited Loading

Eclips4 commented May 26, 2023 • edited Loading

erlend-aasland left a comment

Choose a reason for hiding this comment

sunmy2019 commented May 30, 2023 • edited Loading

erlend-aasland commented May 30, 2023

Footnotes

sunmy2019 commented May 30, 2023 • edited Loading

erlend-aasland commented May 30, 2023 • edited Loading

This comment was marked as outdated.

ericsnowcurrently left a comment

Choose a reason for hiding this comment

ericsnowcurrently commented May 31, 2023

sunmy2019 commented May 31, 2023

sunmy2019 commented Jun 1, 2023

bedevere-bot commented Jun 23, 2023

bedevere-bot commented Jun 23, 2023

erlend-aasland commented Jun 23, 2023

gh-102251: add missing cleanups for `test_import` #104796

gh-102251: add missing cleanups for `test_import` #104796

sunmy2019 commented May 23, 2023 •

edited by bedevere-bot

Loading

sunmy2019 commented May 23, 2023 •

edited

Loading

Eclips4 commented May 23, 2023 •

edited

Loading

Eclips4 commented May 26, 2023 •

edited

Loading

sunmy2019 commented May 30, 2023 •

edited

Loading

sunmy2019 commented May 30, 2023 •

edited

Loading

erlend-aasland commented May 30, 2023 •

edited

Loading