Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions Lib/test/test_capi/test_object.py
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,80 @@ def test_pyobject_dump(self):
output = self.pyobject_dump(NULL)
self.assertRegex(output, r'<object at .* is freed>')

def test_object_clear_managed_dict(self):
# PyObject_ClearManagedDict
clear = _testcapi.pyobject_clear_managed_dict

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I would prefer to call it clear_managed_dict, since clear() sounds like a different function (like PyDict_Clear()). Same remark in the second test.

ManagedDictType = _testcapi.ManagedDictType

obj = ManagedDictType()
obj.value = 'first'
# asserting `__dict__` is a must to materialize the value before clean:
self.assertEqual(obj.__dict__, {'value': 'first'})
self.assertIs(type(obj.__dict__), dict)
Comment thread
vstinner marked this conversation as resolved.
Resolved
clear(obj)
self.assertEqual(obj.__dict__, {})
self.assertIs(type(obj.__dict__), dict)

# big object:
obj = ManagedDictType()
existing_dict = {f'attr_{x}': x for x in range(100)}
obj.__dict__ = existing_dict
self.assertIs(obj.__dict__, existing_dict)
clear(obj)
self.assertEqual(obj.__dict__, {})
self.assertIs(type(obj.__dict__), dict)

# empty dict:
obj = ManagedDictType()
self.assertEqual(obj.__dict__, {})
self.assertIs(type(obj.__dict__), dict)
clear(obj)
self.assertEqual(obj.__dict__, {})
self.assertIs(type(obj.__dict__), dict)

# double clear:
obj = ManagedDictType()
obj.value = 'first'
self.assertEqual(obj.__dict__, {'value': 'first'})
clear(obj)
clear(obj)
self.assertEqual(obj.__dict__, {})
self.assertIs(type(obj.__dict__), dict)

# unmaterialized:
obj = ManagedDictType()
obj.value = 'first'
clear(obj)
self.assertEqual(obj.__dict__, {})
self.assertIs(type(obj.__dict__), dict)

# CRASHES: clear(NULL)

@threading_helper.requires_working_threading()
def test_object_clear_managed_dict_ft_memory_error(self):
# PyObject_ClearManagedDict, gh-152254

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you write a short sentence to describe the purpose of the test? I understand that you call PyObject_ClearManagedDict() from a different thread so _Py_IsOwnedByCurrentThread() is false.

clear = _testcapi.pyobject_clear_managed_dict
ManagedDictType = _testcapi.ManagedDictType

obj = ManagedDictType()
obj.value = 'first'
self.assertEqual(len(obj.__dict__), 1)

def worker():
# This needs to be in a separate thread for the crash to happen:
with support.catch_unraisable_exception() as ex:
# `clear` used to crash:
_testcapi.call_with_nomemory(0, 1, clear, obj)
self.assertIsNotNone(ex.unraisable)
self.assertIs(ex.unraisable.exc_type, MemoryError)
self.assertIn('managed dict', ex.unraisable.err_msg)
self.assertEqual(obj.__dict__, {})
self.assertIs(type(obj.__dict__), dict)

threading_helper.run_concurrently([worker])
self.assertEqual(obj.__dict__, {})
self.assertIs(type(obj.__dict__), dict)


class RefTracerTest(unittest.TestCase):
@support.requires_resource('cpu')
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a crash during the clean up of managed dict objects under a memory
pressure in :c:func:`PyObject_ClearManagedDict`.
8 changes: 8 additions & 0 deletions Modules/_testcapi/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,13 @@ pyobject_is_unique_temporary_new_object(PyObject *self, PyObject *unused)
return PyLong_FromLong(result);
}

static PyObject *
pyobject_clear_managed_dict(PyObject *self, PyObject *obj)
{
PyObject_ClearManagedDict(obj);
Py_RETURN_NONE;
}

static int MyObject_dealloc_called = 0;

static void
Expand Down Expand Up @@ -358,6 +365,7 @@ static PyMethodDef test_methods[] = {
{"pyobject_enable_deferred_refcount", pyobject_enable_deferred_refcount, METH_O},
{"pyobject_is_unique_temporary", pyobject_is_unique_temporary, METH_O},
{"pyobject_is_unique_temporary_new_object", pyobject_is_unique_temporary_new_object, METH_NOARGS},
{"pyobject_clear_managed_dict", pyobject_clear_managed_dict, METH_O},
{"test_py_try_inc_ref", test_py_try_inc_ref, METH_NOARGS},
{"test_py_set_immortal", test_py_set_immortal, METH_NOARGS},
#ifdef Py_REF_DEBUG
Expand Down
1 change: 1 addition & 0 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -8146,6 +8146,7 @@ PyObject_ClearManagedDict(PyObject *obj)
/* Clear the dict */
Py_BEGIN_CRITICAL_SECTION(dict);
PyDictKeysObject *oldkeys = dict->ma_keys;
ensure_shared_on_resize(dict);
set_keys(dict, Py_EMPTY_KEYS);
dict->ma_values = NULL;
dictkeys_decref(oldkeys, IS_DICT_SHARED(dict));
Expand Down
Loading