Repository navigation
gh-160864: Clean up refused native thread starts #160865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
93d0581
14224f9
7de1182
785af1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -463,20 +463,71 @@ def catch_exception(): | |
|
|
||
| self.assertIsInstance(caught_exception, InjectedException) | ||
|
|
||
| @requires_subprocess() | ||
| def test_limbo_cleanup(self): | ||
| # Issue 7481: Failure to start thread should cleanup the limbo map. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't think of a reason to touch this line, other than making the diff look busier.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was incidental grammar cleanup, you're right and we should revert, my apologies. |
||
| def fail_new_thread(*args, **kwargs): | ||
| raise threading.ThreadError() | ||
| _start_joinable_thread = threading._start_joinable_thread | ||
| threading._start_joinable_thread = fail_new_thread | ||
| try: | ||
| t = threading.Thread(target=lambda: None) | ||
| self.assertRaises(threading.ThreadError, t.start) | ||
| self.assertFalse( | ||
| t in threading._limbo, | ||
| "Failed to cleanup _limbo map on failure of Thread.start().") | ||
| finally: | ||
| threading._start_joinable_thread = _start_joinable_thread | ||
| # Issue 7481: Failure to start a thread must clean up the limbo map. | ||
| code = """if 1: | ||
| import builtins | ||
| import sys | ||
| import threading | ||
|
|
||
| error = getattr(builtins, sys.argv[1])() | ||
| def audit(event, args): | ||
| if event == '_thread.start_joinable_thread': | ||
| raise error | ||
|
|
||
| sys.addaudithook(audit) | ||
| thread = threading.Thread() | ||
| try: | ||
| thread.start() | ||
| except BaseException as caught: | ||
| assert caught is error | ||
| else: | ||
| raise AssertionError('Thread.start() did not raise') | ||
|
|
||
| assert thread._os_thread_handle.ident == 0 | ||
| assert not thread._started.is_set() | ||
| assert thread not in threading._limbo | ||
| assert thread not in threading.enumerate() | ||
| """ | ||
| for exception in ('RuntimeError', 'KeyboardInterrupt', 'SystemExit'): | ||
| with self.subTest(exception=exception): | ||
| assert_python_ok('-c', code, exception) | ||
|
|
||
| @requires_subprocess() | ||
| def test_start_interrupted_after_native_start(self): | ||
| code = """if 1: | ||
| import sys | ||
| import threading | ||
| from test.support import SHORT_TIMEOUT, threading_helper | ||
|
|
||
| def interrupt(frame, event, arg): | ||
| if (event == 'c_return' | ||
| and arg is threading._start_joinable_thread): | ||
| raise KeyboardInterrupt | ||
|
|
||
| completed = threading.Event() | ||
| thread = threading.Thread(target=completed.set) | ||
| # Hold the child before it signals successful startup. | ||
| with thread._started._cond: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this lock for?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| sys.setprofile(interrupt) | ||
| try: | ||
| thread.start() | ||
| except KeyboardInterrupt: | ||
| pass | ||
| else: | ||
| raise AssertionError('Thread.start() did not raise') | ||
| finally: | ||
| sys.setprofile(None) | ||
| assert thread._os_thread_handle.ident | ||
| assert thread in threading._limbo | ||
|
|
||
| assert thread._started.wait(SHORT_TIMEOUT) | ||
| threading_helper.join_thread(thread) | ||
| assert completed.is_set() | ||
| """ | ||
| _, _, stderr = assert_python_ok('-c', code) | ||
| self.assertEqual(stderr, b'') | ||
|
|
||
| def test_finalize_running_thread(self): | ||
| # Issue 1402: the PyGILState_Ensure / _Release functions may be called | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Remove unstarted threads from the threading registry when an audit hook aborts | ||
| native thread creation with :exc:`KeyboardInterrupt` or :exc:`SystemExit`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain why this test had to switch to
sys.addaudithookand thus to a subprocess.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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