bpo-1635741: test_embed cheks that Python does not leak by vstinner · Pull Request #31555 · python/cpython

vstinner · 2022-02-24T17:02:36Z

https://bugs.python.org/issue1635741

kumaraditya303

LGTM, thanks.

corona10 · 2022-02-24T17:18:16Z

FYI, from my macOS
test test_cmd_line failed -- Traceback (most recent call last): File "/Users/user/oss/cpython/Lib/test/test_cmd_line.py", line 124, in test_showrefcount self.assertRegex(err, br'^\[\d+ refs, \d+ blocks\]') ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: Regex didn't match: b'^\\[\\d+ refs, \\d+ blocks\\]' not found in b'[-1 refs, 0 blocks]\n'

Oh crap. Ok, let's tolerate negative ref count for now. Checking that refs <= 0 and blocks == 0 is already a step forward!

cc @kumaraditya303

I didn't account for negative refcount on other platforms as I only saw output on Linux where it is zero.

For the record,
https://github.com/python/cpython/runs/5322493186?check_suite_focus=true
Ubuntu is also failed with the same reason.
FAIL: test_showrefcount (test.test_cmd_line.CmdLineTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_cmd_line.py", line 124, in test_showrefcount self.assertRegex(err, br'^\[\d+ refs, \d+ blocks\]') ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: Regex didn't match: b'^\\[\\d+ refs, \\d+ blocks\\]' not found in b'[-1 refs, 0 blocks]\n'

corona10

lgtm again

erlend-aasland

The test itself looks good to me (I suggested to use a test.support helper, if you want fewer lines), butEmbeddingTestsMixin (where this test lives) does not run at all on out-of-tree-builds (at least on my MacBook):

$ ./python.exe -m test test_embed -m test_no_memleak -v
Raised RLIMIT_NOFILE: 256 -> 1024
== CPython 3.11.0a5+ (heads/test_no_memleak-dirty:e6ca72c9e4, Feb 24 2022, 20:21:11) [Clang 13.0.0 (clang-1300.0.29.30)]
== macOS-12.2.1-x86_64-i386-64bit little-endian
== cwd: /Users/erlendaasland/src/cpython-build/build/test_python_68548æ
== CPU count: 16
== encodings: locale=UTF-8, FS=utf-8
0:00:00 load avg: 1.87 Run tests sequentially
0:00:00 load avg: 1.87 [1/1] test_embed
test_no_memleak (test.test_embed.MiscTests) ... skipped "'/Users/erlendaasland/src/cpython-build/Programs/_testembed' doesn't exist"

EmbeddingTestsMixin is fenced out by its setUp function:

ifexepath!=expecteddirornotos.path.exists(exe):
self.skipTest("%r doesn't exist"%exe)

For OOT builds, exepath != expecteddir resolves to True and not os.path.exists(exe) resolves to False.

Hm, looks like this is a deliberate choice. See f4b1244 / GH-29063.

erlend-aasland · 2022-02-24T19:16:56Z

+ cmd = [sys.executable, "-I", "-X", "showrefcount", "-c", "pass"]
+ proc = subprocess.run(cmd,
+ stdout=subprocess.PIPE,
+ stderr=subprocess.STDOUT,
+ text=True)
+ self.assertEqual(proc.returncode, 0)
+ out = proc.stdout.rstrip()


Why not this?
Suggested change
cmd= [sys.executable, "-I", "-X", "showrefcount", "-c", "pass"]
proc=subprocess.run(cmd,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
text=True)
self.assertEqual(proc.returncode, 0)
out=proc.stdout.rstrip()
rc, _, out=assert_python_ok("-I", "-X", "showrefcount", "-c", "pass")
self.assertEqual(rc, 0)
out=out.decode()
(You'd need from test.support.script_helper import assert_python_ok)

assert_python_ok() adds arguments to the command line. Here I really want to control the exact command line. Sadly, Python still leaks some references depending on the exact command line.

vstinner · 2022-02-24T23:04:09Z

test_no_memleak (test.test_embed.MiscTests) ... skipped "'/Users/erlendaasland/src/cpython-build/Programs/_testembed' doesn't exist"

That's unfortunate :-( test_no_memleak() doesn't use _testembed program.

vstinner · 2022-02-25T01:16:45Z

test_embed fails on AMD64 Windows10 3.x:
https://buildbot.python.org/all/#/builders/146/builds/2092

FAIL: test_no_memleak (test.test_embed.MiscTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\test_embed.py", line 1662, in test_no_memleak
self.assertLessEqual(refs, 0, out)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 254 not less than or equal to 0 : [254 refs, 105 blocks]

Strange. The test didn't fail on the Windows CI jobs on this PR.

vstinner · 2022-02-25T02:04:31Z

I wrote #31560 to fix winreg leaks.

jkloth · 2022-02-25T16:49:35Z

Windows buildbots are still failing

vstinner · 2022-02-25T17:03:17Z

I know and I don't have time to fix the remaining leak, so I created https://bugs.python.org/issue46857 and #31589

vstinner added the skip news label Feb 24, 2022

bedevere-bot added the awaiting core review label Feb 24, 2022

the-knights-who-say-ni added the CLA signed label Feb 24, 2022

bedevere-bot added the tests Tests in the Lib/test dir label Feb 24, 2022

corona10 mentioned this pull request Feb 24, 2022
bpo-46430: Add test for checking block leak is zero #31556
Closed

corona10 approved these changes Feb 24, 2022
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Feb 24, 2022

kumaraditya303 approved these changes Feb 24, 2022
View reviewed changes

kumaraditya303 requested changes Feb 24, 2022
View reviewed changes

Comment threadLib/test/test_embed.py Outdated

corona10 reviewed Feb 24, 2022
View reviewed changes

bpo-1635741: test_embed cheks that Python does not leak
e6ca72c

corona10 approved these changes Feb 24, 2022
View reviewed changes

erlend-aasland approved these changes Feb 24, 2022
View reviewed changes

vstinner merged commit c9c178f into python:mainFeb 24, 2022

bedevere-bot removed the awaiting merge label Feb 24, 2022

vstinner deleted the test_no_memleak branch February 24, 2022 23:03

asvetlov pushed a commit that referenced this pull request Feb 26, 2022
bpo-1635741: test_embed cheks that Python does not leak (GH-31555)
cd08421

kylotanmannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed

-cmd= [sys.executable, "-I", "-X", "showrefcount", "-c", "pass"]
-proc=subprocess.run(cmd,
-stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT,
-text=True)
-self.assertEqual(proc.returncode, 0)
-out=proc.stdout.rstrip()
+rc, _, out=assert_python_ok("-I", "-X", "showrefcount", "-c", "pass")
+self.assertEqual(rc, 0)
+out=out.decode()

Uh oh!

Uh oh!

Conversation

vstinner commented Feb 24, 2022• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumaraditya303 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corona10Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

vstinnerFeb 24, 2022

Choose a reason for hiding this comment

Uh oh!

corona10Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

kumaraditya303Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

corona10Feb 24, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

erlend-aasland left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erlend-aaslandFeb 24, 2022

Choose a reason for hiding this comment

Uh oh!

vstinnerFeb 24, 2022

Choose a reason for hiding this comment

Uh oh!

vstinner commented Feb 24, 2022

Uh oh!

vstinner commented Feb 25, 2022

Uh oh!

vstinner commented Feb 25, 2022

Uh oh!

jkloth commented Feb 25, 2022

Uh oh!

vstinner commented Feb 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vstinner commented Feb 24, 2022•
edited by bedevere-bot
Loading

corona10Feb 24, 2022•
edited
Loading

erlend-aasland left a comment •
edited
Loading