gh-120858: PyDict_Next should not lock the dict (#120859)

PyDict_Next no longer locks the dictionary in the free-threaded build. Locking
around individual PyDict_Next calls is not sufficient because the function
returns borrowed references and because it allows concurrent modifications
during the iteraiton loop.

The internal locking also interferes with correct external synchronization
because it may suspend outer critical sections created by the caller.

diff --git a/Doc/c-api/dict.rst b/Doc/c-api/dict.rst
index 49a78583a6fe26512872c8687b03411e02cde95d..b4fdf47bc915bdab36f489d6d8459a6c4ec38681 100644 (file)
--- a/Doc/c-api/dict.rst
+++ b/Doc/c-api/dict.rst
@@ -290,6 +290,17 @@ Dictionary Objects
           Py_DECREF(o);
       }
 
+   The function is not thread-safe in the :term:`free-threaded <free threading>`
+   build without external synchronization.  You can use
+   :c:macro:`Py_BEGIN_CRITICAL_SECTION` to lock the dictionary while iterating
+   over it::
+
+      Py_BEGIN_CRITICAL_SECTION(self->dict);
+      while (PyDict_Next(self->dict, &pos, &key, &value)) {
+          ...
+      }
+      Py_END_CRITICAL_SECTION();
+
 
 .. c:function:: int PyDict_Merge(PyObject *a, PyObject *b, int override)
 

diff --git a/Doc/howto/free-threading-extensions.rst b/Doc/howto/free-threading-extensions.rst
index 080170de1e5d16c893b6a99c49cf5afe47d4302c..1ba91b09516f9c738bc7d89db587689bf1c5f349 100644 (file)
--- a/Doc/howto/free-threading-extensions.rst
+++ b/Doc/howto/free-threading-extensions.rst
@@ -114,6 +114,24 @@ Containers like :c:struct:`PyListObject`,
 in the free-threaded build.  For example, the :c:func:`PyList_Append` will
 lock the list before appending an item.
 
+.. _PyDict_Next:
+
+``PyDict_Next``
+'''''''''''''''
+
+A notable exception is :c:func:`PyDict_Next`, which does not lock the
+dictionary.  You should use :c:macro:`Py_BEGIN_CRITICAL_SECTION` to protect
+the dictionary while iterating over it if the dictionary may be concurrently
+modified::
+
+    Py_BEGIN_CRITICAL_SECTION(dict);
+    PyObject *key, *value;
+    Py_ssize_t pos = 0;
+    while (PyDict_Next(dict, &pos, &key, &value)) {
+        ...
+    }
+    Py_END_CRITICAL_SECTION();
+
 
 Borrowed References
 ===================
@@ -141,7 +159,7 @@ that return :term:`strong references <strong reference>`.
 +-----------------------------------+-----------------------------------+
 | :c:func:`PyDict_SetDefault`       | :c:func:`PyDict_SetDefaultRef`    |
 +-----------------------------------+-----------------------------------+
-| :c:func:`PyDict_Next`             | no direct replacement             |
+| :c:func:`PyDict_Next`             | none (see :ref:`PyDict_Next`)     |
 +-----------------------------------+-----------------------------------+
 | :c:func:`PyWeakref_GetObject`     | :c:func:`PyWeakref_GetRef`        |
 +-----------------------------------+-----------------------------------+

diff --git a/Misc/NEWS.d/next/C API/2024-06-21-16-41-21.gh-issue-120858.Z5_-Mn.rst b/Misc/NEWS.d/next/C API/2024-06-21-16-41-21.gh-issue-120858.Z5_-Mn.rst
new file mode 100644 (file)
index 0000000..b5df2a5
--- /dev/null
+++ b/Misc/NEWS.d/next/C API/2024-06-21-16-41-21.gh-issue-120858.Z5_-Mn.rst
@@ -0,0 +1,3 @@
+:c:func:`PyDict_Next` no longer locks the dictionary in the free-threaded
+build.  The locking needs to be done by the caller around the entire iteration
+loop.

diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index 5529e527d8bea3b91e37d3f73f0be94bf4e94ad6..5d325465608f99333153e87b4badad917b0e3901 100644 (file)
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -2808,8 +2808,6 @@ _PyDict_Next(PyObject *op, Py_ssize_t *ppos, PyObject **pkey,
     if (!PyDict_Check(op))
         return 0;
 
-    ASSERT_DICT_LOCKED(op);
-
     mp = (PyDictObject *)op;
     i = *ppos;
     if (_PyDict_HasSplitTable(mp)) {
@@ -2882,11 +2880,7 @@ _PyDict_Next(PyObject *op, Py_ssize_t *ppos, PyObject **pkey,
 int
 PyDict_Next(PyObject *op, Py_ssize_t *ppos, PyObject **pkey, PyObject **pvalue)
 {
-    int res;
-    Py_BEGIN_CRITICAL_SECTION(op);
-    res = _PyDict_Next(op, ppos, pkey, pvalue, NULL);
-    Py_END_CRITICAL_SECTION();
-    return res;
+    return _PyDict_Next(op, ppos, pkey, pvalue, NULL);
 }