X Tutup
The Wayback Machine - https://web.archive.org/web/20240907000757/https://github.com/python/cpython/issues/120858
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PyDict_Next should not lock the dict #120858

Closed
colesbury opened this issue Jun 21, 2024 · 0 comments
Closed

PyDict_Next should not lock the dict #120858

colesbury opened this issue Jun 21, 2024 · 0 comments
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-free-threading type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented Jun 21, 2024 •
edited by bedevere-app bot
Loading

Bug report

PyDict_Next currently wraps _PyDict_Next in a critical section. We shouldn't do this -- the locking needs to be external to the call.

  1. It's not sufficient to lock the dict just for each _PyDict_Next call because we return borrowed references and because pos becomes meaningless if the dictionary gets resized or rehashed.
  2. It interferes with externally locking the dict because the inner critical sections can suspend the outer ones. In other words, if the caller use a critical section to lock the dict for multiple iterations, this will break that.

cpython/Objects/dictobject.c

Lines 2883 to 2890 in 8f17d69

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;
}

cc @DinoV

Linked PRs
@colesbury colesbury added type-bug An unexpected behavior, bug, or error 3.13 bugs and security fixes topic-free-threading 3.14 new features, bugs and security fixes labels Jun 21, 2024
@colesbury colesbury changed the title PyDict_Next should include begin a critical section PyDict_Next should not begin a critical section Jun 21, 2024
@colesbury colesbury changed the title PyDict_Next should not begin a critical section PyDict_Next should not lock the dict Jun 21, 2024
colesbury added a commit to colesbury/cpython that referenced this issue Jun 21, 2024
@colesbury
pythongh-120858: PyDict_Next should not lock the dict
2cbcde2 
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.
@bedevere-app bedevere-app bot mentioned this issue Jun 21, 2024
Merged
colesbury added a commit that referenced this issue Jun 24, 2024
@colesbury
gh-120858: PyDict_Next should not lock the dict (#120859)
375b723 
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.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 24, 2024
@colesbury @miss-islington
pythongh-120858: PyDict_Next should not lock the dict (pythonGH-120859)
f103aeb 
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.
(cherry picked from commit 375b723)

Co-authored-by: Sam Gross <colesbury@gmail.com>
@bedevere-app bedevere-app bot mentioned this issue Jun 24, 2024
Merged
colesbury added a commit that referenced this issue Jun 24, 2024
@miss-islington @colesbury
[3.13] gh-120858: PyDict_Next should not lock the dict (GH-120859) (#…
0a77058 
…120964)

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.
(cherry picked from commit 375b723)

Co-authored-by: Sam Gross <colesbury@gmail.com>
mrahtz pushed a commit to mrahtz/cpython that referenced this issue Jun 30, 2024
@colesbury @mrahtz
pythongh-120858: PyDict_Next should not lock the dict (python#120859)
3bbe605 
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.
noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
@colesbury @noahbkim
pythongh-120858: PyDict_Next should not lock the dict (python#120859)
685b3d2 
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.
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
@colesbury @estyxx
pythongh-120858: PyDict_Next should not lock the dict (python#120859)
202d1a8 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.13 bugs and security fixes 3.14 new features, bugs and security fixes topic-free-threading type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@colesbury
X Tutup