-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
gh-144763: Don't detach the GIL in tracemalloc #144779
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.
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
Changes from 4 commits
00c69d2
a91fc22
78ceae3
609f7fb
ff3241a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix a race condition in :mod:`tracemalloc`: it no longer detachs the attached | ||
| thread state to acquire its internal lock. Patch by Victor Stinner. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,7 @@ static int _PyTraceMalloc_TraceRef(PyObject *op, PyRefTracerEvent event, | |
| the GIL held from PyMem_RawFree(). It cannot acquire the lock because it | ||
| would introduce a deadlock in _PyThreadState_DeleteCurrent(). */ | ||
| #define tables_lock _PyRuntime.tracemalloc.tables_lock | ||
| #define TABLES_LOCK() PyMutex_Lock(&tables_lock) | ||
| #define TABLES_LOCK() PyMutex_LockFlags(&tables_lock, _Py_LOCK_DONT_DETACH) | ||
| #define TABLES_UNLOCK() PyMutex_Unlock(&tables_lock) | ||
|
|
||
|
|
||
|
|
@@ -224,13 +224,20 @@ tracemalloc_get_frame(_PyInterpreterFrame *pyframe, frame_t *frame) | |
| assert(PyStackRef_CodeCheck(pyframe->f_executable)); | ||
| frame->filename = &_Py_STR(anon_unknown); | ||
|
|
||
| int lineno = PyUnstable_InterpreterFrame_GetLine(pyframe); | ||
| int lineno = -1; | ||
| PyCodeObject *code = _PyFrame_GetCode(pyframe); | ||
| // PyUnstable_InterpreterFrame_GetLine() cannot but used, since it uses | ||
| // a critical section which can trigger a deadlock. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the problem is that critical sections requires an active thread state and this code can be called with detached thread state iirc.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tracemalloc_get_frame() is called with an attached thread state, see the caller traceback_get_frames() which has the code: PyThreadState *tstate = _PyThreadState_GET();
assert(tstate != NULL);Example of deadlock when running #144763 (comment) reproducer on a free-threaded build:
Locks:
Thread A and thread B want to use => deadlock :-(
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are probably some thread-safety issues here if the code object is being concurrently instrumented or deinstrumented. I don't see a better way of handling it, so let's leave a TODO or note. |
||
| int lasti = _PyFrame_SafeGetLasti(pyframe); | ||
| if (lasti >= 0) { | ||
| lineno = _PyCode_SafeAddr2Line(code, lasti); | ||
| } | ||
| if (lineno < 0) { | ||
| lineno = 0; | ||
| } | ||
| frame->lineno = (unsigned int)lineno; | ||
|
|
||
| PyObject *filename = _PyFrame_GetCode(pyframe)->co_filename; | ||
| PyObject *filename = code->co_filename; | ||
| if (filename == NULL) { | ||
| #ifdef TRACE_DEBUG | ||
| tracemalloc_error("failed to get the filename of the code object"); | ||
|
|
@@ -863,7 +870,8 @@ _PyTraceMalloc_Stop(void) | |
| TABLES_LOCK(); | ||
|
|
||
| if (!tracemalloc_config.tracing) { | ||
| goto done; | ||
| TABLES_UNLOCK(); | ||
| return; | ||
| } | ||
|
|
||
| /* stop tracing Python memory allocations */ | ||
|
|
@@ -880,10 +888,12 @@ _PyTraceMalloc_Stop(void) | |
| raw_free(tracemalloc_traceback); | ||
| tracemalloc_traceback = NULL; | ||
|
|
||
| (void)PyRefTracer_SetTracer(NULL, NULL); | ||
|
|
||
| done: | ||
| TABLES_UNLOCK(); | ||
|
|
||
| // Call it after TABLES_UNLOCK() since it calls _PyEval_StopTheWorldAll() | ||
| // which would lead to a deadlock with TABLES_LOCK() which doesn't detaches | ||
| // the thread state. | ||
|
vstinner marked this conversation as resolved.
Outdated
|
||
| (void)PyRefTracer_SetTracer(NULL, NULL); | ||
| } | ||
|
|
||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.