Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 64 additions & 13 deletions Lib/test/test_threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -463,20 +463,71 @@ def catch_exception():

self.assertIsInstance(caught_exception, InjectedException)

@requires_subprocess()

Copy link
Copy Markdown
Member

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.addaudithook and thus to a subprocess.

Copy link
Copy Markdown
Author

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

def test_limbo_cleanup(self):
# Issue 7481: Failure to start thread should cleanup the limbo map.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is this lock for?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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
Expand Down
8 changes: 5 additions & 3 deletions Lib/threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -1141,9 +1141,11 @@ def start(self):
# Start joinable thread
_start_joinable_thread(self._bootstrap, handle=self._os_thread_handle,
daemon=self.daemon)
except Exception:
with _active_limbo_lock:
del _limbo[self]
except BaseException:
# The native call can be interrupted after creating the OS thread.
if not self._os_thread_handle.ident:
with _active_limbo_lock:
del _limbo[self]
raise
self._started.wait() # Will set ident and native_id

Expand Down
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`.
Loading