Skip to content

Conversation

@vstinner
Copy link
Member

@vstinnervstinner commented Feb 24, 2022

Copy link
Contributor

@kumaraditya303kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

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' 

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@corona10corona10Feb 24, 2022

Choose a reason for hiding this comment

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

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' 

Copy link
Member

@corona10corona10 left a comment

Choose a reason for hiding this comment

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

lgtm again

Copy link
Contributor

@erlend-aaslanderlend-aasland left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +1648 to +1654
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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.

@vstinnervstinner merged commit c9c178f into python:mainFeb 24, 2022
@vstinnervstinner deleted the test_no_memleak branch February 24, 2022 23:03
@vstinner
Copy link
MemberAuthor

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
Copy link
MemberAuthor

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
Copy link
MemberAuthor

I wrote #31560 to fix winreg leaks.

@jkloth
Copy link
Contributor

Windows buildbots are still failing

@vstinner
Copy link
MemberAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@vstinner@jkloth@corona10@erlend-aasland@kumaraditya303@the-knights-who-say-ni@bedevere-bot