From 8cc649813748db66ea26c8604a6dd1699b03539f Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 22 Jul 2024 20:00:38 +0000 Subject: [PATCH 1/3] gh-120974: Make _asyncio._leave_task atomic in the free-threaded build Update `_PyDict_DelItemIf` to allow for an argument to be passed to the predicate. --- Include/internal/pycore_dict.h | 6 ++++-- Modules/_asynciomodule.c | 21 ++++++++++----------- Modules/_weakref.c | 13 +++---------- Objects/dictobject.c | 22 ++++++++++------------ 4 files changed, 27 insertions(+), 35 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index a4bdf0d7ad8283..3caeb417a290f9 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -14,8 +14,10 @@ extern "C" { // Unsafe flavor of PyDict_GetItemWithError(): no error checking extern PyObject* _PyDict_GetItemWithError(PyObject *dp, PyObject *key); -extern int _PyDict_DelItemIf(PyObject *mp, PyObject *key, - int (*predicate)(PyObject *value)); +// Export for '_asyncio' shared extension +PyAPI_FUNC(int) _PyDict_DelItemIf(PyObject *mp, PyObject *key, + int (*predicate)(PyObject *value, void *arg), + void *arg); // "KnownHash" variants // Export for '_asyncio' shared extension diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 372f19794be0dd..c3ede487a5b88b 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2031,18 +2031,9 @@ enter_task(asyncio_state *state, PyObject *loop, PyObject *task) return _PyDict_SetItem_KnownHash(state->current_tasks, loop, task, hash); } - static int -leave_task(asyncio_state *state, PyObject *loop, PyObject *task) -/*[clinic end generated code: output=0ebf6db4b858fb41 input=51296a46313d1ad8]*/ +leave_task_predicate(PyObject *item, void *task) { - PyObject *item; - Py_hash_t hash; - hash = PyObject_Hash(loop); - if (hash == -1) { - return -1; - } - item = _PyDict_GetItem_KnownHash(state->current_tasks, loop, hash); if (item != task) { if (item == NULL) { /* Not entered, replace with None */ @@ -2054,7 +2045,15 @@ leave_task(asyncio_state *state, PyObject *loop, PyObject *task) task, item, NULL); return -1; } - return _PyDict_DelItem_KnownHash(state->current_tasks, loop, hash); + return 1; +} + +static int +leave_task(asyncio_state *state, PyObject *loop, PyObject *task) +/*[clinic end generated code: output=0ebf6db4b858fb41 input=51296a46313d1ad8]*/ +{ + return _PyDict_DelItemIf(state->current_tasks, loop, leave_task_predicate, + task); } static PyObject * diff --git a/Modules/_weakref.c b/Modules/_weakref.c index a5c15c0f10b930..ecaa08ff60f203 100644 --- a/Modules/_weakref.c +++ b/Modules/_weakref.c @@ -31,7 +31,7 @@ _weakref_getweakrefcount_impl(PyObject *module, PyObject *object) static int -is_dead_weakref(PyObject *value) +is_dead_weakref(PyObject *value, void *unused) { if (!PyWeakref_Check(value)) { PyErr_SetString(PyExc_TypeError, "not a weakref"); @@ -56,15 +56,8 @@ _weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct, PyObject *key) /*[clinic end generated code: output=d9ff53061fcb875c input=19fc91f257f96a1d]*/ { - if (_PyDict_DelItemIf(dct, key, is_dead_weakref) < 0) { - if (PyErr_ExceptionMatches(PyExc_KeyError)) - /* This function is meant to allow safe weak-value dicts - with GC in another thread (see issue #28427), so it's - ok if the key doesn't exist anymore. - */ - PyErr_Clear(); - else - return NULL; + if (_PyDict_DelItemIf(dct, key, is_dead_weakref, NULL) < 0) { + return NULL; } Py_RETURN_NONE; } diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 7310c3c8e13b5b..f054a145f34d84 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2608,7 +2608,8 @@ _PyDict_DelItem_KnownHash(PyObject *op, PyObject *key, Py_hash_t hash) static int delitemif_lock_held(PyObject *op, PyObject *key, - int (*predicate)(PyObject *value)) + int (*predicate)(PyObject *value, void *arg), + void *arg) { Py_ssize_t ix; PyDictObject *mp; @@ -2618,10 +2619,6 @@ delitemif_lock_held(PyObject *op, PyObject *key, ASSERT_DICT_LOCKED(op); - if (!PyDict_Check(op)) { - PyErr_BadInternalCall(); - return -1; - } assert(key); hash = PyObject_Hash(key); if (hash == -1) @@ -2630,12 +2627,8 @@ delitemif_lock_held(PyObject *op, PyObject *key, ix = _Py_dict_lookup(mp, key, hash, &old_value); if (ix == DKIX_ERROR) return -1; - if (ix == DKIX_EMPTY || old_value == NULL) { - _PyErr_SetKeyError(key); - return -1; - } - res = predicate(old_value); + res = predicate(old_value, arg); if (res == -1) return -1; @@ -2655,11 +2648,16 @@ delitemif_lock_held(PyObject *op, PyObject *key, */ int _PyDict_DelItemIf(PyObject *op, PyObject *key, - int (*predicate)(PyObject *value)) + int (*predicate)(PyObject *value, void *arg), + void *arg) { + if (!PyDict_Check(op)) { + PyErr_BadInternalCall(); + return -1; + } int res; Py_BEGIN_CRITICAL_SECTION(op); - res = delitemif_lock_held(op, key, predicate); + res = delitemif_lock_held(op, key, predicate, arg); Py_END_CRITICAL_SECTION(); return res; } From 01f8052dd079755b9cb79e8b5db9c7186d1a35a4 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 22 Jul 2024 20:24:06 +0000 Subject: [PATCH 2/3] Fix handling of case where key is missing --- Include/internal/pycore_dict.h | 2 ++ Modules/_asynciomodule.c | 29 ++++++++++++++++++----------- Objects/dictobject.c | 15 ++++++++++----- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 3caeb417a290f9..fc304aca7fea10 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -14,6 +14,8 @@ extern "C" { // Unsafe flavor of PyDict_GetItemWithError(): no error checking extern PyObject* _PyDict_GetItemWithError(PyObject *dp, PyObject *key); +// Delete an item from a dict if a predicate is true +// Returns -1 on error, 1 if the item was deleted, 0 otherwise // Export for '_asyncio' shared extension PyAPI_FUNC(int) _PyDict_DelItemIf(PyObject *mp, PyObject *key, int (*predicate)(PyObject *value, void *arg), diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index c3ede487a5b88b..6fa3d1c0fe92d4 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2031,19 +2031,21 @@ enter_task(asyncio_state *state, PyObject *loop, PyObject *task) return _PyDict_SetItem_KnownHash(state->current_tasks, loop, task, hash); } +static int +err_leave_task(PyObject *item, PyObject *task) +{ + PyErr_Format( + PyExc_RuntimeError, + "Leaving task %R does not match the current task %R.", + task, item); + return -1; +} + static int leave_task_predicate(PyObject *item, void *task) { if (item != task) { - if (item == NULL) { - /* Not entered, replace with None */ - item = Py_None; - } - PyErr_Format( - PyExc_RuntimeError, - "Leaving task %R does not match the current task %R.", - task, item, NULL); - return -1; + return err_leave_task(item, (PyObject *)task); } return 1; } @@ -2052,8 +2054,13 @@ static int leave_task(asyncio_state *state, PyObject *loop, PyObject *task) /*[clinic end generated code: output=0ebf6db4b858fb41 input=51296a46313d1ad8]*/ { - return _PyDict_DelItemIf(state->current_tasks, loop, leave_task_predicate, - task); + int res = _PyDict_DelItemIf(state->current_tasks, loop, + leave_task_predicate, task); + if (res == 0) { + // task was not found + return err_leave_task(Py_None, task); + } + return res; } static PyObject * diff --git a/Objects/dictobject.c b/Objects/dictobject.c index f054a145f34d84..4bb2824c4dcf68 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2508,7 +2508,7 @@ delete_index_from_values(PyDictValues *values, Py_ssize_t ix) values->size = size; } -static int +static void delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix, PyObject *old_value, uint64_t new_version) { @@ -2550,7 +2550,6 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix, Py_DECREF(old_value); ASSERT_CONSISTENT(mp); - return 0; } int @@ -2593,7 +2592,8 @@ delitem_knownhash_lock_held(PyObject *op, PyObject *key, Py_hash_t hash) PyInterpreterState *interp = _PyInterpreterState_GET(); uint64_t new_version = _PyDict_NotifyEvent( interp, PyDict_EVENT_DELETED, mp, key, NULL); - return delitem_common(mp, hash, ix, old_value, new_version); + delitem_common(mp, hash, ix, old_value, new_version); + return 0; } int @@ -2625,8 +2625,12 @@ delitemif_lock_held(PyObject *op, PyObject *key, return -1; mp = (PyDictObject *)op; ix = _Py_dict_lookup(mp, key, hash, &old_value); - if (ix == DKIX_ERROR) + if (ix == DKIX_ERROR) { return -1; + } + if (ix == DKIX_EMPTY || old_value == NULL) { + return 0; + } res = predicate(old_value, arg); if (res == -1) @@ -2636,7 +2640,8 @@ delitemif_lock_held(PyObject *op, PyObject *key, PyInterpreterState *interp = _PyInterpreterState_GET(); uint64_t new_version = _PyDict_NotifyEvent( interp, PyDict_EVENT_DELETED, mp, key, NULL); - return delitem_common(mp, hash, ix, old_value, new_version); + delitem_common(mp, hash, ix, old_value, new_version); + return 1; } else { return 0; } From 5c3a8cf1093024483fec8e80069b64889a1c7453 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 23 Jul 2024 16:39:28 +0000 Subject: [PATCH 3/3] Changes from review --- Objects/dictobject.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 4bb2824c4dcf68..ee88576cc77dec 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2656,10 +2656,7 @@ _PyDict_DelItemIf(PyObject *op, PyObject *key, int (*predicate)(PyObject *value, void *arg), void *arg) { - if (!PyDict_Check(op)) { - PyErr_BadInternalCall(); - return -1; - } + assert(PyDict_Check(op)); int res; Py_BEGIN_CRITICAL_SECTION(op); res = delitemif_lock_held(op, key, predicate, arg);