gh-116915: Make _thread._ThreadHandle support GC

[mpage]

Even though it has no internal references to Python objects it still has a reference to its type by virtue of being a heap type. We need to provide a traverse function that visits the type, but we do not need to provide a clear function.

• Issue: test_capi leaks references #116915

[Fidget-Spinner]

Would a fix that only decrefs the type in dealloc be sufficient, without supporting GC? That could save some GC cycles.

https://docs.python.org/3/c-api/typeobj.html#c.Py_TPFLAGS_HEAPTYPE

[mpage]

Would a fix that only decrefs the type in dealloc be sufficient, without supporting GC?

Unfortunately, no. That decref was already present.

[mpage]

The CIFuzz (memory) failure is unrelated to this PR (gh-116886)

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @colesbury for commit 2ef088d 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

[Fidget-Spinner]

Would a fix that only decrefs the type in dealloc be sufficient, without supporting GC?

Unfortunately, no. That decref was already present.

Huh that's really strange. Usually cycles between types and the object itself are special. Will think about this.

[Fidget-Spinner]

Ah right I forgot that heap types need to support GC as well. This can be seen in previous work. In this case, this LGTM.

E.g.
https://github.com/python/cpython/pull/23136/files

[pitrou]

+1. Let's wait for the refleaks builders to be become green again and then merge this.

[Fidget-Spinner]

It seems there is indeed a reference cycle between heap types and the module object itself

cpython/Objects/typeobject.c

Line 4557 in 2982bdb

res->ht_module = Py_XNewRef(module);

.

[colesbury]

Windows 11:

https://buildbot.python.org/all/#/builders/1214/builds/304/steps/4/logs/stdio

======================================================================
FAIL: test_finalize_running_thread (test.test_threading.ThreadTests.test_finalize_running_thread)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "b:\uildarea\pull_request.ware-win11.refleak\build\Lib\test\test_threading.py", line 459, in test_finalize_running_thread
    self.assertEqual(rc, 42)
    ~~~~~~~~~~~~~~~~^^^^^^^^
AssertionError: 3221225477 != 42
----------------------------------------------------------------------
Ran 207 tests in 34.247s

3221225477 is 0xC0000005 or STATUS_ACCESS_VIOLATION

cpython/Lib/test/test_threading.py

Lines 424 to 459 in 2982bdb

	def test_finalize_running_thread(self):
	# Issue 1402: the PyGILState_Ensure / _Release functions may be called
	# very late on python exit: on deallocation of a running thread for
	# example.
	if support.check_sanitizer(thread=True):
	# the thread running `time.sleep(100)` below will still be alive
	# at process exit
	self.skipTest("TSAN would report thread leak")
	import_module("ctypes")
	rc, out, err = assert_python_failure("-c", """if 1:
	import ctypes, sys, time, _thread
	# This lock is used as a simple event variable.
	ready = _thread.allocate_lock()
	ready.acquire()
	# Module globals are cleared before __del__ is run
	# So we save the functions in class dict
	class C:
	ensure = ctypes.pythonapi.PyGILState_Ensure
	release = ctypes.pythonapi.PyGILState_Release
	def __del__(self):
	state = self.ensure()
	self.release(state)
	def waitingThread():
	x = C()
	ready.release()
	time.sleep(100)
	_thread.start_new_thread(waitingThread, ())
	ready.acquire() # Be sure the other thread is waiting.
	sys.exit(42)
	""")
	self.assertEqual(rc, 42)

[vstinner]

LGTM. I looked at my checkbox: https://pythondev.readthedocs.io/subinterpreters.html#convert-static-type-to-heap-type

[vstinner]

@colesbury: I cannot reproduce the crash on Windows 11.

vstinner@WIN C:\victor\python\main>python -m test test_threading -m test_finalize_running_thread -R 3:200
Running Debug|x64 interpreter...
Using random seed: 3966998252
0:00:00 Run 1 test sequentially
0:00:00 [1/1] test_threading
beginning 203 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:45678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123
XX1 ........................................................................................................................................................................................................
test_threading passed in 1 min 17 sec

== Tests result: SUCCESS ==

1 test OK.

Total duration: 1 min 17 sec
Total tests: run=1 (filtered)
Total test files: run=1/1 (filtered)
Result: SUCCESS

I stressed the test:

• Windows: 1 terminal: python -m test.bisect_cmd -o bisect test_threading -R 3:3 -v
• Windows: 2 terminals: python -m test test_threading -m test_finalize_running_thread -R 3:200
• Windows: 2 terminals: python loop.py -- custom script running the test_finalize_running_thread() script in a loop
• Linux: ./python -m test test_threading -m test_finalize_running_thread -F -j50

I tried for 15-20 minutes, without reproducing the crash :-(

[vstinner]

Nice fix @mpage. I confirm that it does fix the leak: #116915 (comment)

[vstinner]

If anyone see the test_threading.test_finalize_running_thread() crash again, please open a new issue.