test_weakref.test_threaded_weak_key_dict_deepcopy crash: merged objects should have ob_tid == 0

Bug report

0:08:41 load avg: 0.10 [402/491/1] test_weakref worker non-zero exit code (Exit code 2147483651) -- running (7): test_largefile (2 min 38 sec), test_zipimport (2 min 25 sec), test_compileall (2 min 14 sec), test_dbm (1 min 54 sec), test_free_threading (2 min 32 sec), test_regrtest (1 min 36 sec), test_pathlib (2 min 44 sec)
...
test.test_weakref.MappingTestCase.test_threaded_weak_key_dict_deepcopy) ... C:\Users\Administrator\buildarea\pull_request.itamaro-win64-srv-22-aws.x64.nogil\build\Python\gc_free_threading.c:1081: validate_refcounts: Assertion "op->ob_tid == 0" failed: merged objects should have ob_tid == 0
Enable tracemalloc to get the memory block allocation traceback

object address  : 0000020002444810
object refcount : 0
object type     : 00007FFC8D8A1620
object type name: weakref.ReferenceType
object repr     : <refcnt 0 at 0000020002444810>

Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed
Python runtime state: initialized

Stack (most recent call first):
  File "C:\Users\Administrator\buildarea\pull_request.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\test\test_weakref.py", line 1999 in pop_and_collect
  File "C:\Users\Administrator\buildarea\pull_request.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\threading.py", line 1016 in run
  File "C:\Users\Administrator\buildarea\pull_request.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\threading.py", line 1074 in _bootstrap_inner
  File "C:\Users\Administrator\buildarea\pull_request.itamaro-win64-srv-22-aws.x64.nogil\build\Lib\threading.py", line 1036 in _bootstrap

Looks like it happens during a gc.collect() call:

cpython/Lib/test/test_weakref.py

Lines 1992 to 1999 in 1ffe913

    
           def pop_and_collect(lst): 
        
               gc_ctr = 0 
        
               while lst: 
        
                   i = random.randint(0, len(lst) - 1) 
        
                   gc_ctr += 1 
        
                   lst.pop(i) 
        
                   if gc_ctr % 10000 == 0: 
        
                       gc.collect()  # just in case

https://buildbot.python.org/#/builders/1295/builds/591

The test passed on retry, but this seems like a bug.

Seen in main (3.15) on Windows debug build with free threading

Linked PRs

Seen in main (3.15) on Windows debug build with free threading

The bug was also seen on the 3.14 branch on Windows with Free Threading: https://buildbot.python.org/#/builders/1717/builds/206

@nascheme

The trashcan mechanism is broken in the 3.14 free threaded build. _PyTrash_thread_deposit_object overwrites the ob_tid in the free threading build, but that's not safe if the object is still tracked by the GC.

cpython/Objects/object.c

Lines 3166 to 3176 in 21bac3a

    
           void 
        
           _Py_Dealloc(PyObject *op) 
        
           { 
        
               PyTypeObject *type = Py_TYPE(op); 
        
               destructor dealloc = type->tp_dealloc; 
        
               PyThreadState *tstate = _PyThreadState_GET(); 
        
               intptr_t margin = _Py_RecursionLimit_GetMargin(tstate); 
        
               if (margin < 2) { 
        
                   _PyTrash_thread_deposit_object(tstate, (PyObject *)op); 
        
                   return; 
        
               }

(from #132280)

We're only seeing this currently on Windows probably because of the smaller stack size.

cc @nascheme @markshannon

pythongh-135106: Randomly return `1` in _Py_RecursionLimit_GetMargin

I think this is also broken in the default build. If you change _Py_RecursionLimit_GetMargin to occasionally return 1 as in #135589 you get lots of crashes.

How does the FT GC use ob_tid?
ob_tid should only be overwritten when the refcount reaches zero, so the GC shouldn't be looking at the object.

Another issue is that we now can trigger arbitrary code from Py_DECREF() on types like PyLongObject if there's trash in tstate->delete_later whereas previously we assumed that these destructors had constrained behavior.

I think we should revert it sooner rather than later unless there are easy fixes to the problems.

I'm not sure there are easy fixes. The objects that previously used Py_TRASHCAN_BEGIN would expect that the object is untracked when executing the rest of their dealloc function. However, now the trashcan applies to all "container" objects, we don't know if the dealloc function expects that or not. Should _PyTrash_thread_destroy_chain should re-track or not?

My old PR (GH-124716) untracked in the deposit and didn't re-track. That made the change a fair bit more complex than GH-132280. I think it is more logical that Py_Dealloc untracks automatically but I suspect that could could break some extension types.

The objects are still visible to the GC and may be treated as cyclic trash, which then corrupts the reference count field.

If we untrack in deposit, this is solved, right?

[a deposited] object can still be accessed via weakrefs

Ugh, I don't think we want to clear the weakrefs when we deposit. If we untrack the object, we could make the trashcan chain use _PyGCHead_SET_PREV(), at least in the default build. All of this doesn't sound very easy, at least in terms of putting it in a beta.

The objects that previously used Py_TRASHCAN_BEGIN would expect that the object is untracked when executing the rest of their dealloc function. However, now the trashcan applies to all "container" objects, we don't know if the dealloc function expects that or not.

If we allow _PyObject_GC_UNTRACK() to be called on an already untracked object, things should mostly work if deposit untracks. Types that previously used Py_TRASHCAN_BEGIN must call untrack first and will expect that the object is untracked in the body. So those are okay.

Container objects that don't explicitly use the trashcan should untrack before tearing down the object. There could be an issue if a type resurrects an object and therefore doesn't expect that dealloc is called with the object already untracked.

Weakrefs are a pain, and why we can't thread the destroy chain through objects 😞

I'd like to keep the general approach of using the stack limits to determine when to use the trashcan as that is key for performance.

Once we've hit the trashcan, performance is less important, so adding a _PyObject_IS_GC check should be fine.
With a _PyObject_IS_GC check we can go back to only putting GC-able objects in the trashcan and using the GC fields, not the object header for the linked list.
We'll need to untrack the objects to put them in the list and track them again before cleaning them up.

I'll make a PR later in the week if no one beats me to it.

GH-135106: Restrict trashcan to GC'ed objects (GH-135682)

pythonGH-135106: Restrict trashcan to GC'ed objects (pythonGH-135682)

[3.14] GH-135106: Restrict trashcan to GC'ed objects (GH-135682) (#13…

Closing, this has been fixed and backported, thanks all. Please re-open if necessary.

pythonGH-135106: Restrict trashcan to GC'ed objects (pythonGH-135682)

Revert "[3.14] pythonGH-135106: Restrict trashcan to GC'ed objects (p…

Navigation Menu

Repository navigation

`test_weakref.test_threaded_weak_key_dict_deepcopy` crash: merged objects should have ob_tid == 0 #135106

Bug report

Linked PRs

3 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

	def pop_and_collect(lst):
	gc_ctr = 0
	while lst:
	i = random.randint(0, len(lst) - 1)
	gc_ctr += 1
	lst.pop(i)
	if gc_ctr % 10000 == 0:
	gc.collect() # just in case

	void
	_Py_Dealloc(PyObject *op)
	{
	PyTypeObject *type = Py_TYPE(op);
	destructor dealloc = type->tp_dealloc;
	PyThreadState *tstate = _PyThreadState_GET();
	intptr_t margin = _Py_RecursionLimit_GetMargin(tstate);
	if (margin < 2) {
	_PyTrash_thread_deposit_object(tstate, (PyObject *)op);
	return;
	}

test_weakref.test_threaded_weak_key_dict_deepcopy crash: merged objects should have ob_tid == 0 #135106

Description

Bug report

Linked PRs

Activity

vstinner commented on Jun 5, 2025

colesbury commented on Jun 16, 2025

colesbury commented on Jun 16, 2025

markshannon commented on Jun 17, 2025

3 remaining items

colesbury commented on Jun 17, 2025

nascheme commented on Jun 17, 2025

nascheme commented on Jun 17, 2025

markshannon commented on Jun 18, 2025

hugovk commented on Jul 7, 2025

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

`test_weakref.test_threaded_weak_key_dict_deepcopy` crash: merged objects should have ob_tid == 0 #135106