gh-141592: test_perf_profiler: fix race in perf map cleanup using addCleanup by pareshjoshij · Pull Request #141593 · python/cpython

pareshjoshij · 2025-11-15T14:49:20Z

The test suite in Lib/test/test_perf_profiler.py previously used setUp/tearDown to scan /tmp for perf-*.map files and delete any that appeared after the test started.

This creates a race condition when tests run in parallel (e.g., make -j, CI, pytest-xdist): one test deletes map files belonging to another, leading to:

Flaky test failures
Leftover files in /tmp
Unreliable CI behavior

Fix

Removed global /tmp scanning in setUp/tearDown
Added addCleanup(self._cleanup_perf_map, pid) for every subprocess PID
Applied consistently across:
- TestPerfTrampoline
- TestPerfProfiler
- test_pre_fork_compile
run_perf() already uses temp_dir() → no cleanup needed

Testing

Verified in GitHub Codespaces (Linux, India/IST):

./configure --with-pydebug
make -j$(nproc)
make test TESTOPTS="-v test_perf_profiler"

All 4 tests passed
No leftover perf-*.map files in /tmp
Safe under parallel execution

Issue: test_perf_profiler: race condition when cleaning up perf-*.map files #141592

…ng addCleanup Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>

python-cla-bot · 2025-11-15T14:49:24Z

All commit authors signed the Contributor License Agreement.

bedevere-app · 2025-11-15T14:49:26Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

pareshjoshij · 2025-11-15T14:57:31Z

@bedevere-bot This is a test-only change (fixes race in test_perf_profiler.py). No user-facing impact. skip news should apply.

picnixz

There are unnecessary comments, especially those about "buggy" methods. If those method don't exist, don't refer to them and don't say they are buggy. They are not really buggy in some sense, they just don't support parallel execution.

picnixz · 2025-11-15T15:06:13Z

+    #
+    # 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.
+    #


This comment should not be kept. It refers to something that doesn't exist anymore.

picnixz · 2025-11-15T15:06:23Z

+        We register this with addCleanup() to make sure it runs
+        even if the test assertions fail.


This part of the docstring is unnecessary.

picnixz · 2025-11-15T15:06:40Z

            env = {**os.environ, "PYTHON_JIT": "0"}
-            with subprocess.Popen(
+
+            # Start the subprocess...


This is unnecessary.

picnixz · 2025-11-15T15:07:01Z

+            # ...and immediately register a cleanup task for its PID.
+            # This is guaranteed to run after the test.


This comment is not necessary.

picnixz · 2025-11-15T15:07:09Z

                def foo():
                    pid = os.fork()
                    if pid == 0:
+                        # We're in the child process


Please avoid adding unnecessary redundant comments.

picnixz · 2025-11-15T15:07:22Z

+    #
+    # No setUp or tearDown needed!
+    #


Remove this comment. It's not necessary.

bedevere-app · 2025-11-15T15:08:04Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>

pareshjoshij · 2025-11-15T15:34:54Z

Done, @picnixz!

Removed all explanatory comments
No references to "buggy" or "racy" methods
No inline comments
Clean, minimal test code only

I have made the requested changes; please review again.

bedevere-app · 2025-11-15T15:35:00Z

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

…mments) Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>

pareshjoshij · 2025-11-15T18:44:00Z

Done, @picnixz!

Fixed docstring spacing
Used os_helper.unlink()
All Popen use with + addCleanup in body
Restored all non-trivial comments

I have made the requested changes; please review again.

bedevere-app · 2025-11-15T18:44:03Z

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

picnixz · 2025-11-15T18:44:53Z

        with temp_dir() as script_dir:
            script = make_script(script_dir, "perftest", code)
            env = {**os.environ, "PYTHON_JIT": "0"}
+


You can now remove the extra blank line.

picnixz · 2025-11-15T18:45:07Z

    if "perf.data" not in stdout:
        return False

-    # Check that we can run a simple perf run


Keep existing comments since they are unrelated to the PR.

picnixz · 2025-11-15T18:46:00Z


        self.assertEqual(process.returncode, 0)
        self.assertNotIn("Error:", stderr)
+


Remove this blank line (IOW, don't change blank lines)

picnixz · 2025-11-15T18:46:54Z

-        self.perf_files = set(pathlib.Path("/tmp/").glob("jit*.dump"))
-        self.perf_files |= set(pathlib.Path("/tmp/").glob("jitted-*.so"))


Those files are no more deleted. So the same should be done for jit-related files.

pareshjoshij · 2025-11-15T19:25:47Z

Done, @picnixz!

Removed all extra blank lines
Restored unrelated comments
Used setUp() + addCleanup() for JIT file cleanup
Used pathlib.Path().glob() + unlink() (no glob import)
TestPerfProfiler and TestPerfProfilerWithDwarf inherit cleanup via TestPerfTrampoline

I have made the requested changes; please review again.

bedevere-app · 2025-11-15T19:25:51Z

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

picnixz

I see many new lines. What happened? was it an AI-aided PR?

bedevere-app · 2025-11-15T19:26:29Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>

pareshjoshij · 2025-11-15T19:41:21Z

@picnixz You were right to ask, my apologies. I did use AI help to refactor the cleanup logic, and it inserted all those extra newlines.

In my hurry to push the fix, I accidentally pushed that working file instead of the final version I had corrected myself.

I've just pushed a fix by amending the last commit. This new file is the correct one, and it's clean. Sorry for the noise.
I have made the requested changes; please review again

bedevere-app · 2025-11-15T19:41:26Z

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

picnixz

This change is incorrect. Now all tests inherit from the same test and we have multiple tests being executed redundantly. In addition, the JIT files should only be removed if we use -Xperf_jit and not for the other runs.

Please read https://devguide.python.org/getting-started/generative-ai/ about our policy on using AI.

picnixz · 2025-11-15T22:38:00Z

+        """Helper to safely delete a specific perf map file."""
+        unlink(f"/tmp/perf-{pid}.map")
+
+    def _cleanup_jit_files(self):


Why are we clearing JIT files when testing PerfTrampoline?

picnixz · 2025-11-15T22:38:51Z

-        if activate_trampoline:
-            return run_perf(script_dir, sys.executable, "-Xperf", script)
-        return run_perf(script_dir, sys.executable, script)
+class TestPerfProfiler(TestPerfTrampoline, TestPerfProfilerMixin):


Why is thius one also a perf trampoline check?

bedevere-app · 2025-11-15T22:46:02Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

pythongh-141592: test_perf_profiler: fix race in perf map cleanup usi…

2f3e277

…ng addCleanup Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>

bedevere-app Bot added awaiting review tests Tests in the Lib/test dir labels Nov 15, 2025

bedevere-app Bot mentioned this pull request Nov 15, 2025

test_perf_profiler: race condition when cleaning up perf-*.map files #141592

Open

picnixz added the skip news label Nov 15, 2025

picnixz requested changes Nov 15, 2025

View reviewed changes

bedevere-app Bot added awaiting changes and removed awaiting review labels Nov 15, 2025

pythongh-141592: remove all explanatory comments per review

c56afcd

Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>

bedevere-app Bot added awaiting change review and removed awaiting changes labels Nov 15, 2025

bedevere-app Bot requested a review from picnixz November 15, 2025 15:35

picnixz reviewed Nov 15, 2025

View reviewed changes

pythongh-141592: final polish per review (docstring, unlink, with, co…

7fbbfde

…mments) Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>

bedevere-app Bot requested a review from picnixz November 15, 2025 18:44

picnixz reviewed Nov 15, 2025

View reviewed changes

bedevere-app Bot requested a review from picnixz November 15, 2025 19:25

picnixz requested changes Nov 15, 2025

View reviewed changes

bedevere-app Bot added awaiting changes and removed awaiting change review labels Nov 15, 2025

pythongh-141592: final: setUp cleanup, pathlib, no extra lines

d14cac1

Signed-off-by: Paresh Joshi <rahulj9223@gmail.com>

pareshjoshij force-pushed the fix-perf-profiler-cleanup branch from b0365b4 to d14cac1 Compare November 15, 2025 19:38

bedevere-app Bot added awaiting change review and removed awaiting changes labels Nov 15, 2025

bedevere-app Bot requested a review from picnixz November 15, 2025 19:41

Merge branch 'main' into fix-perf-profiler-cleanup

339209b

picnixz requested changes Nov 15, 2025

View reviewed changes

bedevere-app Bot added awaiting changes and removed awaiting change review labels Nov 15, 2025

pareshjoshij closed this Nov 16, 2025

pareshjoshij deleted the fix-perf-profiler-cleanup branch November 16, 2025 02:43

		We register this with addCleanup() to make sure it runs
		even if the test assertions fail.

		# ...and immediately register a cleanup task for its PID.
		# This is guaranteed to run after the test.


		self.assertEqual(process.returncode, 0)
		self.assertNotIn("Error:", stderr)

		self.perf_files = set(pathlib.Path("/tmp/").glob("jit*.dump"))
		self.perf_files \|= set(pathlib.Path("/tmp/").glob("jitted-*.so"))

Uh oh!

Conversation

pareshjoshij commented Nov 15, 2025 • edited by picnixz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Testing

Uh oh!

python-cla-bot Bot commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app Bot commented Nov 15, 2025

Uh oh!

pareshjoshij commented Nov 15, 2025

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Nov 15, 2025

Uh oh!

pareshjoshij commented Nov 15, 2025

Uh oh!

bedevere-app Bot commented Nov 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pareshjoshij commented Nov 15, 2025

Uh oh!

bedevere-app Bot commented Nov 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Nov 15, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

pareshjoshij commented Nov 15, 2025

Uh oh!

bedevere-app Bot commented Nov 15, 2025

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Nov 15, 2025

Uh oh!

pareshjoshij commented Nov 15, 2025

Uh oh!

bedevere-app Bot commented Nov 15, 2025

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented Nov 15, 2025

Uh oh!

Reviewers

Assignees

Labels

pareshjoshij commented Nov 15, 2025 •

edited by picnixz

Loading

python-cla-bot Bot commented Nov 15, 2025 •

edited

Loading

picnixz Nov 15, 2025 •

edited

Loading