Repository navigation
gh-160864: Clean up refused native thread starts - #160865
GriffinCanCode wants to merge 4 commits into
Conversation
ASan hit pythongh-160334 in the unchanged collapsed-stack profiler test. The approved fix is tracked in pythongh-160542. Retry the same source because this account cannot rerun upstream jobs.
NekoAsakura
added
the
AI-needs-explanation
|
Thank you for your interest in contributing to Python. This is a community-driven project, and we want it to be a welcoming place to learn and collaborate. Contributors are responsible for reviewing and understanding what they submit, checking its claims, and being able to explain changes in their own words. This helps reviewers focus on the contribution and leaves more time to support other contributors. This contribution appears to have been authored with AI assistance. Please explain your contribution in your own words and describe how you checked its correctness. Please read our guidelines for using AI tools. |
NekoAsakura
left a comment
NekoAsakura
left a comment
There was a problem hiding this comment.
Specifically, you are responsible for what is in this PR, and changes are expected to be minimal and focused.
|
|
||
| self.assertIsInstance(caught_exception, InjectedException) | ||
|
|
||
| @requires_subprocess() |
There was a problem hiding this comment.
Please explain why this test had to switch to sys.addaudithook and thus to a subprocess.
There was a problem hiding this comment.
We (GPT and I) used the audit hook to reproduce refusal through the real native call. The subprocess isolates the hook because there’s no public API to remove it afterward. But yea, on second look, I see that isn’t necessary to test the exception handler. Fair point
|
|
||
| @requires_subprocess() | ||
| def test_limbo_cleanup(self): | ||
| # Issue 7481: Failure to start thread should cleanup the limbo map. |
There was a problem hiding this comment.
I can't think of a reason to touch this line, other than making the diff look busier.
There was a problem hiding this comment.
It was incidental grammar cleanup, you're right and we should revert, my apologies.
| completed = threading.Event() | ||
| thread = threading.Thread(target=completed.set) | ||
| # Hold the child before it signals successful startup. | ||
| with thread._started._cond: |
There was a problem hiding this comment.
The lock makes the post-native test deterministic. The child must acquire thread._started._cond inside Event.set() before bootstrap removes its _limbo entry.
So holding it keeps the child there while the parent’s profiler raises on the native call’s return. The test can then check that the registration was preserved.
Releasing the lock lets the child finish, after which the test joins it and checks for bootstrap errors.
Fixes #160864.
Remove a thread from
_limbowhen the native-start call refuses creation with aBaseException, includingKeyboardInterruptandSystemExit. The original exception still propagates.Check the native handle before cleanup. An exception can arrive as the native call returns after creating an OS thread; that thread must keep its registration until bootstrap consumes it. The handle check runs only on the exceptional path.
Tests exercise real audit-hook refusal for
RuntimeError,KeyboardInterruptandSystemExit, plus a profiler interruption immediately after native creation. The latter checks that the child still completes without bootstrap errors.This leaves context initialization and child-bootstrap failures outside the scope of this fix. Related work in #140746 and #153776 addresses failures after OS-thread creation.
Validation:
BaseExceptioncleanup fails the post-native test with a child-bootstrapKeyError.test_chown_gidand three pathlibtest_glob_dotdotassertions; all four reproduce with originalthreading.py. Threading, thread pools and subprocess tests pass, as do the multiprocessing modules that ran.All four changed executable lines were profiled, including both guard outcomes. The successful path executes the same 60 Python instructions. Timing on the busy macOS host was noisy: normal-start CPU medians rose 3.3% initially and 0.4% in one bounded follow-up; wall medians rose 50.1% and 3.5%. I am not claiming a speedup. Refused-start CPU medians were 2.858 to 2.807 microseconds for
RuntimeError, 2.657 to 2.897 forKeyboardInterrupt, and 2.564 to 2.914 forSystemExit; the latter two now perform cleanup that the baseline omits.After 1,000 real audit-hook refusals, the baseline retained 1,000 thread objects and the fix retained none. Tracemalloc recorded 2,449,950 versus 88,960 current bytes, including the harness's weak-reference bookkeeping.
AI tools helped investigate the failure, prepare the patch and tests, and run the checks.