From 2f3e2771a4d9a52c27f8eb227aec9bd9133bcb5a Mon Sep 17 00:00:00 2001 From: Paresh Joshi Date: Sat, 15 Nov 2025 14:28:23 +0000 Subject: [PATCH 1/4] gh-141592: test_perf_profiler: fix race in perf map cleanup using addCleanup Signed-off-by: Paresh Joshi --- Lib/test/test_perf_profiler.py | 120 +++++++++++++++++++++++---------- 1 file changed, 83 insertions(+), 37 deletions(-) diff --git a/Lib/test/test_perf_profiler.py b/Lib/test/test_perf_profiler.py index e6852c93e69830..d6057e7109c4eb 100644 --- a/Lib/test/test_perf_profiler.py +++ b/Lib/test/test_perf_profiler.py @@ -35,17 +35,27 @@ def supports_trampoline_profiling(): class TestPerfTrampoline(unittest.TestCase): - def setUp(self): - super().setUp() - self.perf_files = set(pathlib.Path("/tmp/").glob("perf-*.map")) - - def tearDown(self) -> None: - super().tearDown() - files_to_delete = ( - set(pathlib.Path("/tmp/").glob("perf-*.map")) - self.perf_files - ) - for file in files_to_delete: - file.unlink() + # + # We've removed the buggy setUp and tearDown methods. + # They caused race conditions by scanning /tmp for `perf-*.map` files. + # Instead, we now use `addCleanup()` inside each test to delete + # the *specific* files we know that test created. + # + + def _cleanup_perf_map(self, pid): + """ + Helper to safely delete a specific perf map file. + We register this with addCleanup() to make sure it runs + even if the test assertions fail. + """ + perf_map = pathlib.Path(f"/tmp/perf-{pid}.map") + try: + if perf_map.exists(): + perf_map.unlink() + except OSError: + # Surpress errors, e.g., file already removed or + # another kind of race condition. + pass @unittest.skipIf(support.check_bolt_optimized(), "fails on BOLT instrumented binaries") def test_trampoline_works(self): @@ -64,13 +74,21 @@ def baz(): with temp_dir() as script_dir: script = make_script(script_dir, "perftest", code) env = {**os.environ, "PYTHON_JIT": "0"} - with subprocess.Popen( + + # Start the subprocess... + process = subprocess.Popen( [sys.executable, "-Xperf", script], text=True, stderr=subprocess.PIPE, stdout=subprocess.PIPE, env=env, - ) as process: + ) + + # ...and immediately register a cleanup task for its PID. + # This is guaranteed to run after the test. + self.addCleanup(self._cleanup_perf_map, process.pid) + + with process: stdout, stderr = process.communicate() self.assertEqual(stderr, "") @@ -116,9 +134,11 @@ def baz_fork(): def foo(): pid = os.fork() if pid == 0: + # We're in the child process print(os.getpid()) baz_fork() else: + # We're in the parent process _, status = os.waitpid(-1, 0) sys.exit(status) @@ -140,11 +160,19 @@ def baz(): stdout=subprocess.PIPE, env=env, ) as process: + # Register cleanup for the PARENT process + self.addCleanup(self._cleanup_perf_map, process.pid) stdout, stderr = process.communicate() self.assertEqual(process.returncode, 0) self.assertEqual(stderr, "") + + # The child process printed its PID to stdout child_pid = int(stdout.strip()) + + # Register cleanup for the CHILD process + self.addCleanup(self._cleanup_perf_map, child_pid) + perf_file = pathlib.Path(f"/tmp/perf-{process.pid}.map") perf_child_file = pathlib.Path(f"/tmp/perf-{child_pid}.map") self.assertTrue(perf_file.exists()) @@ -199,6 +227,8 @@ def baz(): stdout=subprocess.PIPE, env=env, ) as process: + # Register cleanup inside the loop for each process created + self.addCleanup(self._cleanup_perf_map, process.pid) stdout, stderr = process.communicate() self.assertEqual(stderr, "") @@ -308,6 +338,10 @@ def perf_command_works(): def run_perf(cwd, *args, use_jit=False, **env_vars): + # This helper function runs perf and stores its output files *inside* + # the 'cwd' (which is a temp_dir()). This is good! It means + # those files (`perf_output.perf`, `jit_output.dump`) + # are automatically cleaned up when the temp_dir() context manager exits. env = os.environ.copy() if env_vars: env.update(env_vars) @@ -434,23 +468,30 @@ def baz(n): "Unwinding is unreliable with frame pointers", ) class TestPerfProfiler(unittest.TestCase, TestPerfProfilerMixin): + # + # Just like in TestPerfTrampoline, we remove the racy + # setUp/tearDown methods. + # + + def _cleanup_perf_map(self, pid): + """ + Helper to safely delete a specific perf map file. + We need this for test_pre_fork_compile. + """ + perf_map = pathlib.Path(f"/tmp/perf-{pid}.map") + try: + if perf_map.exists(): + perf_map.unlink() + except OSError: + pass + def run_perf(self, script_dir, script, activate_trampoline=True): + # The run_perf helper handles its own temp files inside script_dir + # so we don't need to add cleanups for it here. if activate_trampoline: return run_perf(script_dir, sys.executable, "-Xperf", script) return run_perf(script_dir, sys.executable, script) - def setUp(self): - super().setUp() - self.perf_files = set(pathlib.Path("/tmp/").glob("perf-*.map")) - - def tearDown(self) -> None: - super().tearDown() - files_to_delete = ( - set(pathlib.Path("/tmp/").glob("perf-*.map")) - self.perf_files - ) - for file in files_to_delete: - file.unlink() - def test_pre_fork_compile(self): code = """if 1: import sys @@ -499,11 +540,17 @@ def compile_trampolines_for_all_functions(): stdout=subprocess.PIPE, env=env, ) as process: + # This test also creates map files, so we clean them up + # just like in TestPerfTrampoline. + self.addCleanup(self._cleanup_perf_map, process.pid) stdout, stderr = process.communicate() self.assertEqual(process.returncode, 0) self.assertNotIn("Error:", stderr) + child_pid = int(stdout.strip()) + self.addCleanup(self._cleanup_perf_map, child_pid) + perf_file = pathlib.Path(f"/tmp/perf-{process.pid}.map") perf_child_file = pathlib.Path(f"/tmp/perf-{child_pid}.map") self.assertTrue(perf_file.exists()) @@ -550,6 +597,14 @@ def _is_perf_version_at_least(major, minor): _is_perf_version_at_least(6, 6), "perf command may not work due to a perf bug" ) class TestPerfProfilerWithDwarf(unittest.TestCase, TestPerfProfilerMixin): + # + # We've removed the racy setUp/tearDown methods from here, too. + # The `run_perf` function (with use_jit=True) already creates its + # .dump and .so files inside the `temp_dir()`, which gets + # cleaned up automatically. The old setUp/tearDown were + # trying to clean up /tmp, which was buggy and unreliable. + # + def run_perf(self, script_dir, script, activate_trampoline=True): if activate_trampoline: return run_perf( @@ -557,18 +612,9 @@ def run_perf(self, script_dir, script, activate_trampoline=True): ) return run_perf(script_dir, sys.executable, script, use_jit=True) - def setUp(self): - super().setUp() - self.perf_files = set(pathlib.Path("/tmp/").glob("jit*.dump")) - self.perf_files |= set(pathlib.Path("/tmp/").glob("jitted-*.so")) - - def tearDown(self) -> None: - super().tearDown() - files_to_delete = set(pathlib.Path("/tmp/").glob("jit*.dump")) - files_to_delete |= set(pathlib.Path("/tmp/").glob("jitted-*.so")) - files_to_delete = files_to_delete - self.perf_files - for file in files_to_delete: - file.unlink() + # + # No setUp or tearDown needed! + # if __name__ == "__main__": From c56afcda537a758fa75d949546fc4bda8b26bcc1 Mon Sep 17 00:00:00 2001 From: Paresh Joshi Date: Sat, 15 Nov 2025 15:32:34 +0000 Subject: [PATCH 2/4] gh-141592: remove all explanatory comments per review Signed-off-by: Paresh Joshi --- Lib/test/test_perf_profiler.py | 56 ---------------------------------- 1 file changed, 56 deletions(-) diff --git a/Lib/test/test_perf_profiler.py b/Lib/test/test_perf_profiler.py index d6057e7109c4eb..af2952d5eafb6e 100644 --- a/Lib/test/test_perf_profiler.py +++ b/Lib/test/test_perf_profiler.py @@ -35,26 +35,16 @@ def supports_trampoline_profiling(): class TestPerfTrampoline(unittest.TestCase): - # - # We've removed the buggy setUp and tearDown methods. - # They caused race conditions by scanning /tmp for `perf-*.map` files. - # Instead, we now use `addCleanup()` inside each test to delete - # the *specific* files we know that test created. - # def _cleanup_perf_map(self, pid): """ Helper to safely delete a specific perf map file. - We register this with addCleanup() to make sure it runs - even if the test assertions fail. """ perf_map = pathlib.Path(f"/tmp/perf-{pid}.map") try: if perf_map.exists(): perf_map.unlink() except OSError: - # Surpress errors, e.g., file already removed or - # another kind of race condition. pass @unittest.skipIf(support.check_bolt_optimized(), "fails on BOLT instrumented binaries") @@ -75,7 +65,6 @@ def baz(): script = make_script(script_dir, "perftest", code) env = {**os.environ, "PYTHON_JIT": "0"} - # Start the subprocess... process = subprocess.Popen( [sys.executable, "-Xperf", script], text=True, @@ -84,8 +73,6 @@ def baz(): env=env, ) - # ...and immediately register a cleanup task for its PID. - # This is guaranteed to run after the test. self.addCleanup(self._cleanup_perf_map, process.pid) with process: @@ -134,11 +121,9 @@ def baz_fork(): def foo(): pid = os.fork() if pid == 0: - # We're in the child process print(os.getpid()) baz_fork() else: - # We're in the parent process _, status = os.waitpid(-1, 0) sys.exit(status) @@ -160,17 +145,13 @@ def baz(): stdout=subprocess.PIPE, env=env, ) as process: - # Register cleanup for the PARENT process self.addCleanup(self._cleanup_perf_map, process.pid) stdout, stderr = process.communicate() self.assertEqual(process.returncode, 0) self.assertEqual(stderr, "") - # The child process printed its PID to stdout child_pid = int(stdout.strip()) - - # Register cleanup for the CHILD process self.addCleanup(self._cleanup_perf_map, child_pid) perf_file = pathlib.Path(f"/tmp/perf-{process.pid}.map") @@ -227,7 +208,6 @@ def baz(): stdout=subprocess.PIPE, env=env, ) as process: - # Register cleanup inside the loop for each process created self.addCleanup(self._cleanup_perf_map, process.pid) stdout, stderr = process.communicate() @@ -301,12 +281,9 @@ def perf_command_works(): except (subprocess.SubprocessError, OSError): return False - # perf version does not return a version number on Fedora. Use presence - # of "perf.data" in help as indicator that it's perf from Linux tools. if "perf.data" not in stdout: return False - # Check that we can run a simple perf run with temp_dir() as script_dir: try: output_file = script_dir + "/perf_output.perf" @@ -338,10 +315,6 @@ def perf_command_works(): def run_perf(cwd, *args, use_jit=False, **env_vars): - # This helper function runs perf and stores its output files *inside* - # the 'cwd' (which is a temp_dir()). This is good! It means - # those files (`perf_output.perf`, `jit_output.dump`) - # are automatically cleaned up when the temp_dir() context manager exits. env = os.environ.copy() if env_vars: env.update(env_vars) @@ -392,7 +365,6 @@ def run_perf(cwd, *args, use_jit=False, **env_vars): if proc.returncode: print(proc.stderr, file=sys.stderr) raise ValueError(f"Perf failed with return code {proc.returncode}") - # Copy the jit_output_file to the output_file os.rename(jit_output_file, output_file) base_cmd = ("perf", "script") @@ -468,15 +440,10 @@ def baz(n): "Unwinding is unreliable with frame pointers", ) class TestPerfProfiler(unittest.TestCase, TestPerfProfilerMixin): - # - # Just like in TestPerfTrampoline, we remove the racy - # setUp/tearDown methods. - # def _cleanup_perf_map(self, pid): """ Helper to safely delete a specific perf map file. - We need this for test_pre_fork_compile. """ perf_map = pathlib.Path(f"/tmp/perf-{pid}.map") try: @@ -486,8 +453,6 @@ def _cleanup_perf_map(self, pid): pass def run_perf(self, script_dir, script, activate_trampoline=True): - # The run_perf helper handles its own temp files inside script_dir - # so we don't need to add cleanups for it here. if activate_trampoline: return run_perf(script_dir, sys.executable, "-Xperf", script) return run_perf(script_dir, sys.executable, script) @@ -540,8 +505,6 @@ def compile_trampolines_for_all_functions(): stdout=subprocess.PIPE, env=env, ) as process: - # This test also creates map files, so we clean them up - # just like in TestPerfTrampoline. self.addCleanup(self._cleanup_perf_map, process.pid) stdout, stderr = process.communicate() @@ -566,8 +529,6 @@ def compile_trampolines_for_all_functions(): self.assertIn(f"py::foo_fork:{script}", child_perf_file_contents) self.assertIn(f"py::bar_fork:{script}", child_perf_file_contents) - # Pre-compiled perf-map entries of a forked process must be - # identical in both the parent and child perf-map files. perf_file_lines = perf_file_contents.split("\n") for line in perf_file_lines: if f"py::foo_fork:{script}" in line or f"py::bar_fork:{script}" in line: @@ -575,12 +536,6 @@ def compile_trampolines_for_all_functions(): def _is_perf_version_at_least(major, minor): - # The output of perf --version looks like "perf version 6.7-3" but - # it can also be perf version "perf version 5.15.143", or even include - # a commit hash in the version string, like "6.12.9.g242e6068fd5c" - # - # PermissionError is raised if perf does not exist on the Windows Subsystem - # for Linux, see #134987 try: output = subprocess.check_output(["perf", "--version"], text=True) except (subprocess.CalledProcessError, FileNotFoundError, PermissionError): @@ -597,13 +552,6 @@ def _is_perf_version_at_least(major, minor): _is_perf_version_at_least(6, 6), "perf command may not work due to a perf bug" ) class TestPerfProfilerWithDwarf(unittest.TestCase, TestPerfProfilerMixin): - # - # We've removed the racy setUp/tearDown methods from here, too. - # The `run_perf` function (with use_jit=True) already creates its - # .dump and .so files inside the `temp_dir()`, which gets - # cleaned up automatically. The old setUp/tearDown were - # trying to clean up /tmp, which was buggy and unreliable. - # def run_perf(self, script_dir, script, activate_trampoline=True): if activate_trampoline: @@ -612,10 +560,6 @@ def run_perf(self, script_dir, script, activate_trampoline=True): ) return run_perf(script_dir, sys.executable, script, use_jit=True) - # - # No setUp or tearDown needed! - # - if __name__ == "__main__": unittest.main() From 7fbbfdecad07c1af3ca5e65231d415d0b86fb922 Mon Sep 17 00:00:00 2001 From: Paresh Joshi Date: Sat, 15 Nov 2025 18:39:56 +0000 Subject: [PATCH 3/4] gh-141592: final polish per review (docstring, unlink, with, comments) Signed-off-by: Paresh Joshi --- Lib/test/test_perf_profiler.py | 43 ++++++++++++++-------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/Lib/test/test_perf_profiler.py b/Lib/test/test_perf_profiler.py index af2952d5eafb6e..30948ab580ca61 100644 --- a/Lib/test/test_perf_profiler.py +++ b/Lib/test/test_perf_profiler.py @@ -11,7 +11,7 @@ assert_python_failure, assert_python_ok, ) -from test.support.os_helper import temp_dir +from test.support.os_helper import temp_dir, unlink if not support.has_subprocess_support: @@ -37,15 +37,8 @@ def supports_trampoline_profiling(): class TestPerfTrampoline(unittest.TestCase): def _cleanup_perf_map(self, pid): - """ - Helper to safely delete a specific perf map file. - """ - perf_map = pathlib.Path(f"/tmp/perf-{pid}.map") - try: - if perf_map.exists(): - perf_map.unlink() - except OSError: - pass + """Helper to safely delete a specific perf map file.""" + unlink(f"/tmp/perf-{pid}.map") @unittest.skipIf(support.check_bolt_optimized(), "fails on BOLT instrumented binaries") def test_trampoline_works(self): @@ -65,17 +58,14 @@ def baz(): script = make_script(script_dir, "perftest", code) env = {**os.environ, "PYTHON_JIT": "0"} - process = subprocess.Popen( + with subprocess.Popen( [sys.executable, "-Xperf", script], text=True, stderr=subprocess.PIPE, stdout=subprocess.PIPE, env=env, - ) - - self.addCleanup(self._cleanup_perf_map, process.pid) - - with process: + ) as process: + self.addCleanup(self._cleanup_perf_map, process.pid) stdout, stderr = process.communicate() self.assertEqual(stderr, "") @@ -281,6 +271,8 @@ def perf_command_works(): except (subprocess.SubprocessError, OSError): return False + # perf version does not return a version number on Fedora. Use presence + # of "perf.data" in help as indicator that it's perf from Linux tools. if "perf.data" not in stdout: return False @@ -442,15 +434,8 @@ def baz(n): class TestPerfProfiler(unittest.TestCase, TestPerfProfilerMixin): def _cleanup_perf_map(self, pid): - """ - Helper to safely delete a specific perf map file. - """ - perf_map = pathlib.Path(f"/tmp/perf-{pid}.map") - try: - if perf_map.exists(): - perf_map.unlink() - except OSError: - pass + """Helper to safely delete a specific perf map file.""" + unlink(f"/tmp/perf-{pid}.map") def run_perf(self, script_dir, script, activate_trampoline=True): if activate_trampoline: @@ -529,6 +514,8 @@ def compile_trampolines_for_all_functions(): self.assertIn(f"py::foo_fork:{script}", child_perf_file_contents) self.assertIn(f"py::bar_fork:{script}", child_perf_file_contents) + # Pre-compiled perf-map entries of a forked process must be + # identical in both the parent and child perf-map files. perf_file_lines = perf_file_contents.split("\n") for line in perf_file_lines: if f"py::foo_fork:{script}" in line or f"py::bar_fork:{script}" in line: @@ -536,6 +523,12 @@ def compile_trampolines_for_all_functions(): def _is_perf_version_at_least(major, minor): + # The output of perf --version looks like "perf version 6.7-3" but + # it can also be perf version "perf version 5.15.143", or even include + # a commit hash in the version string, like "6.12.9.g242e6068fd5c" + # + # PermissionError is raised if perf does not exist on the Windows Subsystem + # for Linux, see #134987 try: output = subprocess.check_output(["perf", "--version"], text=True) except (subprocess.CalledProcessError, FileNotFoundError, PermissionError): From d14cac16e5d8064873f9a4f005d7bda438a45582 Mon Sep 17 00:00:00 2001 From: Paresh Joshi Date: Sat, 15 Nov 2025 19:23:15 +0000 Subject: [PATCH 4/4] gh-141592: final: setUp cleanup, pathlib, no extra lines Signed-off-by: Paresh Joshi --- Lib/test/test_perf_profiler.py | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_perf_profiler.py b/Lib/test/test_perf_profiler.py index 30948ab580ca61..3244e720993458 100644 --- a/Lib/test/test_perf_profiler.py +++ b/Lib/test/test_perf_profiler.py @@ -36,10 +36,20 @@ def supports_trampoline_profiling(): class TestPerfTrampoline(unittest.TestCase): + def setUp(self): + # Register cleanup for JIT files for all tests in this class + self.addCleanup(self._cleanup_jit_files) + def _cleanup_perf_map(self, pid): """Helper to safely delete a specific perf map file.""" unlink(f"/tmp/perf-{pid}.map") + def _cleanup_jit_files(self): + """Helper to safely delete JIT-related temp files.""" + for pattern in ("jit*.dump", "jitted-*.so"): + for path in pathlib.Path("/tmp/").glob(pattern): + unlink(path) + @unittest.skipIf(support.check_bolt_optimized(), "fails on BOLT instrumented binaries") def test_trampoline_works(self): code = """if 1: @@ -57,7 +67,6 @@ def baz(): with temp_dir() as script_dir: script = make_script(script_dir, "perftest", code) env = {**os.environ, "PYTHON_JIT": "0"} - with subprocess.Popen( [sys.executable, "-Xperf", script], text=True, @@ -276,6 +285,7 @@ def perf_command_works(): if "perf.data" not in stdout: return False + # Check that we can run a simple perf run with temp_dir() as script_dir: try: output_file = script_dir + "/perf_output.perf" @@ -372,6 +382,10 @@ def run_perf(cwd, *args, use_jit=False, **env_vars): class TestPerfProfilerMixin: + def setUp(self): + # This setUp is added to make super() calls in child classes work smoothly. + pass + def run_perf(self, script_dir, perf_mode, script): raise NotImplementedError() @@ -431,11 +445,11 @@ def baz(n): is_unwinding_reliable_with_frame_pointers(), "Unwinding is unreliable with frame pointers", ) -class TestPerfProfiler(unittest.TestCase, TestPerfProfilerMixin): +class TestPerfProfiler(TestPerfTrampoline, TestPerfProfilerMixin): - def _cleanup_perf_map(self, pid): - """Helper to safely delete a specific perf map file.""" - unlink(f"/tmp/perf-{pid}.map") + def setUp(self): + super().setUp() + TestPerfProfilerMixin.setUp(self) def run_perf(self, script_dir, script, activate_trampoline=True): if activate_trampoline: @@ -495,7 +509,6 @@ def compile_trampolines_for_all_functions(): self.assertEqual(process.returncode, 0) self.assertNotIn("Error:", stderr) - child_pid = int(stdout.strip()) self.addCleanup(self._cleanup_perf_map, child_pid) @@ -544,7 +557,13 @@ def _is_perf_version_at_least(major, minor): @unittest.skipUnless( _is_perf_version_at_least(6, 6), "perf command may not work due to a perf bug" ) -class TestPerfProfilerWithDwarf(unittest.TestCase, TestPerfProfilerMixin): +class TestPerfProfilerWithDwarf(TestPerfTrampoline, TestPerfProfilerMixin): + + def setUp(self): + # This calls TestPerfTrampoline.setUp() which registers jit_cleanup + super().setUp() + # This calls TestPerfProfilerMixin.setUp() + TestPerfProfilerMixin.setUp(self) def run_perf(self, script_dir, script, activate_trampoline=True): if activate_trampoline: