Skip to content

GH-96071: fix deadlock in PyGILState_Ensure #96107

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

Closed
wants to merge 2 commits into from
Closed

GH-96071: fix deadlock in PyGILState_Ensure #96107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4eb4ae6
fix deadlock
kumaraditya303 Aug 19, 2022
e0b93d6
📜🤖 Added by blurb_it.
blurb-it[bot] Aug 19, 2022
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
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Core and Builtins/2022-08-19-06-51-17.gh-issue-96071.mVgPAo.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a deadlock in :c:func:`PyGILState_Ensure` when allocating new thread state. Patch by Kumar Aditya.
7 changes: 6 additions & 1 deletion Python/pystate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,11 +829,16 @@ new_threadstate(PyInterpreterState *interp)
// Every valid interpreter must have at least one thread.
assert(id > 1);
assert(old_head->prev == NULL);

// Unlock before allocating memory.
HEAD_UNLOCK(runtime);
Copy link
Member

Choose a reason for hiding this comment

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

If possible, I prefer to move the allocation at the beginning as @tom-pytel is suggesting because is much easier to reason about. Here you are interrupting the critical section in the middle which is potentially incorrect depending on what tracemalloc does (currently it done not do anything fancy like spawning threads but you are implicitly relyi ng on that contract).

Copy link
Contributor Author

@kumaraditya303 kumaraditya303 Aug 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Whatever tracemalloc or any other memory allocator may do (even spawn threads), the thread unique counter is protected above by the mutex and we only unlock for memory allocation, after that we read with mutex locked so this is re-entrant safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you concerned about this I can change it to always allocate memory and free it if unused but I would like to avoid it unless necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean and is great that you want to ensure maximum performance but I personally think that is premature optimization. Creating thread states is not a performance-critical part of the code and this problem with the allocation being unused only happens if tracemalloc is in use, which impacts performance by a ton already, so this is a negligible advantage at the cost of making reasoning about the correctness of the code harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating thread states is not a performance-critical part of the code and this problem with the allocation being unused only happens if tracemalloc is in use,

Just a clarification that the allocation can be unused even without tracemalloc if interp->threads.head is NULL which is the common case of this function. But I see your point and will change. Thanks for your feedback.

tstate = alloc_threadstate();
HEAD_LOCK(runtime);
if (tstate == NULL) {
goto error;
}
// Read interp->threads.head again as another thread could
// have created a thread state while we were allocating memory.
old_head = interp->threads.head;
// Set to _PyThreadState_INIT.
memcpy(tstate,
&initial._main_interpreter._initial_thread,
Expand Down