Skip to content
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

Class objects become immortal and cannot be GC after creating a new thread in free-threaded Python #124239

Closed
XuehaiPan opened this issue Sep 19, 2024 · 6 comments
Labels
topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@XuehaiPan
Copy link
Contributor

XuehaiPan commented Sep 19, 2024

Bug report

Bug description:

A simple reproduce script to print the refcount when there is main thread only and after creating a new thread.

# test.py

import gc
import sys
import threading
import weakref


Py_GIL_DISABLED = 't' in getattr(sys, 'abiflags', '')


class Foo:
    pass


print(f'refcount before: {sys.getrefcount(Foo)}')


# Start a new thread
thread = threading.Thread(target=str)  # no-op
thread.start()
thread.join()


print(f'refcount after:  {sys.getrefcount(Foo)}')


wr = weakref.ref(Foo)

# Ensure class Foo is collected
del Foo
for _ in range(10):
    gc.collect()


if Py_GIL_DISABLED:
    assert wr() is not None
    print('Class Foo is not collected!')
    print('sys.getrefcount(wr()):', sys.getrefcount(wr()))
else:
    assert wr() is None
    print('Class Foo is collected!')

On Python 3.13:

$ python3 test.py
refcount before: 5
refcount after:  5
Class Foo is collected!

On Python 3.13t:

$ python3 test.py
refcount before: 6
refcount after:  4294967295
Class Foo is not collected!
sys.getrefcount(wr()): 4294967295

CPython versions tested on:

3.13

Operating systems tested on:

macOS

@XuehaiPan XuehaiPan added the type-bug An unexpected behavior, bug, or error label Sep 19, 2024
@Chen-Chang
Copy link

My friend encountered the same bug!

@ZeroIntensity
Copy link
Member

I was able to reproduce this behavior on 3.13, but this is probably a wontfix. Reference counts are, more or less, an implementation detail. On free threading, I'm guessing that type objects are immortalized to avoid reference count contention. (Types are something that are likely to be accessed across many threads, so immortalizing them allows threads to use an object without doing atomic reference count modifications.)

We have a tradeoff to make when it comes to free threading. Should we hurt the performance of threads to support some users that relying on when type objects get garbage collected?

Out of curiosity, is there something that's relying on type objects getting manually garbage collected?

@colesbury
Copy link
Contributor

This is the intended behavior in 3.13. See the following issue for a description:

In 3.14, type objects are not immortalized and are collected by the GC.

@XuehaiPan
Copy link
Contributor Author

XuehaiPan commented Sep 19, 2024

This is the intended behavior in 3.13. See the following issue for a description:

In 3.14, type objects are not immortalized and are collected by the GC.

Thanks for the information!


Out of curiosity, is there something that's relying on type objects getting manually garbage collected?

Let me share more context about this. I'm enabling free-threading support on a C extension.

There is a register function to register a Python type object into a C++ static std::unordered_map and increases the refcount. And there is also a corresponding unregister function removes the entry in std::unordered_map and decreases the refcount.

I'm trying to test whether the paired register / unregister function does not leak the memory in the C extension and the Python class object can be properly GC-ed.

The test log is here: https://github.com/metaopt/optree/actions/runs/10937390199/job/30363160663?pr=137

    def test_unregister_pytree_node_memory_leak():  # noqa: C901
    
        @optree.register_pytree_node_class(namespace=GLOBAL_NAMESPACE)
        class MyList1(UserList):
            def tree_flatten(self):
                return self.data, None, None
    
            @classmethod
            def tree_unflatten(cls, metadata, children):
                return cls(children)
    
        wr = weakref.ref(MyList1)
        assert wr() is not None
    
        optree.unregister_pytree_node(MyList1, namespace=GLOBAL_NAMESPACE)
        del MyList1
        gc_collect()
>       assert wr() is None
E       AssertionError: assert <class 'test_registry.test_unregister_pytree_node_memory_leak.<locals>.MyList1'> is None
E        +  where <class 'test_registry.test_unregister_pytree_node_memory_leak.<locals>.MyList1'> = <weakref at 0x20036242b90; to 'abc.ABCMeta' at 0x20031870010 (MyList1)>()

This test works fine in Python 3.7-3.13 and failed in 3.13t.

There is one more thing I want to point out is that the test order matters.

$ pytest tests/test_registry.py tests/test_free_threading.py  # OK

$ pytest tests/test_free_threading.py tests/test_registry.py  # FAILED

@colesbury
Copy link
Contributor

You can use the @test.support.suppress_immortalization() decorator for tests to suppress the behavior. (Or skip the refleak tests when sysconfig.get_config_var("Py_GIL_DISABLED") is 1)

@XuehaiPan
Copy link
Contributor Author

You can use the @test.support.suppress_immortalization() decorator for tests to suppress the behavior.

Thanks for the hint. I'll try it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

6 participants