From: Sam Gross <colesbury@gmail.com>
Date: Mon, 24 Jun 2024 18:15:15 +0000 (-0400)
Subject: gh-120858: PyDict_Next should not lock the dict (#120859)
X-Git-Tag: v3.14.0a1~1341
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=375b723d5873f948696c7e85a97f4778d9e00ff0;p=thirdparty%2FPython%2Fcpython.git

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 49a78583a6fe..b4fdf47bc915 100644
--- 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 080170de1e5d..1ba91b09516f 100644
--- 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
index 000000000000..b5df2a567b9d
--- /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 5529e527d8be..5d325465608f 100644
--- 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);
 }