-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
gh-137400: Fix thread-safety issues when profiling all threads #137518
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
base: main
Are you sure you want to change the base?
Conversation
There were a few thread-safety issues when profiling or tracing all threads via PyEval_SetProfileAllThreads or PyEval_SetTraceAllThreads: * The loop over thread states could crash if a thread exits concurrently (in both the free threading and default build) * The modification of `c_profilefunc` and `c_tracefunc` wasn't thread-safe on the free threading build.
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 09b28c8 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F137518%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
@@ -387,6 +413,38 @@ def noop(): | |||
|
|||
self.observe_threads(noop, buf) | |||
|
|||
def test_trace_concurrent(self): | |||
# Test calling a function concurrently from a tracing and a non-tracing |
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.
What was the behavior of this test when this was not fixed? Will it crash because the code object is partially instrumented, or will we miss some trace func calls because the code object is not instrumented at all?
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.
Mostly it showed up as data races reported by TSan. I don't think I saw any crashes for this one.
The test above (SetProfileAllThreadsMultiThreaded
) would crash before this PR.
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.
For clarification, this test is related to the change to _MAYBE_INSTRUMENT
with int check_instrumentation = (tstate->tracing == 0);
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.
A few minor comments
assert(is_tstate_valid(current_tstate)); | ||
assert(current_tstate->interp == interp); | ||
|
||
if (_PySys_Audit(current_tstate, "sys.setprofile", NULL) < 0) { |
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.
Shouldn't this be sys._setprofileallthreads
?
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 it's less risky to keep the auditing even name the same as it was before.
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
There were a few thread-safety issues when profiling or tracing all threads via PyEval_SetProfileAllThreads or PyEval_SetTraceAllThreads:
c_profilefunc
andc_tracefunc
wasn't thread-safe on the free threading build.sys._setprofileallthreads
race condition #137400