GeezerVison is showing github.com — gh-117657: Fix some races in tracebacks by Fidget-Spinner · Pull Request #121748 · python/cpython
Conversation
TSAN reported race:
WARNING: ThreadSanitizer: data race (pid=31237)
Read of size 8 at 0x7fffb9ac6148 by thread T333:
#0 PyException_GetTraceback /home/ken/Documents/GitHub/cpython/Objects/exceptions.c:415:12 (python+0x27c20b) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#1 _PyErr_SetObject /home/ken/Documents/GitHub/cpython/Python/errors.c:242:14 (python+0x52cf07) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#2 do_raise /home/ken/Documents/GitHub/cpython/Python/ceval.c:2031:5 (python+0x4dd3c9) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#3 _PyEval_EvalFrameDefault /home/ken/Documents/GitHub/cpython/Python/generated_cases.c.h:5532:21 (python+0x4dd3c9)
#4 _PyEval_EvalFrame /home/ken/Documents/GitHub/cpython/./Include/internal/pycore_ceval.h:119:16 (python+0x4b39b3) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#5 _PyEval_Vector /home/ken/Documents/GitHub/cpython/Python/ceval.c:1866:12 (python+0x4b39b3)
#6 _PyFunction_Vectorcall /home/ken/Documents/GitHub/cpython/Objects/call.c (python+0x25b2ac) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#7 _PyObject_VectorcallTstate /home/ken/Documents/GitHub/cpython/./Include/internal/pycore_call.h:167:11 (python+0x261410) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#8 method_vectorcall /home/ken/Documents/GitHub/cpython/Objects/classobject.c:70:20 (python+0x25f8fd) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#9 _PyVectorcall_Call /home/ken/Documents/GitHub/cpython/Objects/call.c:273:16 (python+0x25ac2a) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#10 _PyObject_Call /home/ken/Documents/GitHub/cpython/Objects/call.c:348:16 (python+0x25aed3) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#11 PyObject_Call /home/ken/Documents/GitHub/cpython/Objects/call.c:373:12 (python+0x25b0b7) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#12 thread_run /home/ken/Documents/GitHub/cpython/./Modules/_threadmodule.c:337:21 (python+0x6cbcd8) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#13 pythread_wrapper /home/ken/Documents/GitHub/cpython/Python/thread_pthread.h:243:5 (python+0x604b0b) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
Previous write of size 8 at 0x7fffb9ac6148 by main thread:
#0 BaseException_set_tb /home/ken/Documents/GitHub/cpython/Objects/exceptions.c:327:9 (python+0x27c4d8) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#1 PyException_SetTraceback /home/ken/Documents/GitHub/cpython/Objects/exceptions.c:422:12 (python+0x27c2fc) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#2 PyTraceBack_Here /home/ken/Documents/GitHub/cpython/Python/traceback.c:273:5 (python+0x605636) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#3 _PyEval_EvalFrameDefault /home/ken/Documents/GitHub/cpython/Python/ceval.c:927:17 (python+0x4e365d) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#4 _PyEval_EvalFrame /home/ken/Documents/GitHub/cpython/./Include/internal/pycore_ceval.h:119:16 (python+0x4b39b3) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#5 _PyEval_Vector /home/ken/Documents/GitHub/cpython/Python/ceval.c:1866:12 (python+0x4b39b3)
and
WARNING: ThreadSanitizer: data race (pid=31237)
Atomic read of size 4 at 0x7fffb9a8439c by thread T333:
#0 _Py_atomic_load_uint32_relaxed /home/ken/Documents/GitHub/cpython/./Include/cpython/pyatomic_gcc.h:339:10 (python+0x27c222) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#1 Py_INCREF /home/ken/Documents/GitHub/cpython/./Include/refcount.h:227:22 (python+0x27c222)
#2 Py_XINCREF /home/ken/Documents/GitHub/cpython/./Include/refcount.h:449:9 (python+0x27c222)
#3 _Py_XNewRef /home/ken/Documents/GitHub/cpython/./Include/refcount.h:481:5 (python+0x27c222)
#4 PyException_GetTraceback /home/ken/Documents/GitHub/cpython/Objects/exceptions.c:415:12 (python+0x27c222)
#5 _PyErr_SetObject /home/ken/Documents/GitHub/cpython/Python/errors.c:242:14 (python+0x52cf07) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#6 do_raise /home/ken/Documents/GitHub/cpython/Python/ceval.c:2031:5 (python+0x4dd3c9) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#7 _PyEval_EvalFrameDefault /home/ken/Documents/GitHub/cpython/Python/generated_cases.c.h:5532:21 (python+0x4dd3c9)
#8 _PyEval_EvalFrame /home/ken/Documents/GitHub/cpython/./Include/internal/pycore_ceval.h:119:16 (python+0x4b39b3) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#9 _PyEval_Vector /home/ken/Documents/GitHub/cpython/Python/ceval.c:1866:12 (python+0x4b39b3)
#10 _PyFunction_Vectorcall /home/ken/Documents/GitHub/cpython/Objects/call.c (python+0x25b2ac) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#11 _PyObject_VectorcallTstate /home/ken/Documents/GitHub/cpython/./Include/internal/pycore_call.h:167:11 (python+0x261410) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#12 method_vectorcall /home/ken/Documents/GitHub/cpython/Objects/classobject.c:70:20 (python+0x25f8fd) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#13 _PyVectorcall_Call /home/ken/Documents/GitHub/cpython/Objects/call.c:273:16 (python+0x25ac2a) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#14 _PyObject_Call /home/ken/Documents/GitHub/cpython/Objects/call.c:348:16 (python+0x25aed3) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#15 PyObject_Call /home/ken/Documents/GitHub/cpython/Objects/call.c:373:12 (python+0x25b0b7) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#16 thread_run /home/ken/Documents/GitHub/cpython/./Modules/_threadmodule.c:337:21 (python+0x6cbcd8) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#17 pythread_wrapper /home/ken/Documents/GitHub/cpython/Python/thread_pthread.h:243:5 (python+0x604b0b) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
Previous write of size 4 at 0x7fffb9a8439c by main thread:
#0 new_reference /home/ken/Documents/GitHub/cpython/Objects/object.c:2367:22 (python+0x32793a) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#1 _Py_NewReference /home/ken/Documents/GitHub/cpython/Objects/object.c:2386:5 (python+0x32793a)
#2 _PyObject_Init /home/ken/Documents/GitHub/cpython/./Include/internal/pycore_object.h:271:5 (python+0x544ee6) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#3 _PyObject_GC_New /home/ken/Documents/GitHub/cpython/Python/gc_free_threading.c:1677:5 (python+0x544ee6)
#4 tb_create_raw /home/ken/Documents/GitHub/cpython/Python/traceback.c:53:10 (python+0x605418) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#5 _PyTraceBack_FromFrame /home/ken/Documents/GitHub/cpython/Python/traceback.c:257:12 (python+0x605306) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#6 PyTraceBack_Here /home/ken/Documents/GitHub/cpython/Python/traceback.c:267:23 (python+0x6055a4) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
#7 _PyEval_EvalFrameDefault /home/ken/Documents/GitHub/cpython/Python/ceval.c:927:17 (python+0x4e365d) (BuildId: e8ff96bc7d1e595a25cb4534a571573eaa53d4f4)
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
LGTM, but I'd suggest to have another reviewer with more knowledge of the thread safe patterns.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the races in our tests involving tracebacks are broader than this.
The critical sections on getters and setters probably still makes sense, but I don't think they'll fix the traceback races.
The big problem is using sys._current_exceptions() to access exceptions for other threads while they are running. That function runs in a stop-the-world context, but the exception tracebacks are created lazily when other threads are resumed. I think that will still not be thread-safe even with this change.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
|
op->ob_refcnt = 1;
|
|
|
#else
|
|
|
op->ob_tid = _Py_ThreadId();
|
|
|
_Py_atomic_store_uintptr_release(&op->ob_tid, _Py_ThreadId());
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to do this, at least not for now. I want to wait until we fix all the other races before we address new_reference because there are perf implications and the race is in practice benign.
The "release" memory order is also stronger (and more expensive on arm64) than necessary.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
All commit authors signed the Contributor License Agreement.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
This is fixed on main under #128421
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
picnixz
picnixz left review comments
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|
colesbury
left a comment