gh-140232: Do not track frozenset objects with immutables #140234

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

Open

eendebakpt wants to merge 46 commits into python:main from eendebakpt:frozenset_immutable_tracking

+114 −3

Lib/test/test_capi/test_set.py

-Original file line number
+Diff line change
@@ Expand Up / @@ -166,6 +166,12 @@ def test_add(self): @@
             # CRASHES: add(instance, NULL)
             # CRASHES: add(NULL, NULL)
+        def test_add_frozenset(self):
+           add = _testlimitedcapi.set_add
+           frozen_set = frozenset()
+           # test adding an element to a non-uniquely referenced frozenset throws an exception
+           self.assertRaises(SystemError, add, frozen_set, 1)
         def test_discard(self):
             discard = _testlimitedcapi.set_discard
             for cls in (set, set_subclass):
@@ Expand Down @@

Lib/test/test_sys.py

-Original file line number
+Diff line change
@@ Expand Up / @@ -1880,7 +1880,10 @@ class S(set): @@
             check(S(), set(), '3P')
             class FS(frozenset):
                 __slots__ = 'a', 'b', 'c'
-            check(FS(), frozenset(), '3P')
+            class mytuple(tuple):
+                pass
+            check(FS([mytuple()]), frozenset([mytuple()]), '3P')
             from collections import OrderedDict
             class OD(OrderedDict):
                 __slots__ = 'a', 'b', 'c'
@@ Expand Down @@

Misc/NEWS.d/next/Core_and_Builtins/2025-10-16-22-36-05.gh-issue-140232.u3srgv.rst

Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Frozenset objects with immutable elements are no longer tracked by the garbage collector.

Modules/_testlimitedcapi/set.c

-Original file line number
+Diff line change
@@ Expand Up @@
         return NULL;
     }
+    static PyObject *
+    raiseTestError(const char* test_name, const char* msg)
+    {
+        PyErr_Format(PyExc_AssertionError, "%s: %s", test_name, msg);
+        return NULL;
+    }
+    static PyObject *
+    test_frozenset_add_in_capi_tracking_immutable(PyObject *self, PyObject *Py_UNUSED(ignored))
+    {
+        // Test: GC tracking - frozenset with only immutable items should not be tracked
+        PyObject *frozenset = PyFrozenSet_New(NULL);
+        if (frozenset == NULL) {
+            return NULL;
+        }
+        if (PySet_Add(frozenset, Py_True) < 0) {
+            Py_DECREF(frozenset);
+            return NULL;
+        }
+        if (PyObject_GC_IsTracked(frozenset)) {
+            Py_DECREF(frozenset);
+            return raiseTestError("test_frozenset_add_in_capi_tracking_immutable",
+                                  "frozenset with only bool should not be GC tracked");
+        }
+        Py_DECREF(frozenset);
+        Py_RETURN_NONE;
+    }
+    static PyObject *
+    test_frozenset_add_in_capi_tracking(PyObject *self, PyObject *Py_UNUSED(ignored))
+    {
+        // Test: GC tracking - frozenset with tracked object should be tracked
+        PyObject *frozenset = PyFrozenSet_New(NULL);
+        if (frozenset == NULL) {
+            return NULL;
+        }
+        PyObject *tracked_obj = PyErr_NewException("_testlimitedcapi.py_set_add", NULL, NULL);
+        if (tracked_obj == NULL) {
+            goto error;
+        }
+        if (!PyObject_GC_IsTracked(tracked_obj)) {
+            Py_DECREF(frozenset);
+            Py_DECREF(tracked_obj);
+            return raiseTestError("test_frozenset_add_in_capi_tracking",
+                                  "test object should be tracked");
+        }
+        if (PySet_Add(frozenset, tracked_obj) < 0) {
+            goto error;
+        }
+        Py_DECREF(tracked_obj);
+        if (!PyObject_GC_IsTracked(frozenset)) {
+            Py_DECREF(frozenset);
+            return raiseTestError("test_frozenset_add_in_capi_tracking",
+                                  "frozenset with with GC tracked object should be tracked");
+        }
+        Py_DECREF(frozenset);
+        Py_RETURN_NONE;
+    error:
+        Py_DECREF(frozenset);
+        Py_XDECREF(tracked_obj);
+        return NULL;
+    }
     static PyObject *
     test_set_contains_does_not_convert_unhashable_key(PyObject *self, PyObject *Py_UNUSED(obj))
     {
@@ Expand Down Expand Up / @@ -219,6 +285,8 @@ static PyMethodDef test_methods[] = { @@
         {"set_clear", set_clear, METH_O},
         {"test_frozenset_add_in_capi", test_frozenset_add_in_capi, METH_NOARGS},
+        {"test_frozenset_add_in_capi_tracking", test_frozenset_add_in_capi_tracking, METH_NOARGS},
+        {"test_frozenset_add_in_capi_tracking_immutable", test_frozenset_add_in_capi_tracking_immutable, METH_NOARGS},
         {"test_set_contains_does_not_convert_unhashable_key",
          test_set_contains_does_not_convert_unhashable_key, METH_NOARGS},
@@ Expand Down @@

Objects/setobject.c

-Original file line number
+Diff line change
@@ Expand Up @@
         return make_new_set(type, iterable);
     }
+    // gh-140232: check whether a frozenset can be untracked from the GC
+    static void
+    _PyFrozenSet_MaybeUntrack(PyObject *op)
+    {
+        assert(op != NULL);
+        // subclasses of a frozenset can generate reference cycles, so do not untrack
+        if (!PyFrozenSet_CheckExact(op)) {
+            return;
+        }
+        // if no elements of a frozenset are tracked by the GC, we untrack the object
+        Py_ssize_t pos = 0;
+        setentry *entry;
+        while (set_next((PySetObject *)op, &pos, &entry)) {
+            if (_PyObject_GC_MAY_BE_TRACKED(entry->key)) {
+                return;
+            }
+        }
+        _PyObject_GC_UNTRACK(op);
+    }
     static PyObject *
     make_new_frozenset(PyTypeObject *type, PyObject *iterable)
     {
@@ Expand All @@
             /* frozenset(f) is idempotent */
             return Py_NewRef(iterable);
         }
-        return make_new_set(type, iterable);
+        PyObject *obj = make_new_set(type, iterable);
+        if (obj != NULL) {
+            _PyFrozenSet_MaybeUntrack(obj);
+        }
+        return obj;
     }
     static PyObject *
@@ Expand Down Expand Up / @@ -2932,7 +2956,11 @@ PySet_New(PyObject *iterable) @@
     PyObject *
     PyFrozenSet_New(PyObject *iterable)
     {
-        return make_new_set(&PyFrozenSet_Type, iterable);
+        PyObject *result = make_new_set(&PyFrozenSet_Type, iterable);
+        if (result != NULL) {
+            _PyFrozenSet_MaybeUntrack(result);
+        }
+        return result;
     }
     Py_ssize_t
@@ Expand Down Expand Up / @@ -3010,6 +3038,11 @@ PySet_Add(PyObject *anyset, PyObject *key) @@
             // API limits the usage of `PySet_Add` to "fill in the values of brand
             // new frozensets before they are exposed to other code". In this case,
             // this can be done without a lock.
+            // Since another key is added to the set, we must track the frozenset
+            // if needed.
+            if (PyFrozenSet_CheckExact(anyset) && !PyObject_GC_IsTracked(anyset) && PyObject_GC_IsTracked(key)) {
+                _PyObject_GC_TRACK(anyset);
+            }
             return set_add_key((PySetObject *)anyset, key);
         }
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

gh-140232: Do not track frozenset objects with immutables #140234

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gh-140232: Do not track frozenset objects with immutables #140234

Are you sure you want to change the base?

gh-140232: Do not track frozenset objects with immutables #140234

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!