Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
a3292c2cd294a67e28cf230057a5c4deb03607237a2735a71c05db547f6bc4b0b97604948daed08e22c362afc76eab653e37fc61d4f8bda7e9d42b44b39149285980208f43c5d12102c5f8b1f2ae5cc7f786019df37f46eafd8a5b377e6d8c62fa9a69b728f71d9eba5c19de2e7dd248075b5823dd4c958d864efe26296360273916269f6837af64fea3ba39ff611556bb0d587a4859ba2bcfdb88c3f4abe20046File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -155,6 +155,72 @@ test_frozenset_add_in_capi(PyObject *self, PyObject *Py_UNUSED(obj)) | ||
| return NULL; | ||
| } | ||
| static PyObject * | ||
| raiseTestError(const char* test_name, const char* msg) | ||
sergey-miryanov marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading. Please reload this page. | ||
| { | ||
| 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", | ||
eendebakpt marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading. Please reload this page. | ||
| "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)) | ||
| { | ||
| @@ -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}, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1368,6 +1368,26 @@ make_new_set_basetype(PyTypeObject *type, PyObject *iterable) | ||
| 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)){ | ||
efimov-mikhail marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading. Please reload this page. | ||
| return; | ||
| } | ||
| } | ||
| _PyObject_GC_UNTRACK(op); | ||
| } | ||
| static PyObject * | ||
| make_new_frozenset(PyTypeObject *type, PyObject *iterable) | ||
| { | ||
| @@ -1379,7 +1399,11 @@ make_new_frozenset(PyTypeObject *type, PyObject *iterable) | ||
| /* 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 * | ||
| @@ -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 | ||
| @@ -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); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.